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

Add macro method ASTNode#nil? #10850

Merged

Conversation

straight-shoota
Copy link
Member

#10616 removed a bug that the compiler accepted calls to nil? in macro expressions where that method does not exist. These calls were internally transformed to is_a?(Nil). In the macro interpreter, no value can be of type Nil because that's a runtime type, so any nil? call would always return false.
Removing that transformation from the parser made nil? calls in macro expressions errors because that method is not defined.

But users would expect nil.nil? to return true. So this patch implements ASTNode#nil? in the macro interpreter. It returns true if the value is a NilLiteral or Nop.

This is technically a feature addition, but we should still merge it for 1.1 despite the feature freeze. It actually avoids a breaking change that would occur with #10616 alone. Code that worked in 1.0 would no longer compile (even though it was already broken before and just worked by chance). With this PR, it continues to compile and fulfills the intended purpose.

@beta-ziliani beta-ziliani added this to the 1.1.0 milestone Jun 24, 2021
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @straight-shoota !

@beta-ziliani
Copy link
Member

I'm merging it now to get it in the next nightly.

@beta-ziliani beta-ziliani merged commit 278c15b into crystal-lang:master Jun 25, 2021
@straight-shoota straight-shoota deleted the feature/macro-nil-method branch June 25, 2021 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants