-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
[JuliaLowering] Refactor scope resolution pass #60316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
"get" is clearer to me that the binding is the output rather than the input,
that it definitely exists, and that the ID resolves uniquely
I know it's a goal to make some of these clearer, but using the same string as
Expr (and using the fall-through case in conversion) will make some planned
code movement easier
912ff3c to
47eb8e9
Compare
topolarity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to do another once-over of the core pass logic, but this should be useful (mostly very minor) feedback in the mean time.
| binfo.is_assigned_once && print(io, ", is_assigned_once") | ||
| binfo.is_captured && print(io, ", is_captured") | ||
| binfo.is_always_defined && print(io, ", is_always_defined") | ||
| binfo.is_used_undef && print(io, ", is_used_undef") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this no longer round-trip when pasting into, e.g., the REPL?
Maybe worth it for compactness, but would be nice to restore.
| # errors like the conflict cases (two of the same decl should never do | ||
| # anything, and the user might be expecting two variables). | ||
| @testset "global,global" begin | ||
| s = "function (); global g; global g; 1; end" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JuliaLowering seems to accept this example, but flisp does not:
julia> JuliaLowering.include_string(TestMod,"""
function (); global g; const g = 1; end
""")
#anon###0 (generic function with 1 method)
julia> function (); global g; const g = 1; end
ERROR: syntax: `global const` declaration not allowed inside function around REPL[5]:1
Stacktrace:
[1] top-level scope
@ REPL[5]:1somehow global g gets JL to look the other way
|
|
||
| # For each distinct outer and inner scope, and each kind of variable in the | ||
| # outer scope, set the same name to true from the inner scope | ||
| @testset "Behaviour of `=` in local scope (shadow or assign-existing)" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love exhaustive testing like this, even if it's a bit "shallow" w.r.t. the AST depth.
Really nice!
| const id::IdTag # Unique integer identifying this binding | ||
| const name::String | ||
| const kind::Symbol # :local :global :argument :static_parameter | ||
| const node_id::Int # ID of associated K"BindingId" node in the syntax graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, is node_id <-> id a 1:1 mapping?
| # Reconstruct the SyntaxTree for this binding. We keep only the node_id | ||
| # here, because that's got a concrete type. Whereas if we stored SyntaxTree | ||
| # that would contain the type of the graph used in the pass where the | ||
| # bindings were created and we'd need to call reparent(), etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by these comments.. this implementation seems very similar to reparent()
I know they're not yours, but can you explain where it's different?
|
|
||
| # Compute fields for a closure type, one field for each captured variable. | ||
| function closure_type_fields(ctx, srcref, closure_binds, is_opaque) | ||
| capture_ids = Vector{IdTag}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another micro-optimization, but Vector{IdTag} should probably be BitSet
(since these are small and contiguous)
| # assigned to at top level, or passing the defined-and-owned-global check. | ||
| defined_toplevel_globals::Set{NameKey} | ||
| enable_soft_scopes::Bool | ||
| expr_compat_mode::Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unused?
| end | ||
|
|
||
| function _resolve_scopes(ctx, ex::SyntaxTree) | ||
| function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope)) | |
| function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope::Union{Nothing,ScopeInfo})) |
IIUC you assume this, so best to mark it
(should also dramatically help inference, given the @nospecialize)
| end | ||
| elseif etype == "global_toplevel_only" | ||
| if !ctx.scope_stack[end].is_toplevel_global_scope | ||
| if !is_top_scope(scope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this throw a MethodError is scope isa Nothing?
| parent(ctx, scope::ScopeInfo) = is_top_scope(scope) ? nothing : | ||
| ctx.scopes[scope.parent_id] | ||
|
|
||
| enclosing_lambda(ctx, lb::LambdaBindings) = ctx.scopes[lb.scope_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is somewhat confusing, since it's a weird name if you come to this operation by saying "I'd like to get the ScopeInfo corresponding to the lambda."
| enclosing_lambda(ctx, lb::LambdaBindings) = ctx.scopes[lb.scope_id] |
I'd see whether enclosing_lambda(ctx, scope(ctx, lb)) (or similar) is not too awkward instead
| # Every lexical scope, indexed by ScopeId | ||
| scopes::Vector{ScopeInfo} | ||
| # Current stack of scopes to look for names in, innermost scope last | ||
| scope_stack::Vector{ScopeId} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this identical to the sequence of scopes you get if you follow the parent(ctx, scope) chain?
topolarity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the right approach (and indeed closer to flisp) to update shared bindings across all lambdas simultaneously.
Changes are looking good to me. Nice work @mlechu!
Feel free to take or drop the nits / ideas, as desired.
| throw(LoweringError(arg, "Unexpected lambda arg kind")) | ||
| # If `b` is local and not yet recorded in the lambda bindings, mark it as | ||
| # `capt`. Also, (if `capt==true`), add it to any parent lambdas. | ||
| function record_lambda_var!(ctx, scope::Union{ScopeInfo, LambdaBindings}, b, capt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function record_lambda_var!(ctx, scope::Union{ScopeInfo, LambdaBindings}, b, capt) | |
| function record_lambda_var!(ctx, scope::ScopeInfo, b; is_captured) |
| end | ||
| id | ||
| b = _new_binding(ctx, ex, nk.name, bk; mod, kws...) | ||
| home_scope.vars[nk] = b.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| home_scope.vars[nk] = b.id | |
| declaration_scope.vars[nk] = b.id |
| # Usually, globals in the top scope are ignored. These are globals that may | ||
| # be assigned to without the `global` keyword in soft scopes due to being | ||
| # assigned to at top level, or passing the defined-and-owned-global check. | ||
| defined_toplevel_globals::Set{NameKey} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually do prefer the old implicit name or implicitly_scoped_globals? "top-level global" is a bit redundant, IIUC?
| elseif old_k === new_k | ||
| (new_k === :global || new_k === :local) && return bid | ||
| throw(LoweringError(ex, "function $(_var_str(new_k)) name not unique")) | ||
| # See note in test/scopes.jl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some identifier to the note you mean?
| # mutable nameless bindings may be introduced in desugaring | ||
| maybe_declare_in_scope!(ctx, scope, SyntaxTree(ctx.graph, node_id), | ||
| get_binding(ctx, bid).kind) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like it should be possible to unify this with the logic for scope.assignments below, since semantically they aren't any different iiuc?
Not a required re-work by any means, but it seems like it might be nice
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"softscope"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