Style::HashTransformValues - incorrect autocorrection when wrapped in a block

[franzliedke]

I caught this incorrect auto-correction when running Rubocop in our project:

# before:
wrapping_block do
  x.map do |k, v|
    [k, v.to_s]
  end.to_h
end

# after:
wrapping_block do
  x.transform_values do |v|
    v.to_s
  end.to_h # <-- This #to_h call should have been removed
end

After some trial and error, I managed to create the failing test case. The failure seems to be related to the wrapping block - when not wrapped, the transformation is applied correctly.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[franzliedke]

I hope it's okay to have only submitted a failing test. I would love to help with the actual implementation, but would need some guidance there.

What I did was generate the ASTs with the help of ruby-parse... I assume this will help us apply the transformation correctly.

Without block:

(send
  (block
    (send
      (send nil :x) :map)
    (args
      (arg :k)
      (arg :v))
    (array
      (lvar :k)
      (send
        (lvar :v) :to_s))) :to_h)

With block:

(block
  (send nil :wrapping)
  (args)
  (send
    (block
      (send
        (send nil :x) :map)
      (args
        (arg :k)
        (arg :v))
      (array
        (lvar :k)
        (send
          (lvar :v) :to_s))) :to_h))

[dvandersluis]

@franzliedke are you intending on fixing this or is it meant more as a proof of concept?

[franzliedke]

I would love to fix it, but would need some pointers (to documentation or specific lines of code - I can dig further myself, but am unsure how to tackle this). 🙏🏼

[dvandersluis]

Yeah unfortunately HashTransformValues is a bit arcane. It defines pattern matchers, but the bulk of the work is done in RuboCop::Cop::HashTransformMethod.

rubocop/lib/rubocop/cop/mixin/hash_transform_method.rb

Lines 17 to 36 in 4874caa

	def on_block(node)
	on_bad_each_with_object(node) do |*match|
	handle_possible_offense(node, match, 'each_with_object')
	end
	return if target_ruby_version < 2.6
	on_bad_to_h(node) { |*match| handle_possible_offense(node, match, 'to_h {...}') }
	end
	def on_send(node)
	on_bad_hash_brackets_map(node) do |*match|
	handle_possible_offense(node, match, 'Hash[_.map {...}]')
	end
	on_bad_map_to_h(node) { |*match| handle_possible_offense(node, match, 'map {...}.to_h') }
	end
	def on_csend(node)
	on_bad_map_to_h(node) { |*match| handle_possible_offense(node, match, 'map {...}.to_h') }
	end

It looks like the matchers are working properly (as they should; the block form has the same AST within it as the non-block form), but the autocorrection is not completing properly (possibly because the right matcher isn't being used or it's not capturing the right thing?).

With the wrapping block, prepare_correction is returning

=> #<struct RuboCop::Cop::HashTransformMethod::Autocorrection
 match=[:v, s(:lvar, :k), s(:send,
  s(:lvar, :v), :to_s)],
 block_node=s(:block,
  s(:send,
    s(:send, nil, :x), :map),
  s(:args,
    s(:arg, :k),
    s(:arg, :v)),
  s(:array,
    s(:lvar, :k),
    s(:send,
      s(:lvar, :v), :to_s))),
 leading=0,
 trailing=0>

but without it it's returning

=> #<struct RuboCop::Cop::HashTransformMethod::Autocorrection
 match=[:v, s(:lvar, :k), s(:send,
  s(:lvar, :v), :to_s)],
 block_node=s(:block,
  s(:send,
    s(:send, nil, :x), :map),
  s(:args,
    s(:arg, :k),
    s(:arg, :v)),
  s(:array,
    s(:lvar, :k),
    s(:send,
      s(:lvar, :v), :to_s))),
 leading=0,
 trailing=5>

(trailing=5 vs trailing=0) -- that's where I'd start.

[franzliedke]

@dvandersluis Thanks so much for these first pointers!

Did some step-debugging myself and found these places as well. Here's what I understood:

• There exists a condition that seems to check for a wrapping (?) block - when I comment this out, my test is green. However, this test starts failing.
• Judging by the test case, this condition wants to check for a block being passed to the node we matched
• However, in my scenario (when we're wrapped within a block), the parent node is also a Rubocop::AST::BlockNode - the difference is that our matching node is the body of the block, not the "send node" (my terminology may be off).

Therefore, I extended the condition a bit and this now has all the tests passing. 😊

What do you think? Is this the right way to fix this problem?

[franzliedke]

I added a commit message that may or may not explain what I understood to be the fix. 😬

Now I only need to add a changelog file, I suppose?

[dvandersluis]

I'll take a look at your code in a little bit, thanks for working on this!

You can run bundle exec rake changelog:fix and it'll create a changelog entry for you out of your commit message.

[dvandersluis]

Changes look pretty good. Seems like a sensible fix 👍

Given that that module affects multiple cops can you please make sure to add tests for the other cops as well?

[franzliedke]

Voila. Rebased on latest master, added the changelog file and tests for HashTransformKeys as well (and confirmed it would indeed fail without the fix).

Thanks for your help - this was a great experience! 🙌🏼

[dvandersluis]

Thanks for fixing this! ❤️