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

Remove parser transform for nil? in macro expressions #10616

Conversation

straight-shoota
Copy link
Member

Resolves #10534

@asterite
Copy link
Member

asterite commented Apr 9, 2021

What if we handle that nil? call somehow and give a compile error saying "Calling this method will always return false".

@straight-shoota
Copy link
Member Author

"Calling this method will always return false" implies this method actually exist. But in the macro language, it doesn't need to exist at all.
Now the error message just says undefined macro method 'Type#nil?'. Sure that could be enhanced with something like a recommendation to use == nil instead. But IMO that's not that important. The important part is to fix the bug that nil? doesn't error in the macro language but doesn't work either.

@asterite
Copy link
Member

asterite commented Apr 9, 2021

What will be the value of this?

{% puts nil.nil? %}

@straight-shoota
Copy link
Member Author

It doesn't compile. undefined macro method 'NilLiteral#nil?'

@asterite
Copy link
Member

asterite commented Apr 9, 2021

Now the error message just says undefined macro method 'Type#nil?'

Oh, I thought this "Now" meant "now before this PR", but it seems to be "in this PR".

I think this is fine then 👍

@asterite
Copy link
Member

asterite commented Apr 9, 2021

Just note that if we ever decide to be able to add custom methods to AST nodes in macros, nil? will be a valid method call. And in that case I guess we'd like that to return... maybe true only for the NilLiteral and Nop nodes? Maybe that should be the current behavior?

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Apr 9, 2021

@asterite I'd suggest not having .nil? return true for Nop nodes. I'd be more in favor of adding a dedicated .nop? method. The main reason being it would be easy to do like ivar.default_value.nil? which would return true for both cases of there being a default with a value of nil and no default at all.

EDIT: Forgot there's actually .has_default_value? so this prob isn't as important as I was thinking.

@asterite
Copy link
Member

asterite commented Apr 9, 2021

Oh, yeah, that makes sense. I guess for now we can go with this, and later on change it if needed, still in a backwards compatible way.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented May 18, 2021

While trivial to fix and the .nil? method didn't actually work; this PR was technically a breaking change. Had some code that was doing like !ann[0].nil? that no longer compiles.

@straight-shoota
Copy link
Member Author

I would consider the fact that this code compiled as a compiler bug. It should've always been an error.

Fixing bugs can obviously break code that relied on the bug behaviour, but that shouldn't prevent fixing bugs, right? 😄

@asterite
Copy link
Member

In my mind calling .nil? at the macro level should check if the node is a NilLiteral or Nop. I think that's more intuitive.

@straight-shoota
Copy link
Member Author

Yeah, maybe we should add that. It's not strictly necessary though. And I'm not sure about combining NilLiteral and Nop. So this should really be a dedicated feature discussion.

@asterite
Copy link
Member

For example, when you do if node in a macro, if the node is NilLiteral or Nop that's considered falsey. So I think nil? would be aligned with that behavior.

beta-ziliani pushed a commit that referenced this pull request Jun 25, 2021
#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.
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.

NilLiteral.nil? return false
3 participants