-
Notifications
You must be signed in to change notification settings - Fork 524
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
ruby27: Reject CSend inside left hand side of mass assign #3261
Conversation
This commit tracks upstream commits ruby/ruby@140b811 and ruby/ruby@39ae88a. Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@@ -10,6 +10,5 @@ def some_method(array) | |||
a, b, *c, d, e = array | |||
a, * = array | |||
a.x, b = array | |||
a&.x, b = array |
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.
Not accepted anymore
@@ -69,20 +69,6 @@ class <emptyTree><<C <root>>> < (::<todo sym>) | |||
b = <assignTemp>$17.[](1) | |||
<assignTemp>$16 | |||
end | |||
begin |
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.
Not accepted anymore
if (masgn) { | ||
error(ruby_parser::dclass::CSendInLHSOfMAsgn, tokLoc(dot)); | ||
} |
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.
I feel like it would have been cleaner if done in parser.yyp
instead of the Builder but it seems that all the other error cases are always handled in the builder.
If done here, we need to know if the assignation is on a multi left hand side, hence the extra parameter.
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.
I kind of like your suggestion of inlining this branch into the two cases where masgn
is true
.
If you'd like to change this, let me know. I'll merge this at the end of the day or whenever you get back to me.
Motivation
Before ruby27, the following snippet will cause
ruby
to crash:After ruby27, ruby will return an error instead:
This commit tracks upstream commits ruby/ruby@140b811 and ruby/ruby@39ae88a.
Test plan
See included automated tests.