-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix the scope to look-up the constant as the receiver type of the macro #5354
Fix the scope to look-up the constant as the receiver type of the macro #5354
Conversation
@makenowjust say in that example the macro did want to access |
@RX14 Using |
It does not make a sense that this example works only if class Foo
macro foo
{{Baz}}
end
class Baz
end
end
class Bar
class Baz
end
def self.foo
Foo.foo
end
end
class FooBar
def self.foo
Foo.foo
end
end
p Bar.foo # => Bar::Baz
p FooBar.foo # compilation error: undefined constant 'Baz' Isn't it? |
@makenowjust but now you have to pass in the outer scope somehow? Macros can reference local variables and local methods, why not local types too. |
This example works. What is problem? class Foo
macro foo(x)
{{x}}
end
macro bar
Baz
end
end
class Bar
class Baz
end
def self.foo
Foo.foo Baz
end
def self.bar
Foo.bar
end
end
p Bar.foo # => Bar::Baz
p Bar.bar # => Bar::Baz This PR fixes the semantic in only macro expansion inside macro. |
@makenowjust ah yes never mind me I was just confused. |
I think what @RX14 fears is that we may not be able to access a 'local' type from the macro system, as module IncludeMe
macro included
STORAGE = [] of _ # Create a 'local' Type in the caller's (includer) scope
end
macro add(node)
{% STORAGE << node %} # Access a 'local' type in the caller's scope, from the macro system
end
end
class Foo
include IncludeMe
# Add some stuff from macros to a 'local' type
add a + b
add :bla
end But it seems to still work, so 👍 |
Can this go forward? |
This fix is correct 👍 |
needs a rebase, sorry for the delay merging |
I merged master into this on GitHub Web UI. Please do squash merge. |
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 a lot @makenowjust 🙏
Fixed #5343. It is another solution than #5349. I believe this PR is better than #5349.
Currently the constant in expansion inside macro is resolved by caller-scope, for example:
It is unexpected because the constant inside method is of course resolved by the receiver type of the method. And so, the second example in #5343 is caused.
This PR fixed scope to look-up the constant as the receiver type of the macro.