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/EmptyLiteral seems to want to repalce valid code with invalid code #9316

Closed
agargiulo opened this issue Dec 31, 2020 · 5 comments · Fixed by #9318
Closed

Style/EmptyLiteral seems to want to repalce valid code with invalid code #9316

agargiulo opened this issue Dec 31, 2020 · 5 comments · Fixed by #9318

Comments

@agargiulo
Copy link
Contributor

I have run into an issue with the Style/EmptyLiteral cop. It trys to replace valid Hash.new code with invalid syntax when I run with autocorrect.


Expected behavior

I expected RuboCop to not replace code in such a way that it breaks the rest of the file

Actual behavior

When run without autocorrect, it just flags that I should be using {} instead of Hash.new

[0:25:43] % bundle exec rubocop -c /dev/null --debug empty_literal_test.rb
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning <redacted>/empty_literal_test.rb
Loading cache from /Users/agargiulo/.cache/rubocop_cache/477fdf38a615baaec3d84051ac903ef3d5aac730/93ef223995b1081dff8ec247871eb71b25960a7c/c32fcf3f7f083f5a2e9f122522e371d1c1e3dbe5
C

Offenses:

empty_literal_test.rb:4:23: C: [Correctable] Style/EmptyLiteral: Use hash literal {} instead of Hash.new.
other_valid_hash    = Hash.new { _1[_2] = {} }
                      ^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
Finished in 0.13202199997613207 seconds

Which already feels wrong as that is valid syntax. Maybe it's bad style though so I'm not sure on that front.
But if I run the above with autocorrect enabled, it breaks my code

[0:27:54] % bundle exec rubocop -c /dev/null --debug empty_literal_test.rb -a
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning <redacted>/empty_literal_test.rb
E

Offenses:

empty_literal_test.rb:3:23: C: [Corrected] Style/EmptyLiteral: Use hash literal {} instead of Hash.new.
other_valid_hash    = Hash.new { _1[_2] = {} }
                      ^^^^^^^^
empty_literal_test.rb:3:26: E: Lint/Syntax: unexpected token tLCURLY
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
other_valid_hash    = {} { _1[_2] = {} }
                         ^

1 file inspected, 2 offenses detected, 1 offense corrected
Finished in 0.1898069999879226 seconds

Steps to reproduce the problem

Here are the contents of empty_literal_test.rb before running autocorrect

# frozen_string_literal: true

other_valid_hash    = Hash.new { _1[_2] = {} }
correct_hash_object = Hash.new { |h, k| h[k] = {} }

puts other_valid_hash
puts correct_hash_object

RuboCop version

[0:21:31] % bundle exec rubocop -V
1.7.0 (using Parser 3.0.0.0, rubocop-ast 1.3.0, running on ruby 2.7.2 x86_64-darwin20)
  - rubocop-rake 0.5.1
@tejasbubane
Copy link
Contributor

tejasbubane commented Dec 31, 2020

I am able to reproduce on rubocop 1.7.0 with this minimal code: Hash.new { _1[_2] = {} }.

However strangely, I cannot reproduce it on master and not sure if anything changed after 1.7.0. We have tests to ensure this does not happen: https://github.com/rubocop-hq/rubocop/blob/master/spec/rubocop/cop/style/empty_literal_spec.rb#L103-L105

@agargiulo
Copy link
Contributor Author

That's very strange indeed because the file lib/rubocop/cop/style/empty_literal.rb hasn't changed since 1.7.0 has been released as far as I can tell. But I don't know enough about how the parsing works to say more on what could have changed to affect this. (I just tried running against master and the issue seems fixed there)

EVEN stranger, I tried switching back and forth between v1.7.0 and master in my git clone of rubocop and it seems to now never complain. but in the original directory I spotted this it still complains.

[1:43:59] % bundle exec rubocop -c /dev/null --debug foo.rb -C false
configuration from /dev/null
Default configuration from /Users/agargiulo/gh_repos/rubocop/config/default.yml
Inspecting 1 file
Scanning /Users/agargiulo/gh_repos/rubocop/foo.rb
.

1 file inspected, no offenses detected
Finished in 0.18359899998176843 seconds

I made a new temp directory, copied directly the Gemfile from my project and created a file with just Hash.new { _1[_2] = {} } in it and rubocop doesn't complain. Run the exact same command in my project directory on the exact same file, and it errors out.

[1:59:31] % bundle exec rubocop -c /dev/null --debug foo.rb -C false
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning /private/var/folders/t3/mphyq08965q5blrx_zhfvrs40000gn/T/tmp.SOYrELTR/foo.rb
.

1 file inspected, no offenses detected
Finished in 0.19453999999677762 seconds
[1:59:08] % bundle exec rubocop -c /dev/null --debug foo.rb -C false
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning /Users/agargiulo/projects/<redacted>/foo.rb
C

Offenses:

foo.rb:3:1: C: [Correctable] Style/EmptyLiteral: Use hash literal {} instead of Hash.new.
Hash.new { _1[_2] = {} }
^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
Finished in 0.18898799997987226 seconds

@agargiulo
Copy link
Contributor Author

Okay, so if I create .ruby-version and set it to 2.7.2 in my new temp directory, the issue returns.

[2:08:48] % cat .ruby-version
2.7.2
[2:08:49] % bundle exec rubocop -c /dev/null --debug foo.rb -C false
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning /private/var/folders/t3/mphyq08965q5blrx_zhfvrs40000gn/T/tmp.SOYrELTR/foo.rb
C

Offenses:

foo.rb:3:1: C: [Correctable] Style/EmptyLiteral: Use hash literal {} instead of Hash.new.
Hash.new { _1[_2] = {} }
^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
Finished in 0.12098599999444559 seconds

@agargiulo
Copy link
Contributor Author

Setting the .ruby-version file to 2.6.0 (Even with ruby 2.7.2) results in no issues reported

[2:14:27] % cat .ruby-version
2.6.0

[2:15:06] % bundle exec rubocop -c /dev/null --debug foo.rb -C false
configuration from /dev/null
Default configuration from /Users/agargiulo/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-1.7.0/config/default.yml
Inspecting 1 file
Scanning /private/var/folders/t3/mphyq08965q5blrx_zhfvrs40000gn/T/tmp.oKn5yG7g/foo.rb
.

1 file inspected, no offenses detected
Finished in 0.16874999995343387 seconds

@agargiulo
Copy link
Contributor Author

We have tests to ensure this does not happen: https://github.com/rubocop-hq/rubocop/blob/master/spec/rubocop/cop/style/empty_literal_spec.rb#L103-L105

I've locally added this test below the one you linked and it fails as expected.

context 'Ruby 2.7', :ruby27 do
  it 'does not register an offense for Hash.new { _1[_2] = [] }' do
    expect_no_offenses('test = Hash.new { _1[_2] = [] }')
  end
end
[2:44:39] % bundle exec rspec /Users/agargiulo/gh_repos/rubocop/spec/rubocop/cop/style/empty_literal_spec.rb

Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 27942
.............F................

Failures:

  1) RuboCop::Cop::Style::EmptyLiteral Empty Hash Ruby 2.7 does not register an offense for Hash.new { _1[_2] = [] }
     Failure/Error: expect(actual_annotations.to_s).to eq(source)

       expected: "test = Hash.new { _1[_2] = [] }"
            got: "test = Hash.new { _1[_2] = [] }       ^^^^^^^^ Use hash literal `{}` instead of `Hash.new`.\n"

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -test = Hash.new { _1[_2] = [] }
       +test = Hash.new { _1[_2] = [] }       ^^^^^^^^ Use hash literal `{}` instead of `Hash.new`.

     # ./lib/rubocop/rspec/expect_offense.rb:180:in `expect_no_offenses'
     # ./spec/rubocop/cop/style/empty_literal_spec.rb:109:in `block (4 levels) in <top (required)>'

Finished in 0.17176 seconds (files took 1.62 seconds to load)
30 examples, 1 failure

Failed examples:

rspec ./spec/rubocop/cop/style/empty_literal_spec.rb:108 # RuboCop::Cop::Style::EmptyLiteral Empty Hash Ruby 2.7 does not register an offense for Hash.new { _1[_2] = [] }

Randomized with seed 27942

agargiulo added a commit to agargiulo/rubocop that referenced this issue Jan 2, 2021
…n using a numbered block

`Hash.new { _1[_2] = [] }` is valid code but the cop was not parsing the
numblock correctly when parsing ruby27. Now it treats both the above and
`Hash.new { |h, k| h[k] = [] }`
as valid statements.
@koic koic closed this as completed in #9318 Jan 3, 2021
koic added a commit that referenced this issue Jan 3, 2021
[Fix #9316] Make `Style/EmptyLiteral` not register offense when using a numbered block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants