Skip to content

Make it = it assign nil to match parse.y behavior [Bug #21139] #3604

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.

Sign up for GitHub

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

Merged
tenderlove merged 5 commits into from
Aug 1, 2025
Merged

Make it = it assign nil to match parse.y behavior [Bug #21139] #3604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf3bbf9
Make `it = it` assign `nil` to match parse.y behavior [Bug #21139]
S-H-GAMELINKS Jul 21, 2025
a6b448b
Remove uneeded test
S-H-GAMELINKS Jul 23, 2025
659d769
Add it read and assignment test
S-H-GAMELINKS Jul 23, 2025
51e2b04
Add snapshots
S-H-GAMELINKS Jul 23, 2025
fb136c6
Convert implicit parameter `it` to local variable in `parse_expressio…
S-H-GAMELINKS Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/prism.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16456,6 +16456,13 @@ parse_variable(pm_parser_t *parser) {

return node;
} else if ((parser->version != PM_OPTIONS_VERSION_CRUBY_3_3) && pm_token_is_it(parser->previous.start, parser->previous.end)) {
if (match1(parser, PM_TOKEN_EQUAL)) {
pm_constant_id_t name_id = pm_parser_local_add_location(parser, parser->previous.start, parser->previous.end, 0);
pm_node_t *node = (pm_node_t *) pm_local_variable_read_node_create_constant_id(parser, &parser->previous, name_id, 0, false);

return node;

@vinistock vinistock Jul 21, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we not need to append to the list of implicit parameters still? For example, in this scenario:

42.tap do
  p it
  it = it
  p it
end

The block still needs to have an implicit it parameter as the first print is reading from it. And this matches parse.y.

@S-H-GAMELINKS S-H-GAMELINKS Jul 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback.

I applied this change to Ruby and tested the behavior you mentioned, and the result is:

42.tap do
  p it # => 42
  it = it
  p it # => nil
end

The implicit parameter works correctly for the first it read, and the assignment properly creates a local variable it. So, no additional changes are needed for the implicit parameters list.

}

pm_node_t *node = (pm_node_t *) pm_it_local_variable_read_node_create(parser, &parser->previous);
pm_node_list_append(&current_scope->implicit_parameters, node);

Expand Down
1 change: 1 addition & 0 deletions test/prism/fixtures/it_assignment.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
42.tap { it = it; p it }

@Earlopain Earlopain Jul 21, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe also test 42.tap { it; it = it; p it }? This should still generate a it read for the first one.

@S-H-GAMELINKS S-H-GAMELINKS Jul 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment.

You're right, that test case would be good to have as well.
I've added the test case in the following commit: 659d769

19 changes: 19 additions & 0 deletions test/prism/ruby/parser_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,25 @@ def test_it_block_parameter_syntax
assert_equal(it_block_parameter_sexp, actual_ast.to_sexp)
end

def test_it_assignment_syntax

@Earlopain Earlopain Jul 21, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is for the parser gem translation and is not necessary to add as a test. The fixture you added should generate a snapshot instead that you should commit to test the prism behaviour.

@S-H-GAMELINKS S-H-GAMELINKS Jul 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out.
It appears I misunderstood and added the test incorrectly. I've removed this test case.

@Earlopain Earlopain Jul 23, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If I run bundle exec rake on your branch, I am getting two files that need to be commited:

Started
|Created snapshot at /home/user/code/ruby-prism/snapshots/it_assignment.txt.
-Created snapshot at /home/user/code/ruby-prism/snapshots/it_read_and_assignment.txt

Those basically contain the test I commented on here, just with the native prism ast format. This is automatically tested against and when the output changes, a test would fail.

@S-H-GAMELINKS S-H-GAMELINKS Jul 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment.
I forgot to add the snapshot, and I've pushed it now.

it_assignment_fixture_path = Pathname(__dir__).join('../../../test/prism/fixtures/it_assignment.txt')

buffer = Parser::Source::Buffer.new(it_assignment_fixture_path)
buffer.source = it_assignment_fixture_path.read
actual_ast = Prism::Translation::Parser34.new.tokenize(buffer)[0]

it_assignment_sexp = parse_sexp {
s(:block,
s(:send, s(:int, 42), :tap),
s(:args),
s(:begin,
s(:lvasgn, :it, s(:lvar, :it)),
s(:send, nil, :p, s(:lvar, :it))))
}

assert_equal(it_assignment_sexp, actual_ast.to_sexp)
end

private

def assert_equal_parses(fixture, compare_asts: true, compare_tokens: true, compare_comments: true)
Expand Down
Loading