cleanup: Remove fallback post-lowering global resolution #57051
Conversation
src/method.c
Outdated
| if (e->head == jl_call_sym && jl_expr_nargs(e) == 3 && | ||
| jl_is_globalref(jl_exprarg(e, 0)) && |
There was a problem hiding this comment.
Do we still care about this hack (replacing the syntax a.b with GlobalRef(a, :b) if a appears to be a module?
There was a problem hiding this comment.
I think that hack has been broken for a long time.
There was a problem hiding this comment.
okay, yeah, do we just clean this up by deleting it too?
julia> @eval Base f() = Base.x
f (generic function with 1 method)
julia> @code_lowered Base.f()
CodeInfo(
@ REPL[10]:1 within `unknown scope`
1 ─ %1 = Base.Base
│ %2 = dynamic Base.getproperty(%1, :x)
└── return %2
)
There was a problem hiding this comment.
Fine by me. We could consider putting it into lowering though.
There was a problem hiding this comment.
Lowering doesn't know the symbol will refers to a module constant, so it had to be when we evaluate the expression here
There was a problem hiding this comment.
Alright, fair enough. In that case, I don't think we can really do this at all. With binding redefinition, our semantics do not allow us to make that decision here.
After recent changes, essentially all global symbol scopes are resolved by lowering, so the code in method.c that replaces symbols by globalrefs is no longer necessary. The one small exception to this were symbols resulting from closure conversion, because these get inserted after the point at which lowering converts symbols to globalrefs. However, in the current design, I think that's basically a bug (and easily addressed by inserting `globalref`s where appropriate). Removing this extra resolution step is not particularly necessary, but for the upcoming binding partition backedges, it removes an ambiguity as to which version of the lowered code to scan. It also partially resolves a very old todo about not performing post-hoc mutation of lowered code (although we still do this for ccall).
Keno
force-pushed
the
kf/nopostlowerresolve
branch
from
6ab8ffd to
f52bdb9
Compare
After recent changes, essentially all global symbol scopes are resolved by lowering, so the code in method.c that replaces symbols by globalrefs is no longer necessary. The one small exception to this were symbols resulting from closure conversion, because these get inserted after the point at which lowering converts symbols to globalrefs. However, in the current design, I think that's basically a bug (and easily addressed by inserting
globalrefs where appropriate). Removing this extra resolution step is not particularly necessary, but for the upcoming binding partition backedges, it removes an ambiguity as to which version of the lowered code to scan. It also partially resolves a very old todo about not performing post-hoc mutation of lowered code (although we still do this for ccall).