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

Style/CollectionCompact: bug in auto correction #12801

Closed
gongfarmer opened this issue Mar 20, 2024 · 0 comments · Fixed by #12803
Closed

Style/CollectionCompact: bug in auto correction #12801

gongfarmer opened this issue Mar 20, 2024 · 0 comments · Fixed by #12803
Labels

Comments

@gongfarmer
Copy link

gongfarmer commented Mar 20, 2024

The cop Style/CollectionCompact has unsafe autocorrect which changes calls to Hash#delete_if to Hash#compact.
This is incorrect because Hash#delete_if modifies the object and Hash#compact does not.
The correct replacement would be Hash#compact! (the ! version).

The same applies to Array#delete_if erroneously being converted to Array#compact.
My reproduction below only shows the Hash case. I have not gone looking for other Array / Hash methods that modify the receiver.

Warning: Array#delete_if returns self after modifying the object. Array#compact! sometimes returns nil:
`
$ ri --no-pager Array#compact!
Array#compact!

(from ruby core)

array.compact! -> self or nil

Removes all nil elements from self.

Returns self if any elements removed, otherwise nil.
So maybe it is not as simple as just changingcompacttocompact!` in the replacement.


Expected behavior

I expect a call to Hash#delete_if to be replaced with Hash#compact! (the ! version)

Actual behavior

Hash#delete_if is replaced with Hash#compact (the non ! version)

Steps to reproduce the problem

Example input:

#!/usr/bin/ruby
h = {
  a: "a",
  b: "b",
  c: nil,
}
h.delete_if { |_, v| v.nil? }
pp h

This prints the following:

$ ruby test.rb
{:a=>"a", :b=>"b"}

After letting rubocop correct the source file, the output has changed:

$ rubocop -c /dev/null test.rb  --only Style/CollectionCompact -A
Inspecting 1 file
C

Offenses:

test.rb:8:3: C: [Corrected] Style/CollectionCompact: Use compact instead of delete_if { |_, v| v.nil? }.
h.delete_if { |_, v| v.nil? }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

$ ruby test.rb
{:a=>"a", :b=>"b", :c=>nil}

RuboCop version

$ rubocop -V
1.62.1 (using Parser 3.3.0.5, rubocop-ast 1.31.2, running on ruby 3.1.2) +server [x86_64-linux-gnu]
@koic koic added the bug label Mar 21, 2024
koic added a commit to koic/rubocop that referenced this issue Mar 21, 2024
…mpact`

Fixes rubocop#12801.

This PR fixes incorrect autocorrect for `Style/CollectionCompact` when using `delete_if`.
koic added a commit to koic/rubocop that referenced this issue Mar 21, 2024
…mpact`

Fixes rubocop#12801.

This PR fixes incorrect autocorrect for `Style/CollectionCompact` when using `delete_if`.
koic added a commit that referenced this issue Mar 21, 2024
…le_collection_compact

[Fix #12801] Fix incorrect autocorrect for `Style/CollectionCompact`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants