-
-
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
Prevent None and All members in flags enums #4395
Conversation
Just saw these specs. As per #1251 I don't see a usage for redefining those so i'll remove the specs. |
7aaf518
to
b5b3292
Compare
Happy compiler hacking then! I think we need to allow redefining All and None for enums inside libs. We can't assume the None/All semantic will hold outside crystal. In the current state the user is able to override All and None members. It could lead to some inconsistencies where So.
|
add508c
to
96c0bc6
Compare
These two fixup commits should fix your feedback. |
@@ -550,6 +550,10 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor | |||
node.raise "can't reopen enum and add more constants to it" | |||
end | |||
|
|||
if is_flags && !@in_lib && {"None", "All"}.includes?(member.name) |
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.
Shouldn't that be {"none", "all"}.includes?(member.name.downcase)
?
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.
enum Foo
Foo
FOo
FOO
end
Foo.values # => [Foo, FOo, FOO]
This PR seems to be fixed on macos now. |
@RX14, I think I rushed a bit to merge this, and I should have requested you to review the related documentation to make sure it at least is consistent with these changes. Would mind doing that and sending another PR along? |
@mverzilli Not a problem, is it just the language reference that needs updating? I can't think of anywhere else where this would be documented. |
Actually nevermind. It is a subtle detail and in general we try to keep the language reference simple and straight to the point. Thanks! |
@mverzilli I've looked through the language reference section and I agree with you, it's the wrong place for this detail. |
Hi, I just noticed this and realized that my project has been "broken" by this. It's no big deal, I can just remove my definition of None, but I am losing a documented enum member with important information. |
This is my first "real" compiler contribution, please tell me if there's anything I could improve.
See #1251.