Make Rails/RootPathnameMethods autocorrection safe

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][])

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: '<<next>>'
 
 Rails/RootPublicPath:
   Description: "Favor `Rails.public_path` over `Rails.root` with `'public'`."

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 = '`%<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
+        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
+
+        DIR_METHODS = (DIR_PATHNAMES_RETURNED_METHODS + DIR_NON_PATHNAMES_RETURNED_METHODS).freeze
 
-        FILE_METHODS = %i[
+        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,10 @@ class RootPathnameMethods < Base
           zero?
         ].to_set.freeze
 
+        FILE_PATHNAME_RETURNED_METHODS = %i[basename dirname expand_path 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 +157,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 +191,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 +208,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? }

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)