Skip to content

Conversation

@koic
Copy link
Contributor

@koic koic commented Feb 26, 2025

Summary

itblock node is added to support the it block parameter syntax introduced in Ruby 3.4.

$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
                                                      p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:itblock,
  s(:send, nil, :proc), :it,
  s(:lvar, :it))

This node design is similar to the numblock node, which was introduced for the numbered parameter syntax in Ruby 2.7.

$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { _1 }"; \
                                                      p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:numblock,
  s(:send, nil, :proc), 1,
  s(:lvar, :_1))

The difference is that while numbered parameters can have multiple parameters, the it block parameter syntax allows only a single parameter.

In Ruby 3.3, the conventional node prior to the it block parameter syntax is returned.

$ ruby -Ilib -rprism -rprism/translation/parser33 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
                                                      p Prism::Translation::Parser33.new.tokenize(buffer)[0]'
s(:block,
  s(:send, nil, :proc),
  s(:args),
  s(:send, nil, :it))

Development Note

The Parser gem does not yet support the it block parameter syntax. This is the first case where Prism's node design precedes that of the Parser gem. When implementing whitequark/parser#962, this node design will need to be taken into consideration.

cc @whitequark @iliabylich

@koic koic force-pushed the support_itblock_for_translation_parser branch 2 times, most recently from ff8045e to 7e0e098 Compare February 26, 2025 05:01
@@ -0,0 +1,7 @@
-> do it + 42 end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is actually not really tested, only Parser33 is run in the test suite. I thought about it a bit before, and probably a separate directory for new syntax would make sense (or something along those lines).

You probably need to assert specifically against the ast anyways, as it only compares to the parser gem, and there is nothing to compare against in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I've added a test to check the :itblock node type, which is currently provided only by Prism::Translation::Parser. Since it is essentially an extension of the Parser gem, I've added it to parser_test.rb.

Additionally, I've implemented a mechanism to compare node compatibility with the Parser::Ruby34, but since the Parser gem doesn't yet support the it block parameter, it is currently skipped.

@koic koic force-pushed the support_itblock_for_translation_parser branch 5 times, most recently from 15475ac to 722a9c7 Compare February 26, 2025 09:59
}
end

def assert_prism_only_node(actual_ast, fixture_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's doubtful if parser will ever support 3.4 and this seems highly specific. It also only works because this is valid syntax to the parser gem, if some new syntax gets added you can't test this via this mechanism.

I think I would just add new specific tests for this syntax. If the parser gem does end up supporting it, then these test cases can just be imported. Something like this:

# Pseudo-code like
def test_it_syntax
  parser = Prism:Parser::Translator::Ruby34.new
  expected = s(:itblock, s(:begin, s(:itarg)))
  assert_equal(expected, parser.tokenize(it_fixture)[0].to_sexp)
end

Then, you can extend the existing it.txt (stuff in fixtures/whitequark is pulled from upstream) and explicitly assert against it. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is currently unclear how the Parser gem will support Ruby 3.4 and later, but I agree with the idea of focusing on the necessary test for now. And using S-expressions in Prism::Translation::Parser to test preceding syntax is a good idea, as it clarifies the expected syntax tree. I have updated the test accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good to me, thanks!

It might be a bit annoying to have to update the expectation when the fixture changes, inlining the code so the test is entirely self-contained might be worth a thought. But I think for now this is fine.

## Summary

`itblock` node is added to support the `it` block parameter syntax introduced in Ruby 3.4.

```console
$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
                                                      p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:itblock,
  s(:send, nil, :proc), :it,
  s(:lvar, :it))
```

This node design is similar to the `numblock` node, which was introduced for the numbered parameter syntax in Ruby 2.7.

```
$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { _1 }"; \
                                                      p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:numblock,
  s(:send, nil, :proc), 1,
  s(:lvar, :_1))
```

The difference is that while numbered parameters can have multiple parameters, the `it` block parameter syntax allows only a single parameter.

In Ruby 3.3, the conventional node prior to the `it` block parameter syntax is returned.

```console
$ ruby -Ilib -rprism -rprism/translation/parser33 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
                                                      p Prism::Translation::Parser33.new.tokenize(buffer)[0]'
s(:block,
  s(:send, nil, :proc),
  s(:args),
  s(:send, nil, :it))
```

## Development Note

The Parser gem does not yet support the `it` block parameter syntax. This is the first case where Prism's node design precedes that of the Parser gem.
When implementing whitequark/parser#962, this node design will need to be taken into consideration.
@kddnewton kddnewton merged commit ffa6302 into ruby:main Mar 10, 2025
59 checks passed
@koic koic deleted the support_itblock_for_translation_parser branch March 10, 2025 20:49
koic added a commit to koic/rubocop-ast that referenced this pull request Mar 21, 2025
## Summary

`Prism::Translation::Parser35` has been started development for Ruby 3.5 (edge Ruby):
ruby/prism#3346

At present, there is no timeline for Ruby 3.5 support in the Parser gem:
whitequark/parser#1046

Given this, support for Ruby 3.5 will first be introduced in `Prism::Translation::Parser`.
This can also serve as a trigger to facilitate user migration from `ParserEngine: parser_whitequark` to
`ParserEngine: parser_prism`.

As for Ruby 3.4, `Prism::Translation::Parser` is working on supporting the `it` syntax in
ruby/prism#3481, but the Parser gem has not yet supported for it.
However, whether to deprecate Ruby 3.4 in the Parser gem will be considered separately from this PR.

Since Ruby 3.5 is a development version with minimal impact on users, while Ruby 3.4 is a stable release,
they will be evaluated separately.

## Additional Information

First, the default `parser_engine` for Ruby 3.5 is set to `parser_prism`.
This default aligns with the current state of support, as the Parser gem is not expected to support Ruby 3.5.

The more challenging decision is how to handle the default for Ruby 3.4.
Ruby 3.4 will likely serve as a transition period between the Parser gem and Prism.
While it is possible to set the default `parser_engine` to `parser_prism` for Ruby 3.4 as well,
considering the potential unexpected incompatibilities in `Prism::Translation::Parser`,
the default remains `parser_whitequark`.

In any case, the primary use case is RuboCop, and what matters most is which value RuboCop chooses to pass.
In other words, the key decision lies with RuboCop.
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.

3 participants