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

ConvertTry produces invalid Ruby #126

Closed
celsworth opened this issue Apr 30, 2019 · 4 comments · Fixed by #475
Closed

ConvertTry produces invalid Ruby #126

celsworth opened this issue Apr 30, 2019 · 4 comments · Fixed by #475
Labels
bug Something isn't working

Comments

@celsworth
Copy link

Expected behavior

Given this source:

class Foo
  def foo
    self.try(:bar).try(:baz)
  end
end

And this Rubocop config:

AllCops:
  TargetRubyVersion: 2.3

Rails:
  Enabled: true

Rails/SafeNavigation:
  ConvertTry: true

I expected auto-correction to produce working Ruby.

Actual behavior

Auto-correction produces this output:

# frozen_string_literal: true

class Foo
  def foo
    &.bar&.baz
  end
end

Which does not parse. The Rubocop output is:

Inspecting 1 file
E

Offenses:

test.rb:1:1: C: [Corrected] Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class Foo
^
test.rb:3:1: E: Lint/Syntax: class definition in method body
(Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)
class Foo
^^^^^
test.rb:3:5: C: [Corrected] Rails/SafeNavigation: Use safe navigation (&.) instead of try.
    self.try(:bar).try(:baz)
    ^^^^^^^^^^^^^^^^^^^^^^^^
test.rb:3:5: C: [Corrected] Style/RedundantSelf: Redundant self detected.
    self.try(:bar).try(:baz)
    ^^^^^^^^^^^^^^
test.rb:5:10: E: Lint/Syntax: unexpected token tANDDOT
(Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)
    &.bar&.baz
         ^^

1 file inspected, 5 offenses detected, 3 offenses corrected

Steps to reproduce the problem

Run rubocop -a with the above source and config. Note that setting the target Ruby version to 2.2 (the lowest supported) produces working Ruby (it leaves the try in place).

RuboCop version

0.68.0 (using Parser 2.6.0.0, running on ruby 2.3.3 x86_64-darwin17)
@Darhazer
Copy link
Member

It's a conflict between Style/RedundantSelf, which removes the self and Rails/SafeNavigation, which converts the try.

@rrosenblum
Copy link
Contributor

We seem to be getting more and more issues about auto-correction conflicts between cops. I'm starting to think that we need a better way to run auto-correction. Rather than running the corrections back to back, we need some way to check if the source has changed since we registered the offense. If it has, we need to skip auto-correction or recheck the code for an offense/correction.

@Drenmi
Copy link
Contributor

Drenmi commented May 10, 2019

This is strange. Did we make changes to the correction loop lately? It looks like both cops are attempting to correct the code on the same pass, which is not how it used to work. 🤔

When applying Rails/SafeNavigation and then Style/RedundantSelf (and the other way around) sequentially, there is no problem.

@rubocop rubocop deleted a comment from colleen-card May 23, 2019
@stale
Copy link

stale bot commented Aug 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@Drenmi Drenmi transferred this issue from rubocop/rubocop Sep 9, 2019
@koic koic added the bug Something isn't working label Sep 11, 2019
koic added a commit to koic/rubocop-rails that referenced this issue May 3, 2021
…tion`

Fixes rubocop#126.

This PR fixes an incorrect auto-correct for `Rails/SafeNavigation` with
`Style/RedndantSelf`.
@koic koic closed this as completed in #475 May 4, 2021
koic added a commit that referenced this issue May 4, 2021
…_safe_navigation

[Fix #126] Fix an incorrect auto-correct for `Rails/SafeNavigation`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants