[JuliaLowering] Refactor scope resolution pass #60316
Open
+955
−900
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Soft scope
We currently treat flisp's
'(block (softscope true))as a scope that "issoft", but this isn't correct. It should behave more like a toggle, where if the
scope surrounding
(softscope true)is the top-level thunk, neutral scopes notprotected by a hard scope become permeable to already-defined globals. I've
added
K"use_softscope_if_toplevel"for this.Fixes JuliaLang/JuliaLowering.jl#101.
JuliaLowering.activate!()should be much more usable in the REPL now, asglobals won't be accidentally eaten by the "soft scope."
Shadowing behaviour
Found while cleaning up the lhs-resolution step with the change above. This PR
allows static parameters to be shadowed by globals and locals as long as they're
explicit (and not in the same scope). flisp allows this with globals, and the
explicit locals that desugar to
local-defforms (which JuliaLowering doesn'thave).
This change is more permissive than flisp in the local case, since after looking
into why shadowing was disallowed I realized it was just was just to prevent
assignment to the static parameter (#32623). The flisp fix leads to some funny
behaviour:
Bindings/LambdaBindings
In figuring out how to use the variable bookkeeping system, I ran into inaccuracies.
LambdaBindingsis currently a per-lambda map from unique variable to fourflags:
captured,read,assigned, andcalled. I think (but correct me ifI'm wrong @c42f) this was a misinterpretation of the holy text: in flisp
lowering, a local variable captured by some other lambda does show up in both
lambdas' variable lists, but is the same underlying object, and flag mutations
on one variable are seen by all lambdas.
I tried to think of other reasons for tracking vinfo per lambda within lowering,
but if we're doing something about the capture-boxing issue, we need something
more complex anyway.
This PR moves all vinfo to
BindingInfoand deletes the incorrect bookkeepingin
LambdaBindings. We still need to have a per-lambda flag for capturedness(different from the variable-level
captvinfo flag). With the added flags,I've just made BindingInfo mutable since our previous workflow (BindingId is an
index into a vector; mutate the vector) doesn't give us the benefits of
immutability anyway.
Enhancements
answer questions like "what names are available at my cursor?" Recreating
this previously-discarded information was a bit hacky! [1] [2]
TODO
K"local"in desugaring is dubious in some placesexpr_compat_modeto the scope analysis context, but we still needto implement flisp hygiene exemptions for globals (see note in test/scopes.jl).
#self#argument still has extra flags set insome cases. This is an existing desugaring bug with a comment that took me
too long to find: we shouldnn't be using the same
#self#binding formultiple methods defined by one function body