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

Using captures in ? with OR branches has an effect on captures outside the optional matching #323

Open
Earlopain opened this issue Oct 24, 2024 · 1 comment

Comments

@Earlopain
Copy link
Contributor

Earlopain commented Oct 24, 2024

It's a bit much, so I'll just start with the testcase:

require 'bundler/inline'

# Using captures in `?` with OR branches has an effect on captures outside the optional matching
gemfile do
  source 'https://rubygems.org'
  gem 'rubocop-ast', '1.32.3'
  gem 'minitest'
end

require 'minitest/autorun'

class Matcher
  class << self
    extend RuboCop::AST::NodePattern::Macros

    def_node_matcher :one_branch, <<~PATTERN
      (
        send _ $_method_name
        (
          sym
          {$_}
        )?
      )
    PATTERN

    def_node_matcher :two_branch, <<~PATTERN
      (
        send _ $_method_name
        (
          sym
          {$_ | $_}
        )?
      )
    PATTERN

    def_node_matcher :three_branch, <<~PATTERN
      (
        send _ $_method_name
        (
          sym
          {$_ | $_ | $_}
        )?
      )
    PATTERN
  end
end

class Test < Minitest::Test
  def assert_match(code, matcher, expected)
    ast = RuboCop::AST::ProcessedSource.new(code, 3.3).ast
    actual = Matcher.send(matcher, ast)
    assert_equal(expected, actual)
  end

  def test_one
    assert_match("foo", :one_branch, [:foo, []])
  end

  def test_one_optional_matches
    assert_match("foo(:bar)", :one_branch, [:foo, [:bar]])
  end

  def test_two
    assert_match("foo", :two_branch, [:foo, []])
  end

  def test_two_optional_matches
    assert_match("foo(:bar)", :two_branch, [:foo, [:bar]])
  end

  def test_three
    assert_match("foo", :three_branch, [:foo, []])
  end

  def test_three_optional_matches
    assert_match("foo(:bar)", :three_branch, [:foo, [:bar]])
  end
end

#   1) Failure:
# Test#test_two_optional_matches [test.rb:68]:
# Expected: [:foo, [:bar]]
#   Actual: [[:foo], [:bar]]
# 
#   2) Failure:
# Test#test_two [test.rb:64]:
# Expected: [:foo, []]
#   Actual: [[], []]
# 
#   3) Failure:
# Test#test_three [test.rb:72]:
# Expected: [:foo, []]
#   Actual: [:foo, [], [], []]
# 
# 6 runs, 6 assertions, 3 failures, 0 errors, 0 skips

There are three node patterns, all very similar except that an OR group has different amount of branches. I expect them to all return the same thing but what actually happens is that depending on the amount of branches the output changes:

  • One branch: As expected (but also not very useful)
  • Two branches: The first capture is wrapped in an array and is empty when the optional part matches
  • Three branches: When the optional doesn't match it has 4 captures but the first is not wrapped, and when the optional does match it only return 2 captures (with the expected shape even)

There seems to be some state leak. While I'm getting better at writing node patterns, understanding what is going on here is far beyond me. I think this pattern should be valid but maybe I'm misunderstanding something?

@marcandre
Copy link
Contributor

Thanks for opening the issue.

Yes, I agree that these patterns should all be valid and return the exact same results for any input.

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

No branches or pull requests

2 participants