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

Add new Rails/RootPathnameMethods cop #587

Merged
merged 1 commit into from Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/new_add_new_railsrootpathnamemethods_cop.md
@@ -0,0 +1 @@
* [#587](https://github.com/rubocop/rubocop-rails/pull/587): Add new `Rails/RootPathnameMethods` cop. ([@leoarnold][])
1 change: 1 addition & 0 deletions codespell.txt
@@ -1 +1,2 @@
developpment
filetest
5 changes: 5 additions & 0 deletions config/default.yml
Expand Up @@ -788,6 +788,11 @@ Rails/RootJoinChain:
Enabled: pending
VersionAdded: '2.13'

Rails/RootPathnameMethods:
Description: 'Use `Rails.root` IO methods instead of passing it to `File`.'
Enabled: pending
VersionAdded: '<<next>>'

Rails/RootPublicPath:
Description: "Favor `Rails.public_path` over `Rails.root` with `'public'`."
Enabled: pending
Expand Down
194 changes: 194 additions & 0 deletions lib/rubocop/cop/rails/root_pathname_methods.rb
@@ -0,0 +1,194 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Use `Rails.root` IO methods instead of passing it to `File`.
#
# `Rails.root` is an instance of `Pathname`
# so we can apply many IO methods directly.
#
# This cop works best when used together with
# `Style/FileRead`, `Style/FileWrite` and `Rails/RootJoinChain`.
#
# @example
# # bad
# File.open(Rails.root.join('db', 'schema.rb'))
# File.open(Rails.root.join('db', 'schema.rb'), 'w')
# File.read(Rails.root.join('db', 'schema.rb'))
# File.binread(Rails.root.join('db', 'schema.rb'))
# File.write(Rails.root.join('db', 'schema.rb'), content)
# File.binwrite(Rails.root.join('db', 'schema.rb'), content)
#
# # good
# Rails.root.join('db', 'schema.rb').open
# Rails.root.join('db', 'schema.rb').open('w')
# Rails.root.join('db', 'schema.rb').read
# Rails.root.join('db', 'schema.rb').binread
# Rails.root.join('db', 'schema.rb').write(content)
# Rails.root.join('db', 'schema.rb').binwrite(content)
#
class RootPathnameMethods < Base
extend AutoCorrector

MSG = '`%<rails_root>s` is a `Pathname` so you can just append `#%<method>s`.'

DIR_METHODS = %i[
children
delete
each_child
empty?
entries
exist?
glob
mkdir
open
rmdir
unlink
].to_set.freeze

FILE_METHODS = %i[
Copy link
Member

Choose a reason for hiding this comment

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

File.methods - File.superclass.methods also returns e.g. :absolute_path which is not on those lists.
Which makes me think does it make sense to fill in those constants with those calls?
On the other hand, if the Ruby version with which rubocop is run is different from the target Ruby version, there might be differences.
However, does it matter much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... but Pathname does not have a #absolute_path method. Not sure I can follow here.

I generated this list using

File.methods.sort.select do |m|
  Pathname.instance_methods.include?(m) && !Object.instance_methods.include?(m)
end

Copy link
Member

Choose a reason for hiding this comment

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

Understood.
So basically (File.public_methods(false) & Pathname.instance_methods(false)).sort.
Does it make sense to set the constant to this statement?
What are the consequences? New methods being added on File and to Pathname in future Ruby versions change the behaviour of the cop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, just because there is File.foo and Pathname#foo, it is not guaranteed that File.foo(p, ...) and p.foo(...) is the same, so I'd rather maintain a list of safe methods.

Also, there may be cases where the Ruby that runs Rubocop is not the Ruby which runs production code (e.g. you develop a gem on Ruby 2, but someone uses it on JRuby 9). Are those standard libraries necessarily equivalent?

atime
basename
binread
binwrite
birthtime
blockdev?
chardev?
chmod
chown
ctime
delete
directory?
dirname
empty?
executable?
executable_real?
exist?
expand_path
extname
file?
fnmatch
fnmatch?
ftype
grpowned?
join
lchmod
lchown
lstat
mtime
open
owned?
pipe?
read
readable?
readable_real?
readlines
readlink
realdirpath
realpath
rename
setgid?
setuid?
size
size?
socket?
split
stat
sticky?
symlink?
sysopen
truncate
unlink
utime
world_readable?
world_writable?
writable?
writable_real?
write
zero?
].to_set.freeze

FILE_TEST_METHODS = %i[
blockdev?
chardev?
directory?
empty?
executable?
executable_real?
exist?
file?
grpowned?
owned?
pipe?
readable?
readable_real?
setgid?
setuid?
size
size?
socket?
sticky?
symlink?
world_readable?
world_writable?
writable?
writable_real?
zero?
].to_set.freeze

FILE_UTILS_METHODS = %i[
chmod
chown
mkdir
mkpath
rmdir
rmtree
].to_set.freeze

RESTRICT_ON_SEND = (DIR_METHODS + FILE_METHODS + FILE_TEST_METHODS + FILE_UTILS_METHODS).to_set.freeze

def_node_matcher :pathname_method, <<~PATTERN
{
(send (const {nil? cbase} :Dir) $DIR_METHODS $_ $...)
(send (const {nil? cbase} {:IO :File}) $FILE_METHODS $_ $...)
(send (const {nil? cbase} :FileTest) $FILE_TEST_METHODS $_ $...)
(send (const {nil? cbase} :FileUtils) $FILE_UTILS_METHODS $_ $...)
}
PATTERN

def_node_matcher :rails_root_pathname?, <<~PATTERN
{
$#rails_root?
(send $#rails_root? :join ...)
}
PATTERN

# @!method rails_root?(node)
def_node_matcher :rails_root?, <<~PATTERN
(send (const {nil? cbase} :Rails) {:root :public_path})
PATTERN

def on_send(node)
evidence(node) do |method, path, args, rails_root|
add_offense(node, message: format(MSG, method: method, rails_root: rails_root.source)) do |corrector|
replacement = "#{path.source}.#{method}"
replacement += "(#{args.map(&:source).join(', ')})" unless args.empty?

corrector.replace(node, replacement)
end
end
end

private

def evidence(node)
return if node.method?(:open) && node.parent&.send_type?
return unless (method, path, args = pathname_method(node)) && (rails_root = rails_root_pathname?(path))

yield(method, path, args, rails_root)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Expand Up @@ -97,6 +97,7 @@
require_relative 'rails/reversible_migration'
require_relative 'rails/reversible_migration_method_definition'
require_relative 'rails/root_join_chain'
require_relative 'rails/root_pathname_methods'
require_relative 'rails/root_public_path'
require_relative 'rails/safe_navigation'
require_relative 'rails/safe_navigation_with_blank'
Expand Down
92 changes: 92 additions & 0 deletions spec/rubocop/cop/rails/root_pathname_methods_spec.rb
@@ -0,0 +1,92 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::RootPathnameMethods, :config do
{
Dir: described_class::DIR_METHODS,
Copy link
Member

Choose a reason for hiding this comment

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

It's my personal preference, but I don't feel too confident when some collections from the code are used in tests directly. It may sound far-fetched, but what if someone accidentally removes a line from that array? Specs won't fail.
I'll leave this to @koic to decide on.

File: described_class::FILE_METHODS,
FileTest: described_class::FILE_TEST_METHODS,
FileUtils: described_class::FILE_UTILS_METHODS,
IO: described_class::FILE_METHODS
}.each do |receiver, methods|
methods.each do |method|
it "registers an offense when using `#{receiver}.#{method}(Rails.public_path)` (if arity exists)" do
expect_offense(<<~RUBY, receiver: receiver, method: method)
%{receiver}.%{method}(Rails.public_path)
^{receiver}^^{method}^^^^^^^^^^^^^^^^^^^ `Rails.public_path` is a `Pathname` so you can just append `#%{method}`.
RUBY

expect_correction(<<~RUBY)
Rails.public_path.#{method}
RUBY
end

it "registers an offense when using `::#{receiver}.#{method}(::Rails.root.join(...))` (if arity exists)" do
expect_offense(<<~RUBY, receiver: receiver, method: method)
::%{receiver}.%{method}(::Rails.root.join('db', 'schema.rb'))
^^^{receiver}^^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `::Rails.root` is a `Pathname` so you can just append `#%{method}`.
RUBY

expect_correction(<<~RUBY)
::Rails.root.join('db', 'schema.rb').#{method}
RUBY
end

it "registers an offense when using `::#{receiver}.#{method}(::Rails.root.join(...), ...)` (if arity exists)" do
expect_offense(<<~RUBY, receiver: receiver, method: method)
::%{receiver}.%{method}(::Rails.root.join('db', 'schema.rb'), 20, 5)
^^^{receiver}^^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `::Rails.root` is a `Pathname` so you can just append `#%{method}`.
RUBY

expect_correction(<<~RUBY)
::Rails.root.join('db', 'schema.rb').#{method}(20, 5)
RUBY
end
end
end

# This is handled by `Rails/RootJoinChain`
it 'does not register an offense when using `File.read(Rails.root.join(...).join(...))`' do
expect_no_offenses(<<~RUBY)
File.read(Rails.root.join('db').join('schema.rb'))
RUBY
end

# This is handled by `Style/FileRead`
it 'does not register an offense when using `File.open(Rails.root.join(...)).read`' do
expect_no_offenses(<<~RUBY)
File.open(Rails.root.join('db', 'schema.rb')).read
RUBY
end

# This is handled by `Style/FileRead`
it 'does not register an offense when using `File.open(Rails.root.join(...)).binread`' do
expect_no_offenses(<<~RUBY)
File.open(Rails.root.join('db', 'schema.rb')).binread
RUBY
end

# This is handled by `Style/FileWrite`
it 'does not register an offense when using `File.open(Rails.root.join(...)).write(content)`' do
expect_no_offenses(<<~RUBY)
File.open(Rails.root.join('db', 'schema.rb')).write(content)
RUBY
end

# This is handled by `Style/FileWrite`
it 'does not register an offense when using `File.open(Rails.root.join(...)).binwrite(content)`' do
expect_no_offenses(<<~RUBY)
File.open(Rails.root.join('db', 'schema.rb')).binwrite(content)
RUBY
end

it 'registers an offense when using `File.open(Rails.root.join(...), ...)` inside an iterator' do
expect_offense(<<~RUBY)
files.map { |file| File.open(Rails.root.join('db', file), 'wb') }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#open`.
RUBY

expect_correction(<<~RUBY)
files.map { |file| Rails.root.join('db', file).open('wb') }
RUBY
end
end