Make `it = it` assign `nil` to match parse.y behavior [Bug #21139] by S-H-GAMELINKS · Pull Request #3604 · ruby/prism

S-H-GAMELINKS · 2025-07-21T02:02:27Z

Currently Prism returns 42 for code like this:

42.tap { it = it; p it } # => 42

But parse.y returns nil:

42.tap { it = it; p it } # => nil

In parse.y, it on the right-hand side is parsed as a local variable. In Prism, it was parsed as the implicit block parameter it, which caused this inconsistent behavior.

This change makes the right-hand side it to be parsed as a local variable, aligning with parse.y's behavior.

Bug ticket: https://bugs.ruby-lang.org/issues/21139

Currently Prism returns `42` for code like this: ```ruby 42.tap { it = it; p it } # => 42 ``` But parse.y returns `nil`: ```ruby 42.tap { it = it; p it } # => nil ``` In parse.y, it on the right-hand side is parsed as a local variable. In Prism, it was parsed as the implicit block parameter it, which caused this inconsistent behavior. This change makes the right-hand side it to be parsed as a local variable, aligning with parse.y's behavior. Bug ticket: https://bugs.ruby-lang.org/issues/21139

Earlopain · 2025-07-21T06:42:53Z

      assert_equal(it_block_parameter_sexp, actual_ast.to_sexp)
    end

+    def test_it_assignment_syntax


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.

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

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.

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

Earlopain · 2025-07-21T06:43:47Z

@@ -0,0 +1 @@
+42.tap { it = it; p it }


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

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

vinistock

While I understand the discrepancy, I wonder if parse.y's behaviour could be counter intuitive for developers. In this example:

42.tap { it = it; p it }

The proposed parse.y behaviour would treat it = it as if it were it = nil (reading from a non-intiailized local variable, to a that same local variable). But we think it's more intuitive if it prints 42, because it = it is creating a new local variable from the result of reading the it parameter (before it gets "shadowed" by a local variable of the same name), which is 42. If developers want the the nil behaviour, they can explicitly write it = nil instead.

Maybe someone with more experience on the parser can pitch in, but I'd argue that Prism's current behaviour is more intuitive than having it end up as nil.

Reviewed with @amomchilov

vinistock · 2025-07-21T17:52:04Z

+                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;


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.

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.

tenderlove · 2025-07-21T23:02:06Z

Maybe someone with more experience on the parser can pitch in, but I'd argue that Prism's current behaviour is more intuitive than having it end up as nil.

@vinistock do you mind commenting on the original Redmine ticket? It seems like matz wants the behavior specified in this PR.

kddnewton

I would prefer that we handle this in the place where we handle the = operator. That's where we usually handle these kinds of transformations. (The very top of the parse_expression_infix function.) You'll see we have to handle call nodes in a similar way.

…n_infix` function

S-H-GAMELINKS · 2025-07-28T15:01:03Z

Thank you for your comment.
I've moved the logic to convert it to a local variable to the beginning of parse_expression_infix as you suggested.
All tests including memcheck pass locally, but CI shows memcheck failures. I'm investigating what might be different in the CI environment.

Earlopain · 2025-07-28T15:09:38Z

memcheck runs with ruby-dev from 2025-07-25 and also recently started failing on main. I wouldn't worry about it too much unless you want to report it upstream, very unlikely to be related to your PR.

tenderlove

This looks good to me.

Earlopain reviewed Jul 21, 2025

View reviewed changes

vinistock reviewed Jul 21, 2025

View reviewed changes

S-H-GAMELINKS added 3 commits July 23, 2025 21:20

Remove uneeded test

a6b448b

Add it read and assignment test

659d769

Add snapshots

51e2b04

kddnewton requested changes Jul 27, 2025

View reviewed changes

Convert implicit parameter it to local variable in `parse_expressio…

fb136c6

…n_infix` function

tenderlove approved these changes Aug 1, 2025

View reviewed changes

tenderlove merged commit b35e37b into ruby:main Aug 1, 2025
59 of 60 checks passed

S-H-GAMELINKS deleted the fix/bug-21139 branch August 3, 2025 06:14

Uh oh!

Conversation

S-H-GAMELINKS commented Jul 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinistock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenderlove commented Jul 21, 2025

Uh oh!

kddnewton left a comment

Choose a reason for hiding this comment

Uh oh!

S-H-GAMELINKS commented Jul 28, 2025

Uh oh!

Earlopain commented Jul 28, 2025

Uh oh!

tenderlove left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vinistock left a comment •

edited

Loading