Skip to content
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 constants in macro expressions to be referenced from the top level #14860

Closed
wants to merge 1 commit into from

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Aug 1, 2024

Referencing Crystal in a macro conflicts with user-defined Crystal.

module Jewel
  class Crystal
    delegate to_s, to: self.class
  end
end

puts Jewel::Crystal.new
$ crystal --version
Crystal 1.13.1 [405f313e0] (2024-07-12)

$ crystal jewel.cr
In crystal/src/object.cr:1296:28

 1296 | {% if compare_versions(Crystal::VERSION, "1.12.0-dev") >= 0 %}
                               ^---------------
Error: undefined constant Crystal::VERSION

It works correctly with earlier versions of Crystal or this PR.

$ crystal jewel.cr
Jewel::Crystal

This fix was created mechanically by

sed -i -e 's/compare_versions(Crystal/compare_versions(::Crystal/g' **/*.cr(^/)

Best regards,

@straight-shoota
Copy link
Member

This seems unnecessary for top-level code which always evaluates in the scope where its defined. It might still be good to use the same style everywhere... However, there are other top-level constants in macro expressions. So we should look at those as well.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Aug 13, 2024

Thanks for finding this and for the fix!

Only the macro delegated fix is necessary because the macro is injecting code into other places. The other fixes don't inject code and there won't be conflicts.

I'd prefer to avoid prefixing :: everywhere for no reason.

I'd rather have an analysis of other macros that inject code (getter, setter, etc) at uncontrolled places and fix potentially clashing namespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants