-
Notifications
You must be signed in to change notification settings - Fork 171
Follow fix for method call with in
bug
#3666
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
base: main
Are you sure you want to change the base?
Conversation
This PR is a followup to ruby#3558 and fixes the following cases, which should throw a syntax error but were not: ```ruby A.print message: in 'BAR' A.print message: in 'BAR' ``` To fix this we check for methods calls with arguments and no parens that then call `in`. Note I had claude write this because I wasn't sure where the problem was but then edited it to remove extra unnecessary code the robot wrote.
pm_call_node_t *cast = (pm_call_node_t *) node; | ||
if (cast->arguments != NULL && cast->opening_loc.start == NULL && parser->current.type == PM_TOKEN_KEYWORD_IN) { | ||
return node; | ||
} |
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 need @kddnewton's opinion here, but I feel like testing for a specific token isn't the right fix and that we should be using binding power instead (but this is just a gut feeling). Is in
the only keyword that has a problem?
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.
Is
in
the only keyword that has a problem?
Looks like in
, =>
, .
, &.
and all binary operators except +
-
**
&
*
are wrongly accepted after A.print message:
but not after a.print message:
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.
And ternary operator A.print message: ? 1 : 2
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 agree with @tenderlove that this is not the correct fix. The code that is supposed to handle this is here:
Lines 22253 to 22261 in ce4abe1
case PM_CALL_NODE: | |
// If we have a call node, then we need to check if it looks like a | |
// method call without parentheses that contains arguments. If it | |
// does, then it has different rules for parsing infix operators, | |
// namely that it only accepts composition (and/or) and modifiers | |
// (if/unless/etc.). | |
if ((pm_binding_powers[parser->current.type].left > PM_BINDING_POWER_COMPOSITION) && pm_call_node_command_p((pm_call_node_t *) node)) { | |
return node; | |
} |
But parse_expression_prefix
returns PM_CONSTANT_READ_NODE
. So I feel like the fix would be to modify parse_expression_prefix
so that it parses further instead of just one node deep. Or this check is also needed at some other place.
But I did not manage to actually fix it myself last I looked into this.
This PR is a followup to #3558 and fixes the following cases, which should throw a syntax error but were not:
To fix this we check for methods calls with arguments and no parens that then call
in
.Note I had claude write this because I wasn't sure where the problem was but then edited it to remove extra unnecessary code the robot wrote.
cc/ @tenderlove @kddnewton