From 4e1e7e3cad0d91a1a77af87ccebebc71457a5ba9 Mon Sep 17 00:00:00 2001 From: r7kamura Date: Wed, 26 Apr 2023 08:16:14 +0900 Subject: [PATCH] Make `Rails/RootPathnameMethods` autocorrection safe --- ...change_make_rails_root_pathname_methods.md | 1 + config/default.yml | 2 +- .../cop/rails/root_pathname_methods.rb | 65 +++++++++++++------ .../cop/rails/root_pathname_methods_spec.rb | 61 +++++++++++++---- 4 files changed, 97 insertions(+), 32 deletions(-) create mode 100644 changelog/change_make_rails_root_pathname_methods.md diff --git a/changelog/change_make_rails_root_pathname_methods.md b/changelog/change_make_rails_root_pathname_methods.md new file mode 100644 index 0000000000..e41d637309 --- /dev/null +++ b/changelog/change_make_rails_root_pathname_methods.md @@ -0,0 +1 @@ +* [#993](https://github.com/rubocop/rubocop-rails/pull/993): Make `Rails/RootPathnameMethods` autocorrection safe. ([@r7kamura][]) diff --git a/config/default.yml b/config/default.yml index 477bcd7c1d..3888b1dee2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -878,8 +878,8 @@ Rails/RootJoinChain: Rails/RootPathnameMethods: Description: 'Use `Rails.root` IO methods instead of passing it to `File`.' Enabled: pending - SafeAutoCorrect: false VersionAdded: '2.16' + VersionChanged: '<>' Rails/RootPublicPath: Description: "Favor `Rails.public_path` over `Rails.root` with `'public'`." diff --git a/lib/rubocop/cop/rails/root_pathname_methods.rb b/lib/rubocop/cop/rails/root_pathname_methods.rb index cc7c87a796..bb9fe6969d 100644 --- a/lib/rubocop/cop/rails/root_pathname_methods.rb +++ b/lib/rubocop/cop/rails/root_pathname_methods.rb @@ -11,10 +11,6 @@ module Rails # This cop works best when used together with # `Style/FileRead`, `Style/FileWrite` and `Rails/RootJoinChain`. # - # @safety - # This cop is unsafe for autocorrection because `Dir`'s `children`, `each_child`, `entries`, and `glob` - # methods return string element, but these methods of `Pathname` return `Pathname` element. - # # @example # # bad # File.open(Rails.root.join('db', 'schema.rb')) @@ -32,17 +28,20 @@ module Rails # Rails.root.join('db', 'schema.rb').write(content) # Rails.root.join('db', 'schema.rb').binwrite(content) # - class RootPathnameMethods < Base + class RootPathnameMethods < Base # rubocop:disable Metrics/ClassLength extend AutoCorrector include RangeHelp MSG = '`%s` is a `Pathname` so you can just append `#%s`.' - DIR_METHODS = %i[children delete each_child empty? entries exist? glob mkdir open rmdir unlink].to_set.freeze + DIR_NON_PATHNAMES_RETURNED_METHODS = %i[delete empty? exist? mkdir open rmdir unlink].to_set.freeze + + DIR_PATHNAMES_RETURNED_METHODS = %i[children each_child entries glob].to_set.freeze - FILE_METHODS = %i[ + DIR_METHODS = (DIR_PATHNAMES_RETURNED_METHODS + DIR_NON_PATHNAMES_RETURNED_METHODS).freeze + + FILE_NON_PATHNAME_RETURNED_METHODS = %i[ atime - basename binread binwrite birthtime @@ -53,19 +52,16 @@ class RootPathnameMethods < Base ctime delete directory? - dirname empty? executable? executable_real? exist? - expand_path extname file? fnmatch fnmatch? ftype grpowned? - join lchmod lchown lstat @@ -77,9 +73,6 @@ class RootPathnameMethods < Base readable? readable_real? readlines - readlink - realdirpath - realpath rename setgid? setuid? @@ -102,6 +95,18 @@ class RootPathnameMethods < Base zero? ].to_set.freeze + FILE_PATHNAME_RETURNED_METHODS = %i[ + basename + dirname + expand_path + join + readlink + realdirpath + realpath + ].to_set.freeze + + FILE_METHODS = (FILE_PATHNAME_RETURNED_METHODS + FILE_NON_PATHNAME_RETURNED_METHODS).freeze + FILE_TEST_METHODS = %i[ blockdev? chardev? @@ -160,13 +165,24 @@ class RootPathnameMethods < Base (send (const {nil? cbase} :Rails) {:root :public_path}) PATTERN + # @!method dir_pathnames_returned_method?(node) + def_node_matcher :dir_pathnames_returned_method?, <<~PATTERN + (send (const {nil? cbase} :Dir) DIR_PATHNAMES_RETURNED_METHODS ...) + PATTERN + + # @!method file_pathname_returned_method?(node) + def_node_matcher :file_pathname_returned_method?, <<~PATTERN + (send (const {nil? cbase} {:IO :File}) FILE_PATHNAME_RETURNED_METHODS ...) + 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| + suffix = build_replacement_suffix(node) replacement = if dir_glob?(node) - build_path_glob_replacement(path, method) + build_path_glob_replacement(path, method, suffix) else - build_path_replacement(path, method, args) + build_path_replacement(path, method, args, suffix) end corrector.replace(node, replacement) @@ -183,15 +199,15 @@ def evidence(node) yield(method, path, args, rails_root) end - def build_path_glob_replacement(path, method) + def build_path_glob_replacement(path, method, suffix) receiver = range_between(path.source_range.begin_pos, path.children.first.loc.selector.end_pos).source argument = path.arguments.one? ? path.first_argument.source : join_arguments(path.arguments) - "#{receiver}.#{method}(#{argument})" + "#{receiver}.#{method}(#{argument})#{suffix}" end - def build_path_replacement(path, method, args) + def build_path_replacement(path, method, args, suffix) path_replacement = path.source if path.arguments? && !path.parenthesized_call? path_replacement[' '] = '(' @@ -200,9 +216,20 @@ def build_path_replacement(path, method, args) replacement = "#{path_replacement}.#{method}" replacement += "(#{args.map(&:source).join(', ')})" unless args.empty? + replacement += suffix replacement end + def build_replacement_suffix(node) + if dir_pathnames_returned_method?(node) + '.map(&:to_s)' + elsif file_pathname_returned_method?(node) + '.to_s' + else + '' + end + end + def include_interpolation?(arguments) arguments.any? do |argument| argument.children.any? { |child| child.respond_to?(:begin_type?) && child.begin_type? } diff --git a/spec/rubocop/cop/rails/root_pathname_methods_spec.rb b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb index 709f8592b5..639c7d32d8 100644 --- a/spec/rubocop/cop/rails/root_pathname_methods_spec.rb +++ b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb @@ -2,15 +2,13 @@ RSpec.describe RuboCop::Cop::Rails::RootPathnameMethods, :config do { - Dir: described_class::DIR_METHODS, - File: described_class::FILE_METHODS, + Dir: described_class::DIR_NON_PATHNAMES_RETURNED_METHODS, + File: described_class::FILE_NON_PATHNAME_RETURNED_METHODS, FileTest: described_class::FILE_TEST_METHODS, FileUtils: described_class::FILE_UTILS_METHODS, - IO: described_class::FILE_METHODS + IO: described_class::FILE_NON_PATHNAME_RETURNED_METHODS }.each do |receiver, methods| methods.each do |method| - next if method == :glob - 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) @@ -54,7 +52,7 @@ RUBY expect_correction(<<~RUBY) - Rails.root.glob('**/*.rb') + Rails.root.glob('**/*.rb').map(&:to_s) RUBY end @@ -65,7 +63,7 @@ RUBY expect_correction(<<~RUBY) - Rails.root.glob('**/*.rb') + Rails.root.glob('**/*.rb').map(&:to_s) RUBY end @@ -76,7 +74,7 @@ RUBY expect_correction(<<~'RUBY') - Rails.root.glob("**/#{path}/*.rb") + Rails.root.glob("**/#{path}/*.rb").map(&:to_s) RUBY end @@ -87,7 +85,7 @@ RUBY expect_correction(<<~RUBY) - Rails.root.glob('**/*.rb') + Rails.root.glob('**/*.rb').map(&:to_s) RUBY end @@ -103,7 +101,7 @@ RUBY expect_correction(<<~RUBY) - Rails.root.glob("**/*.rb") + Rails.root.glob("**/*.rb").map(&:to_s) RUBY end end @@ -115,7 +113,7 @@ RUBY expect_correction(<<~'RUBY') - Rails.root.glob("**/#{path}/*.rb") + Rails.root.glob("**/#{path}/*.rb").map(&:to_s) RUBY end @@ -128,13 +126,52 @@ RUBY expect_correction(<<~'RUBY') - Rails.root.glob("db/seeds/#{Rails.env}/*.rb").sort.each do |file| + Rails.root.glob("db/seeds/#{Rails.env}/*.rb").map(&:to_s).sort.each do |file| load file end RUBY end end + { + Dir: described_class::DIR_PATHNAMES_RETURNED_METHODS - %i[glob] + }.each do |receiver, methods| + methods.each do |method| + context "when `#{receiver}.#{method}(Rails.root)` is used" do + it 'registers an offense' do + expect_offense(<<~RUBY, receiver: receiver, method: method) + %{receiver}.%{method}(Rails.root) + ^{receiver}^^{method}^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#%{method}`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.#{method}.map(&:to_s) + RUBY + end + end + end + end + + { + File: described_class::FILE_PATHNAME_RETURNED_METHODS, + IO: described_class::FILE_PATHNAME_RETURNED_METHODS + }.each do |receiver, methods| + methods.each do |method| + context "when `#{receiver}.#{method}(Rails.root)` is used" do + it 'registers an offense' do + expect_offense(<<~RUBY, receiver: receiver, method: method) + %{receiver}.%{method}(Rails.root) + ^{receiver}^^{method}^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#%{method}`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.#{method}.to_s + RUBY + end + 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)