rubocop/rubocop-ast

Set constants in NodePattern don't work in ruby 2.4

dvandersluis opened this issue · 11 comments

dvandersluis commented

I was trying to DRY up a pattern for a PR in rubocop-performance and the NodePattern docs suggest that you can use %CONST with a Set. However, given that rubocop still supports ruby 2.4 I don't believe this is a good recommendation, because Set#=== does not exist in 2.4, and the pattern therefore fails to match there.

For example: https://app.circleci.com/pipelines/github/rubocop-hq/rubocop-performance/341/workflows/528ddd46-d4f6-4dc5-883c-fc02cc002540/jobs/2155

The pattern:

CANDIDATE_METHODS = Set[:select, :find_all, :filter]

def_node_matcher :detect_candidate?, <<~PATTERN
  {
    (send $(block (send _ %CANDIDATE_METHODS) ...) ${:first :last} $...)
    (send $(block (send _ %CANDIDATE_METHODS) ...) $:[] (int ${0 -1}))
    (send $(send _ %CANDIDATE_METHODS ...) ${:first :last} $...)
    (send $(send _ %CANDIDATE_METHODS ...) $:[] (int ${0 -1}))
  }
PATTERN

I wonder if we can refine sets for 2.4 inside NodePattern to add #=== to it? Alternately, should Sets just not be used in rubocop patterns until 2.4 support is dropped? I was considering changing my constant to Set[...].method(:include?).to_proc but that just feels ugly to me.

marcandre commented

I just released 0.4.0, which does add support for Set#=== in Ruby 2.4 😅

  • ❤️1
dvandersluis commented

🤦

Thanks, that's great! Are you maybe able to bump the version in the different rubocop subgems so we can take advantage of this?

marcandre commented

🤦

I wasn't clear: I just released it... because you opened this issue 😆

Are you maybe able to bump the version in the different rubocop subgems so we can take advantage of this?

No need, if you do bundle update rubocop-ast it should upgrade the version as it is compatible with the subgems

dvandersluis commented

Alright I'll do that in my PR! Thanks!

marcandre commented

FYI, I am working on rewriting the NodePattern compiler, and that rewrite should give you the ability to have better unions and also auto-optimize sets...

dvandersluis commented

Yeah I was reading through your PR for that yesterday, great stuff!

  • ❤️1
marcandre commented

Alright I'll do that in my PR! Thanks!

Oh, right, if you do a PR for a gem, then you should have a separate commit that bumps the requirement for rubocop-ast to >= 0.4.0

dvandersluis commented

Oh, right, if you do a PR for a gem, then you should have a separate commit that bumps the requirement for rubocop-ast to >= 0.4.0

Should that be in the .gemspec? Gemfile.lock is gitignored in rubocop-performance (but FWIW CI is installing 0.4.0 now and passing in ruby 2.4).

marcandre commented

Oh, right, if you do a PR for a gem, then you should have a separate commit that bumps the requirement for rubocop-ast to >= 0.4.0

Should that be in the .gemspec? Gemfile.lock is gitignored in rubocop-performance (but FWIW CI is installing 0.4.0 now and passing in ruby 2.4).

Yes, gemspec. I see that rubocop-performance doesn't list explicitly rubocop-ast yet, but I'd recommend you add the line nevertheless:

s.add_runtime_dependency('rubocop-ast', '>= 0.4.0')
  • 🎉1
dvandersluis commented

Thank you for your help!

marcandre commented

My pleasure, thank you for your contributions