Accept a multiple assignment in a rescue modifier value#4128
Accept a multiple assignment in a rescue modifier value#4128kai-matsudate wants to merge 2 commits into
Conversation
A rescue modifier value is a statement in the grammar, so it may be a
multiple assignment, which parse.y accepts but prism rejected:
a rescue b, c = 1 # parse.y: Syntax OK, prism: unexpected ','
It is parsed below the statement level where multiple assignment
detection starts, and lowering the binding power would absorb statement
modifiers too, so set a flag while reading the value instead.
|
Thanks for the contribution! I took it in a slightly different direction in #4133, but this got it going. |
|
@kddnewton Thanks for your comment. I'm glad this PR helped get #4133 going. After reading #4133, I understood that my approach was too narrow because I focused on the multiple-assignment case, but the larger issue was the stmt/arg distinction in the grammar. I have one small follow up question from the note in this PR: No worries if this is outside the scope of this PR. |
|
You're welcome to file something for it, but I don't think it's a "bug" so much as a design decision. |
2 participants
Fixes Bug #22095.
The value of a rescue modifier is a statement in the grammar and may itself be a multiple assignment (
a rescue b, c = 1). parse.y accepts this, but Prism rejected it. The cause is that multiple-assignment detection is only enabled at the statement level (PM_BINDING_POWER_STATEMENT), while the value of a rescue modifier is parsed with the rescue modifier's right binding power (PM_BINDING_POWER_COMPOSITION).Changes
Lowering the binding power so that the value is parsed as a statement would allow the parser to recognize the multiple assignment, but it would also include statement modifiers with lower precedence than
rescue(PM_BINDING_POWER_MODIFIER) in the value. Thena rescue b if cwould parse asa rescue (b if c)instead of(a rescue b) if c. parse.y keeps these concerns separate: precedence keeps statement modifiers out of the value, while a grammar rule allows a multiple assignment in the value. This PR follows the same split.This adds a flag,
PM_PARSE_ACCEPTS_MULTIPLE_ASSIGNMENT, and a predicate macro,PM_PARSE_MULTIPLE_ASSIGNMENT_P, which is true at the statement level or when the flag is set. The checks that decide whether a multiple assignment may start now use this macro: the prefix cases (identifier / instance, global, class variable / constant / leading splat), the infix cases (./[]/::), and the multi-value/splat checks inparse_assignment_values.The flag is set while reading the value of a rescue modifier. The binding power remains
PM_BINDING_POWER_COMPOSITION, which keeps statement modifiers out of the value; the flag enables multiple-assignment parsing for that value without lowering the binding power.The flag is set in two places:
a rescue b), it is passed unconditionally to theparse_expressionthat reads the value. The flag is set before parsing the value begins, so the rescue value can start with a multiple assignment target.parse_assignment_values(a rescue on the right-hand side of an assignment), it is set only when the binding power is that of a multiple assignment right-hand side (PM_BINDING_POWER_MULTI_ASSIGNMENT + 1).The latter is conditional because parse.y allows the rescue value to be a multiple assignment only on the right-hand side of a multiple assignment (
x, y = a rescue b, c = 1), not of a single assignment (x = a rescue b, c = 1). The grammar rule that allows a multiple assignment in a rescue value (mlhs '=' lex_ctxt mrhs_arg modifier_rescue ... stmt) is only reachable from the multiple-assignment left-hand side (mlhs). An endless method definition (def f = a rescue b, c = 1) reads its value through a separate path inparse_defand does not set the flag, so it is rejected as in parse.y as well.Out of scope
A parenthesized group as the first target (
a rescue (b, c), d = 1) is intentionally left out of this PR. Supporting it requires threading the flag throughparse_parentheses, but doing so changes how the opening parenthesis is classified (tLPAREN/tLPAREN2) in the whitequark/parser-compatible translation (lib/prism/translation/parser). That compatibility needs separate handling, so I plan to open a separate PR for it.Tests
test/prism/fixtures/rescue_modifier.txt.=case and the single-assignment right-hand side case totest/prism/errors/. I confirmed that parse.y also rejects these withruby --parser=parse.y -c(therefute_valid_syntaxintest/prism/errorschecks the parse.y-side rejection only when the default parser is parse.y).Note
The asymmetry where a multiple assignment in a rescue value is not allowed on a single-assignment right-hand side (
x = 1 rescue a, b = 1is rejected, butx, y = 1 rescue a, b = 1is accepted) seems to come from the parse.y grammar itself; I plan to file a separate issue for it.