Skip to content

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented Sep 30, 2025

This PR is a followup to #3558 and fixes the following cases, which should throw a syntax error but were not:

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.

cc/ @tenderlove @kddnewton

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;
}
Copy link
Member

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?

Copy link
Member

@tompng tompng Oct 1, 2025

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:

Copy link
Member

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

Copy link
Contributor

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:

prism/src/prism.c

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 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.
@eileencodes eileencodes force-pushed the followup-fix-for-3558 branch from efdbb28 to ed96da8 Compare October 21, 2025 18:17
@eileencodes
Copy link
Member Author

I made some changes and don't think this is done yet. I added some new tests so we have more coverage while figuring this out. The =>, . and &. were passing without changes but I suspect the issue is where the error is indicated?

@tompng do you have a script you're running that we can use to figure out which others are still not working correctly? Thanks!

@tompng
Copy link
Member

tompng commented Oct 22, 2025

For this case, I don't have a script. I just listed up by glazing at all symbol keys in my keyboard and tried Prism.parse_success? manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants