Skip to content

Conversation

@koic
Copy link
Member

@koic koic commented Aug 5, 2025

This PR fixes false positives for Style/SafeNavigation when ternary expression with operator method call with method chain.

Fixes #14420.


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.

@koic koic force-pushed the fix_false_positives_for_style_safe_navigation branch from 4300e07 to da0e3e7 Compare August 5, 2025 03:41

it 'registers an offense for ternary expression with operator method call with method chain' do
expect_offense(<<~RUBY, variable: variable)
it 'does not register an offense for ternary expression with operator method call with method chain' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be move up in the allowed example, earlier in this context.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how is this different from the behavior above in practical terms?

Copy link
Member Author

@koic koic Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a spec that described incorrect behavior. As noted in #14420, the previous behavior of this test was to auto-correct from var.nil? ? nil : var.foo * 42 to var&.foo * 42, which is not safe. To make it safe, the correction would have to be var&.foo&.*(42), but that would reduce readability.

This is a more complex version of examples already documented.

#   foo ? foo[index] : nil    # Ignored `foo&.[](index)` due to unclear readability benefit.                                                                                           
#   foo ? foo[idx] = v : nil  # Ignored `foo&.[]=(idx, v)` due to unclear readability benefit.                                                                                         
#   foo ? foo * 42 : nil      # Ignored `foo&.*(42)` due to unclear readability benefit.  

https://docs.rubocop.org/rubocop/1.79/cops_style.html#examples-stylesafenavigation

So the documentation has not been updated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that https://github.com/rubocop/rubocop/pull/14421/files/da0e3e7a1fb6bf65a17f32fcfa3e5e44f9aec36d#diff-736e663e58ab853ebfddd666306d7bddb37502d4cde5c1b8703cac7f1b20dcc8R871 is essentially the same.

And if you go up in the specs group you'll see that the allowed examples are earlier, so I think those should be grouped together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. In cases like foo.*(42) or foo.[](42), where the method call already includes a dot, converting them to safe navigation like foo&.*(42) or foo&.[42] doesn't reduce readability, so an offense is registered.

On the other hand, for calls like foo * 42 or foo[42], which don't include a dot, no offense is registered in order to preserve readability.

So, there's a distinction based on whether operator method call and index access are written with a dot.
I structured the specs to reflect this symmetry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I misunderstood the intent. I've updated it to keep the diff simple.

@koic koic force-pushed the fix_false_positives_for_style_safe_navigation branch from da0e3e7 to 837d5a2 Compare August 5, 2025 09:31
This PR fixes false positives for `Style/SafeNavigation`
when ternary expression with operator method call with method chain.

Fixes rubocop#14420.
@koic koic force-pushed the fix_false_positives_for_style_safe_navigation branch from 837d5a2 to c9ecf16 Compare August 5, 2025 09:42
@bbatsov bbatsov merged commit 9faac23 into rubocop:master Aug 5, 2025
39 of 40 checks passed
@koic koic deleted the fix_false_positives_for_style_safe_navigation branch August 5, 2025 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Style/SafeNavigation reports on complex nil checks that should pass

2 participants