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 incompatibility AST for regexp match in Prism::Translation::Parser #2548

Merged

Conversation

koic
Copy link
Contributor

@koic koic commented Mar 4, 2024

This PR fixes the following incompatibility AST for regexp match between Parser gem and Prism:

Parser gem

Returns an match_with_lvasgn node:

$ bundle exec ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:match_with_lvasgn,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)),
  s(:send, nil, :bar))

Prism (Prism::Translation::Parser)

Before

Returns an send node:

$ bundle exec ruby -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:send,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)), :=~,
  s(:send, nil, :bar))

After

Returns an match_with_lvasgn node:

$ bundle exec ruby -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:match_with_lvasgn,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)),
  s(:send, nil, :bar))

Background

Found due to incompatibility with RuboCop's Performance/EndWith, Performance/StringInclude, and Performance/StartWith` cops.

Note

This is the incompatibility when the receiver is a regular expression literal and =~ is used. Based on the node name :match_with_lvasgn, it appears that Prism's AST becomes more accurate in cases like visit_match_write_node only.

However, as shown in the background, the current behavior of Parser gem is not like this. Considering compatibility with the published AST of Parser gem, the AST incompatibility will be addressed.

This lvar-injecting feature appears to have not been supported by Parser gem for a long time: whitequark/parser#69 (comment)

There seems to be no indication that it will be supported.

This PR prioritizes AST compatibility between the Parser gem and Prism. However, it is unclear whether this is the best approach.

This PR fixes the following incompatibility AST for regexp match between Parser gem and Prism:

## Parser gem

Returns an `match_with_lvasgn` node:

```console
$ bundle exec ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:match_with_lvasgn,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)),
  s(:send, nil, :bar))
```

## Prism (`Prism::Translation::Parser`)

### Before

Returns an `send` node:

```console
$ bundle exec ruby -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:send,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)), :=~,
  s(:send, nil, :bar))
```

### After

Returns an `match_with_lvasgn` node:

```console
$ bundle exec ruby -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/foo/ =~ bar")'
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:match_with_lvasgn,
  s(:regexp,
    s(:str, "foo"),
    s(:regopt)),
  s(:send, nil, :bar))
```

## Background

Found due to incompatibility with RuboCop's `Performance/EndWith`, `Performance/StringInclude,
and `Performance/StartWith` cops.

## Note

This is the incompatibility when the receiver is a regular expression literal and `=~` is used.
Based on the node name `:match_with_lvasgn`, it appears that Prism's AST becomes more accurate
in cases like `visit_match_write_node` only.

However, as shown in the background, the current behavior of Parser gem is not like this.
Considering compatibility with the published AST of Parser gem, the AST incompatibility will be addressed.

This lvar-injecting feature appears to have not been supported by Parser gem for a long time:
whitequark/parser#69 (comment)

There seems to be no indication that it will be supported.

This PR prioritizes AST compatibility between the Parser gem and Prism.
However, it is unclear whether this is the best approach.
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.

I think this is the right approach, prioritizing compatibility. Even though prism is more "right" here, the point is to emit the same AST.

@kddnewton kddnewton merged commit 663c8b7 into ruby:main Mar 4, 2024
53 of 54 checks passed
@koic koic deleted the fix_incompatibility_ast_for_match_with_lvasgn branch March 4, 2024 16:28
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.

2 participants