-
Notifications
You must be signed in to change notification settings - Fork 9
Make consts, functions distinct from assignments in scope resolution #115
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: main
Are you sure you want to change the base?
Conversation
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.
Thanks for working on this. #101 definitely shows the soft scope handling is broken. I'd like it if we could do that in a separate PR from deciding what to do about const definitions (functions and explicit consts) defined by macros.
I pretty strongly feel that const shouldn't be special as compared to normal assignment in terms of whether it defines a local or global symbol. Obviously we need to accommodate compatibility but I don't want the new system to reproduce things which are arguably bugs if there's a way to avoid it.
test/scopes.jl
Outdated
|
|
||
| @testset "top-level function defs aren't local in macro expansions" begin | ||
| JuliaLowering.include_string(test_mod, "macro_mod.@m function f_nonlocal_1(); 1; end") | ||
| @test isdefined(test_mod.macro_mod, :f_nonlocal_1) |
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.
If the macro itself defines a function I would think it "should" be local for the same reason that assignments are local by default. For example:
macro define_func(outer_def)
quote
function should_be_local()
end
$outer_def
end
end
@define_func function should_be_global() endSo the non-esc version seems like it should be local for the same reasons (ie, not exposing the macro implementation details as global symbols). The same reasoning applies to const.
I'm guessing you found examples where doing the logical thing breaks existing code? It would help if you could list a couple just so I can come to terms with doing the "wrong" thing in the name of compatibility 😅
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.
A possibly-appealing idea would be to use different rules for this, based on whether symbols arise from an old or new-style macro. I think we could do this by recording that information in the scope layer in the same way that we currently record is_macro_expansion?
That way, we don't break old macros but we don't commit to the old and arguably-buggy rules?
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.
The example where this came up was fixed by something else (#114), so I've just removed the macro behaviour change for now in the interest of keeping this PR small.
We'll probably need to implement flisp hygiene exemptions later, though. If you thought this was bad, I think explicitly-declared globals (including abstract/primitive IIRC) unconditionally holepunch their way to the base scope layer.
I'm OK with doing this based on old/new macro status.
Co-authored-by: Claire Foster <[email protected]>
Fixes some of #101. The current scope resolution algorithm counts top-level function and const declarations as bare assignments, but this isn't correct in soft scopes (causing REPL unusability) or unescaped macro expansions.
Right now this PR keeps both in the same bucket, but we may need to split consts and functions later.