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

2.16 autocorrect of Rails/RootPathnameMethods is unsafe, i.e. breaking code #763

Closed
Jay-Schneider opened this issue Sep 9, 2022 · 2 comments · Fixed by #768
Closed

2.16 autocorrect of Rails/RootPathnameMethods is unsafe, i.e. breaking code #763

Jay-Schneider opened this issue Sep 9, 2022 · 2 comments · Fixed by #768
Labels
bug Something isn't working

Comments

@Jay-Schneider
Copy link

Expected behavior

When rubocop detects a violation of of Rails/RootPathnameMethods and autocorrects it, I expect it to keep the behaviour of the code unchanged.

Actual behavior

The code breaks, so autocorrection for this cop is not safe in the current version.

Steps to reproduce the problem

I was able to reproduce the problem with the following simple file:

# foo.rb
puts Dir.glob(Rails.root.join("**/*.rb"))

When I run rails runner foo.rb it prints a list of files.

rubocop foo.rb reports the offense

foo.rb:2:6: C: [Correctable] Rails/RootPathnameMethods: Rails.root is a Pathname so you can just append #glob.
puts Dir.glob(Rails.root.join("**/*.rb"))

Now, using rubocop -a foo.rb changes the content of the file to

# foo.rb
puts Rails.root.join("**/*.rb").glob

which is not runnable code anymore and instead raises an error: foo.rb:2:in 'glob': wrong number of arguments (given 0, expected 1..2) (ArgumentError)

I think what is supposed to happen when autocorrecting is

# foo.rb
puts Rails.root.glob("**/*.rb")

because this actually seems to be equivalent to the original code and does not violate the Rails/RootPathnameMethods cop. But that's not what's happening.

RuboCop version

1.36.0 (using Parser 3.1.2.1, rubocop-ast 1.21.0, running on ruby 3.1.2) [arm64-darwin21]
  - rubocop-performance 1.14.3
  - rubocop-rails 2.16.0
@koic koic added the bug Something isn't working label Sep 9, 2022
koic added a commit to koic/rubocop-rails that referenced this issue Sep 10, 2022
Fixes rubocop#763.

This PR fix a false positive for `Rails/RootPathnameMethods` when using `Dir.glob`.
And marks unsafe for autocorrection because `Dir`'s methods return string element,
but `Pathname`'s methods return `Pathname` element.
@koic
Copy link
Member

koic commented Sep 10, 2022

I've opened #768, fixed autocorrection logic and marked the cop as unsafe due to the following incompatibility:

Dir.glob(Rails.root.join('foo/bar.rb'))
=> ["path/to/foo/bar.rb"]
Rails.root.glob('foo/bar.rb')
=> [#<Pathname:path/to/foo/bar.rb>]

@koic koic closed this as completed in #768 Sep 12, 2022
koic added a commit that referenced this issue Sep 12, 2022
…ils_root_pathname_methods

[Fix #763] Fix a false positive for `Rails/RootPathnameMethods`
@Jay-Schneider
Copy link
Author

Ah, true. It is not even the same 😅

I think marking the autocorrect unsafe is a fair solution. Thanks for addressing this so quickly.

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.

2 participants