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

Fix lex_state_beg_p #1591

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

haldun
Copy link
Collaborator

@haldun haldun commented Sep 22, 2023

Closes #1587
Closes #1572

@haldun haldun force-pushed the haldun/fix-kwdargs-with-plus-minus branch from 6c46750 to 95913a4 Compare September 28, 2023 09:51
@haldun
Copy link
Collaborator Author

haldun commented Oct 2, 2023

@kddnewton wdyt about this one?

src/prism.c Outdated
@@ -6912,7 +6912,7 @@ parser_lex(pm_parser_t *parser) {
);
}

if (lex_state_beg_p(parser) || spcarg) {
if (lex_state_beg_p(parser) || spcarg || parser->in_keyword_arg) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm nervous about this change because it doesn't match up with parse.y, which makes me think we're missing some state change/detail. This line is equivalent to https://github.com/ruby/ruby/blob/486b674e2a8437bacb00c48038c04aec420c47a0/parse.y#L10801 which doesn't check if we're inside a keyword argument.

I did a little debugging and it looks like we're transitioning BEG -> CMDARG and they're transitioning BEG -> ARG. Maybe we're missing something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are transitioning to CMDARG here:

prism/src/prism.c

Lines 8840 to 8848 in 2dc5320

}
pm_lex_state_t last_state = parser->lex_state;
if (type == PM_TOKEN_IDENTIFIER || type == PM_TOKEN_CONSTANT || type == PM_TOKEN_METHOD_NAME) {
if (lex_state_p(parser, PM_LEX_STATE_BEG_ANY | PM_LEX_STATE_ARG_ANY | PM_LEX_STATE_DOT)) {
if (previous_command_start) {
lex_state_set(parser, PM_LEX_STATE_CMDARG);
} else {

because previous_command_start is true. An it is set to be true in

prism/src/prism.c

Lines 14345 to 14350 in 2dc5320

} else {
equal = not_provided(parser);
if (lparen.type == PM_TOKEN_NOT_PROVIDED) {
lex_state_set(parser, PM_LEX_STATE_BEG);
parser->command_start = true;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wonder if that's not correct or if we're missing something here. Because we should definitely be transitioning to ARG and not CMDARG.

Copy link
Collaborator Author

@haldun haldun Nov 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed quite unlikely but can lex_state_beg_p be just wrong? I attempted to fix it here and it seems like this bug gets resolved e854d47 Comparing with the Ruby implementation

https://github.com/ruby/ruby/blob/486b674e2a8437bacb00c48038c04aec420c47a0/parse.y#L8517C1-L8517C89

where IS_lex_state_all_for is defined as

https://github.com/ruby/ruby/blob/486b674e2a8437bacb00c48038c04aec420c47a0/parse.y#L347

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also seems to fix #1572

@haldun haldun force-pushed the haldun/fix-kwdargs-with-plus-minus branch from 95913a4 to e854d47 Compare November 12, 2023 12:52
@haldun haldun force-pushed the haldun/fix-kwdargs-with-plus-minus branch from e854d47 to 399c84b Compare November 12, 2023 13:49
@haldun haldun changed the title Fix parsing keyword args starting with plus and minus Fix lex_state_beg_p Nov 13, 2023
@haldun
Copy link
Collaborator Author

haldun commented Nov 19, 2023

@kddnewton Can you take another look into this?

Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

@kddnewton kddnewton merged commit 46b8576 into ruby:main Nov 21, 2023
46 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruby accepts def f x:-a; end but YARP rejects it Ruby accepts def foo x:%(xx); end but YARP rejects it
2 participants