Desugar for-loops in the AST #3392
Conversation
CohenArthur
force-pushed
the
for-loop-desugar-ast
branch
from
da8f074 to
1ad3039
Compare
|
Although we wanted to do this desugar in the HIR, i see this as our first pass so that we show that our internals are 'working' and i bet down the line we will figure it out. I bet there is a way that during HIR lowering we actually decouple the name resolution entirely by linking name resolution information in a different way to not require the ast_node_id but link directly the HIR node or something else. It would be a fairly big refactor which is basically not worth the effort right now but would be a good google summer of code project we could scope out. |
|
As always great work sir |
CohenArthur
force-pushed
the
for-loop-desugar-ast
branch
2 times, most recently
from
6977a54 to
76d0b09
Compare
|
|
||
| fn next(&mut self) -> Option<A> { | ||
| if self.start < self.end { | ||
| // We check for overflow here, even though it can't actually |
There was a problem hiding this comment.
i'd love to know more about this bit in interators in rustc why this was introduced i wonder what the test case was
| MatchCase | ||
| DesugarForLoops::DesugarCtx::make_continue_arm () | ||
| { | ||
| auto val = builder.identifier_pattern ("val"); |
There was a problem hiding this comment.
Should we use some underscore before val ?
There was a problem hiding this comment.
ah, right - I think we should use an even weirder character like a . or a # to really prevent it from leaking out
There was a problem hiding this comment.
Tbh I don't really like the fact that we could name it, it should really be some kind of long unameable identifier that changes depending on the locus and a few other factor or something along this. Not sure how we should do this properly.
There was a problem hiding this comment.
well we can't name it if theidentifier starts with # because it won't parse. so a user cannot name it. the other option is to add extra hygiene checks (e.g. having a special-cased location for generated code like rustc does) or generating those identifiers in the HIR, after name resolution has gone through
There was a problem hiding this comment.
we could also use $__next or .__next or any other kind of invalid identifier which won't parse in Rust - that's enough to prevent the user from coliding/using these identifiers
CohenArthur
force-pushed
the
for-loop-desugar-ast
branch
from
76d0b09 to
ea2be9c
Compare
CohenArthur
force-pushed
the
for-loop-desugar-ast
branch
2 times, most recently
from
462be8d to
759a3d4
Compare
gcc/rust/ChangeLog: * ast/rust-ast-builder.h: Mark all arguments as &&. * ast/rust-ast-builder.cc (Builder::let): Likewise.
gcc/rust/ChangeLog: * ast/rust-desugar-for-loops.cc: New file. * ast/rust-desugar-for-loops.h: New file. * hir/rust-ast-lower-expr.cc (ASTLoweringExpr::visit): Make lowering of for-loops an unreachable. * Make-lang.in: Compile it. gcc/testsuite/ChangeLog: * rust/compile/for-loop1.rs: New test. * rust/compile/for-loop2.rs: New test. * rust/execute/torture/for-loop1.rs: New test. * rust/execute/torture/for-loop2.rs: New test. * rust/compile/nr2/exclude: Exclude for-loop1.rs
gcc/rust/ChangeLog: * rust-session-manager.cc (Session::compile_crate): Call the visitor.
I'm not a big fan of mutating the AST to do this, however this enables us to desugar for-loops easily. The alternative is desugaring during the lowering phase, which has the issue of not inserting any information about the generated nodes into our name resolver. This leads to failures during typechecking which is annoying.
The best thing would be to figure out a way to insert resolved nodes when we do the Desugar, but it's a bit tricky.