Handling of special nodes in conditional context #69

whitequark · 2013-06-09T21:15:12Z

It came to my attention that parser currently does not handle special nodes in a conditional context (https://github.com/whitequark/parser/blob/master/doc/AST_FORMAT.md#open-questions):

flip-flops
"magic" matches

This must be handled before 2.0 release.

So the question is:

Should parser, by itself, recognize these nodes? They do correspond to Ruby semantics, even if I don't like it. And parser already has the necessary plumbing to handle conditionals. So, probably yes.
Should parser instead have an AST::Processor which updates these nodes? Cons: it's too easy to mistake semantics of an operation which is magic in reality (when interpreted) with semantics of an operation which is mundane.
Node names? match2, match3, flip2, flip3 is really bad.

Invited for discussion: @yorickpeterse @mbj @judofyr @bbatsov

yorickpeterse · 2013-06-09T21:47:23Z

Flip-flops I don't care about, they were going to be removed anyway from what I remember. Magic matches are something I've never seen before so I can't really comment on those either.

whitequark · 2013-06-09T21:50:43Z

@yorickpeterse Removed? Link?

Magic matches... /(?<foo>.*)/ =~ str will capture the content of the group into local variable foo, which gets injected into the current method. (Also I just understood that this can radically affect the way code is parsed :/)

if /foo/ is a shortcut for if $_ =~ /foo/, I think.

yorickpeterse · 2013-06-09T22:24:57Z

Flip-flops discussion can be found here: http://www.ruby-forum.com/topic/2713360.

whitequark · 2013-06-09T22:48:53Z

Well, they stay until 3.0.

bbatsov · 2013-06-10T05:02:07Z

I'd like Parser to recognize flip-flop nodes - mostly because I'd like it to be easy for tool authors to write checks that advise Ruby hackers against their use. As for the node names - I'd suggest iflipflop/eflipflop or iflip/eflip for the flip-flops.

if /foo/ is indeed the same as if $_ =~ /foo/ - this is extremely popular idiom in Perl. Maybe here the correct course here would be for parser to simply insert $_ in the AST. As for match local variables - it's my understanding that they are more or less lvars, although I'm not quite certain about all the magic that happens behind the scenes. If they have to be treated as different nodes maybe they should be mvar or something similar.

whitequark · 2013-06-10T10:05:36Z

@bbatsov ASTs are called syntactic for a reason. If there is no $_ in the source, why would it appear in the AST?

Oh, match local variables are indeed just local variables. It's just that, as I've already shown you, Ruby source can be parsed very differently depending on whether a particular token is a local variable or not; therefore, a compliant parser is required to discover these variables.

judofyr · 2013-06-10T10:16:04Z

I'm indifferent. I very rarely use flip-flops/magic-matches and would accept both the current behavior or having the parser recognizing them.

bbatsov · 2013-06-10T11:36:42Z

@whitequark Fair point. Furthermore my suggestion would have made it harder to differentiate /dfdfdf/ from $_ =~ /dfdfdf/. I guess this should be a separate node type indeed.

whitequark · 2013-06-16T01:38:57Z

Done with everything except lvar-injecting magic matches. They're so fucked up. For example, there is a ton of preconditions:

The regexp should not be interpolated.
Regexp should be at LHS.
The call should appear as operator... no .=~~, only =~~, despite them being equivalent.

You also need to parse the regexp itself to figure out how to parse the remaining parts of ruby source.

05:30 < whitequark> >> def f; if /(?<foo>bar)/.=~('foobar') && p(foo); foo; end; end; f
05:30 < eval-in> whitequark => undefined local variable or method `foo' for main:Object (NameError) ... (https://eval.in/33654)
05:30 < whitequark> >> def f; if /(?<foo>bar)/ =~('foobar') && p(foo); foo; end; end; f
05:30 < eval-in> whitequark => "bar" ... (https://eval.in/33655)

Yeah a single dot changes everything.

Follow up rubocop/rubocop-ast#277 In Prism (`Prism::Translation::Parser`), `match-with-lvasgn` can be distinctly differentiated. It is unclear whether to conform to the current behavior of the Parser gem, but initially, `def_node_matcher` has been updated to accept the following incompatibilities for `Performance/EndWith`, `Performance/StringInclude, and `Performance/StartWith` cops to ensure it works with Prism 0.24.0 as well. ## 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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ``` This lvar-injecting feature appears to have not been supported by Parser gem for a long time: whitequark/parser#69 (comment) ## Prism Returns an `send` node: ```console $ bundle exec ruby -Ilib -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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -Ilib -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ```

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.

Follow up rubocop/rubocop-ast#277 In Prism (`Prism::Translation::Parser`), `match-with-lvasgn` can be distinctly differentiated. It is unclear whether to conform to the current behavior of the Parser gem, but initially, `def_node_matcher` has been updated to accept the following incompatibilities for `Performance/EndWith`, `Performance/StringInclude, and `Performance/StartWith` cops to ensure it works with Prism 0.24.0 as well. ## 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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ``` This lvar-injecting feature appears to have not been supported by Parser gem for a long time: whitequark/parser#69 (comment) ## Prism Returns an `send` node: ```console $ bundle exec ruby -Ilib -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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -Ilib -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ```

…slation::Parser` 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 ruby/prism@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 ruby/prism@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 ruby/prism@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. ruby/prism@dff4abb170

Follow up rubocop/rubocop-ast#277 In Prism (`Prism::Translation::Parser`), `match-with-lvasgn` can be distinctly differentiated. It is unclear whether to conform to the current behavior of the Parser gem, but initially, `def_node_matcher` has been updated to accept the following incompatibilities for `Performance/EndWith`, `Performance/StringInclude`, and `Performance/StartWith` cops to ensure it works with Prism 0.24.0 as well. ## 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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ``` This lvar-injecting feature appears to have not been supported by Parser gem for a long time: whitequark/parser#69 (comment) ## Prism Returns an `send` node: ```console $ bundle exec ruby -Ilib -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)) ``` Returns an `match_with_lvasgn` node: ```console $ bundle exec ruby -Ilib -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("/(?<foo>)foo/ =~ bar")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] s(:match_with_lvasgn, s(:regexp, s(:str, "(?<foo>)foo"), s(:regopt)), s(:send, nil, :bar)) ```

whitequark added a commit that referenced this issue Jun 16, 2013

- builders/default: fix treatment of masgn in &&/|| (refs #69).

75c512b

whitequark added a commit that referenced this issue Jun 16, 2013

+ builders/default: flip-flops (refs #69).

1cc4966

whitequark added a commit that referenced this issue Jun 16, 2013

Describe flip-flops in AST_FORMAT (refs #69).

bf90166

whitequark added a commit that referenced this issue Jun 16, 2013

+ builders/default: implicit matches (refs #69).

9e9b2e5

whitequark added a commit that referenced this issue Jun 16, 2013

Describe lvar-injecting matches in AST_FORMAT (refs #69).

6b75ff0

whitequark closed this as completed in ee46e7e Jun 16, 2013

whitequark added a commit that referenced this issue Jun 16, 2013

Ruby 1.8 does not actually support lvar-injecting match (refs #69).

5644324

koic mentioned this issue Mar 4, 2024

Support Prism as a Ruby parser rubocop/rubocop-performance#446

Merged

8 tasks

koic mentioned this issue Mar 4, 2024

Fix incompatibility AST for regexp match in Prism::Translation::Parser ruby/prism#2548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling of special nodes in conditional context #69

Handling of special nodes in conditional context #69

whitequark commented Jun 9, 2013

yorickpeterse commented Jun 9, 2013

whitequark commented Jun 9, 2013

yorickpeterse commented Jun 9, 2013

whitequark commented Jun 9, 2013

bbatsov commented Jun 10, 2013

whitequark commented Jun 10, 2013

judofyr commented Jun 10, 2013

bbatsov commented Jun 10, 2013

whitequark commented Jun 16, 2013

Handling of special nodes in conditional context #69

Handling of special nodes in conditional context #69

Comments

whitequark commented Jun 9, 2013

yorickpeterse commented Jun 9, 2013

whitequark commented Jun 9, 2013

yorickpeterse commented Jun 9, 2013

whitequark commented Jun 9, 2013

bbatsov commented Jun 10, 2013

whitequark commented Jun 10, 2013

judofyr commented Jun 10, 2013

bbatsov commented Jun 10, 2013

whitequark commented Jun 16, 2013