rubocop / rubocop-rails

A RuboCop extension focused on enforcing Rails best practices and coding conventions.
https://docs.rubocop.org/rubocop-rails
MIT License
803 stars 259 forks source link

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

Closed Jay-Schneider closed 2 years ago

Jay-Schneider commented 2 years ago

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 commented 2 years ago

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>]
Jay-Schneider commented 2 years ago

Ah, true. It is not even the same 😅

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