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

FilePath safe autocorrect breaks code #1071

Closed
mkllnk opened this issue Aug 17, 2023 · 1 comment · Fixed by #1433
Closed

FilePath safe autocorrect breaks code #1071

mkllnk opened this issue Aug 17, 2023 · 1 comment · Fixed by #1433
Labels
bug Something isn't working

Comments

@mkllnk
Copy link

mkllnk commented Aug 17, 2023

The cop Rails/FilePath replaces File.join with Pathname.join but they are not equivalent when the argument has a leading slash. A similar issue has been reported before:

The replacement breaks code with path parts starting with a slash:

default_url = "/default_images/ofn-logo.png"

File.join(Rails.root, 'public', default_url)
=> "/absolute/path/to/public/default_images/ofn-logo.png"

Rails.root.join('public', default_url).to_s
=> "/default_images/ofn-logo.png"

Expected behavior

Rails/FilePath should be marked as unsafe. And the message could be updated:

Rails/FilePath: Prefer Rails.root.join('path/to').to_s joining paths without leading slash or File.join(Rails.root.to_s, '/path/to') joining paths with leading slash.

Actual behavior

Message:

Rails/FilePath: Prefer Rails.root.join('path/to').to_s.
# Applied by: rubocop  -a --only Rails/FilePath
- File.join(Rails.root, 'public', default_url)
+ Rails.root.join('public', default_url).to_s

Note that a shortened version is not flagged by this cop:

# good
File.join(Rails.public_path, default_url)

However, it's flagged by Rails/RootPathnameMethods which is correctly marked as unsafe.

Steps to reproduce the problem

echo "File.join(Rails.root, 'public', default_url)" > test.rb
rubocop test.rb --only Rails/FilePath -a

RuboCop version

$ [bundle exec] rubocop -V
1.56.0 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 3.1.4) [x86_64-linux]
  - rubocop-rails 2.20.2
@koic koic added the bug Something isn't working label Sep 6, 2023
corsonknowles added a commit to corsonknowles/rubocop-rails that referenced this issue Dec 23, 2024
…filepaths by marking cop as unsafe for autocorrection

We correctly add a @safety annotation for Rails/FilePath since it can produce invalid code like this:

# pass any string argument beginning with '/' to Rails.root.join and it will omit the Rails path

(dev)> Rails.root.join '/'
=> #<Pathname:/>
(dev)> Rails.root.join '//'
=> #<Pathname://>
(dev)> Rails.root.join '/abc'
=> #<Pathname:/abc>
(dev)> Rails.root.join '/abc/'
=> #<Pathname:/abc/>
@corsonknowles
Copy link

Safe autocorrect should never break code, so I have opened a PR here to mark this cop as unsafe for this reason:

ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Jan 27, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Jan 30, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 2, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 2, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 2, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 2, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 2, 2025
corsonknowles added a commit to corsonknowles/rubocop-rails that referenced this issue Feb 3, 2025
…filepaths by marking cop as unsafe for autocorrection

We correctly add a @safety annotation for Rails/FilePath since it can produce invalid code like this:

# pass any string argument beginning with '/' to Rails.root.join and it will omit the Rails path

(dev)> Rails.root.join '/'
=> #<Pathname:/>
(dev)> Rails.root.join '//'
=> #<Pathname://>
(dev)> Rails.root.join '/abc'
=> #<Pathname:/abc>
(dev)> Rails.root.join '/abc/'
=> #<Pathname:/abc/>
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 4, 2025
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 5, 2025
….join` with variables and ignore leading and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 6, 2025
….join` with variables and ignore leading and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`
@koic koic closed this as completed in 65199b8 Feb 7, 2025
koic added a commit that referenced this issue Feb 7, 2025
…the-rails-file-path-cop-when-file-join-is-used-with-a-variable

[Fix #1071] Fix `Rails/FilePath` cop to correctly handle `File.join` with variables and ignore leading and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`
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
3 participants