Skip to content

Commit

Permalink
Fix a false positive for Style/SafeNavigation
Browse files Browse the repository at this point in the history
This PR fixes the following error for `Style/SafeNavigation`
when checking `foo&.empty?` in a conditional.

```console
% cat example.rb
do_something if ENV['VERSION'] && ENV['VERSION'].empty?

% bunble exec rubocop -a example.rb --only Style/SafeNavigation,Lint/SafeNavigationWithEmpty
(snip)

Inspecting 1 file
W

Offenses:

example.rb:1:17: W: [Corrected] Lint/SafeNavigationWithEmpty: Avoid
calling empty? with the safe navigation operator in conditionals.
do_something if ENV['VERSION']&.empty?
                ^^^^^^^^^^^^^^^^^^^^^^
example.rb:1:17: C: [Corrected] Style/SafeNavigation: Use safe
navigation (&.) instead of checking if an object exists before calling
the method.
do_something if ENV['VERSION'] && ENV['VERSION'].empty?
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 files inspected, 2 offenses detected, 2 offenses corrected
Infinite loop detected in
/Users/koic/src/github.com/koic/rubocop-issues/rails/example.rb.
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/runner.rb:289:in
`check_for_infinite_loop'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/runner.rb:272:in
`block in iterate_until_no_changes'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/runner.rb:271:in
`loop'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/runner.rb:271:in
`iterate_until_no_changes'
/Users/koic/src/github.com/rubocop-hq/rubocop/lib/rubocop/runner.rb:242:in
`do_inspection_loop'
```

I found it with the following code.
https://github.com/rails/rails/blob/v6.0.3.2/activerecord/lib/active_record/railties/databases.rake#L120
  • Loading branch information
koic committed Sep 6, 2020
1 parent dfdf8f9 commit 35ebbf9
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -12,6 +12,7 @@
* [#8572](https://github.com/rubocop-hq/rubocop/issues/8572): Fix a false positive for `Style/RedundantParentheses` when parentheses are used like method argument parentheses. ([@koic][])
* [#8653](https://github.com/rubocop-hq/rubocop/pull/8653): Fix a false positive for `Layout/DefEndAlignment` when using refinements and `private def`. ([@koic][])
* [#8655](https://github.com/rubocop-hq/rubocop/pull/8655): Fix a false positive for `Style/ClassAndModuleChildren` when using cbase class name. ([@koic][])
* [#8654](https://github.com/rubocop-hq/rubocop/pull/8654): Fix a false positive for `Style/SafeNavigation` when checking `foo&.empty?` in a conditional. ([@koic][])

### Changes

Expand Down
4 changes: 4 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -8653,6 +8653,10 @@ foo && foo.nil? # method that `nil` responds to
foo && foo < bar
foo < bar if foo
# When checking `foo&.empty?` in a conditional, `foo` being `nil` will actually
# do the opposite of what the author intends.
foo && foo.empty?
# This could start returning `nil` as well as the return of the method
foo.nil? || foo.bar
!foo || foo.bar
Expand Down
5 changes: 5 additions & 0 deletions lib/rubocop/cop/style/safe_navigation.rb
Expand Up @@ -49,6 +49,10 @@ module Style
# foo && foo < bar
# foo < bar if foo
#
# # When checking `foo&.empty?` in a conditional, `foo` being `nil` will actually
# # do the opposite of what the author intends.
# foo && foo.empty?
#
# # This could start returning `nil` as well as the return of the method
# foo.nil? || foo.bar
# !foo || foo.bar
Expand Down Expand Up @@ -104,6 +108,7 @@ def check_node(node)
# chain greater than 2
return if chain_size(method_chain, method) > 1
return if unsafe_method_used?(method_chain, method)
return if method_chain.method?(:empty?)

add_offense(node) do |corrector|
autocorrect(corrector, node)
Expand Down
18 changes: 18 additions & 0 deletions spec/rubocop/cli/cli_autocorrect_spec.rb
Expand Up @@ -1581,6 +1581,24 @@ def self.some_method(foo, bar: 1)
RUBY
end

it 'does not crash Lint/SafeNavigationWithEmpty and offenses and accepts Style/SafeNavigation ' \
'when checking `foo&.empty?` in a conditional' do
create_file('example.rb', <<~RUBY)
do_something if ENV['VERSION'] && ENV['VERSION'].empty?
RUBY
expect(
cli.run(
[
'--auto-correct',
'--only', 'Lint/SafeNavigationWithEmpty,Style/SafeNavigation'
]
)
).to eq(0)
expect(IO.read('example.rb')).to eq(<<~RUBY)
do_something if ENV['VERSION'] && ENV['VERSION'].empty?
RUBY
end

it 'corrects TrailingCommaIn(Array|Hash)Literal and ' \
'Multiline(Array|Hash)BraceLayout offenses' do
create_file('.rubocop.yml', <<~YAML)
Expand Down
4 changes: 4 additions & 0 deletions spec/rubocop/cop/style/safe_navigation_spec.rb
Expand Up @@ -90,6 +90,10 @@
expect_no_offenses('foo && foo.bar !~ /baz/')
end

it 'allows an object check before a method call that is used with `empty?`' do
expect_no_offenses('foo && foo.empty?')
end

it 'allows an object check before a method call that is used in ' \
'a spaceship comparison' do
expect_no_offenses('foo && foo.bar <=> baz')
Expand Down

0 comments on commit 35ebbf9

Please sign in to comment.