Fix a suggestions for safer conversions for Lint/NonAtomicFileOperation

changelog/fix_a_suggestions_for_safer_conversions_non_atomic_file_operation.md

@@ -0,0 +1 @@
+* [#10781](https://github.com/rubocop/rubocop/pull/10781): Fix a suggestions for safer conversions for `Lint/NonAtomicFileOperation`. ([@ydah][])

lib/rubocop/cop/lint/non_atomic_file_operation.rb

@@ -25,30 +25,38 @@ module Lint
       #   to be strictly equivalent to that before the replacement.
       #
       # @example
-      #   # bad
-      #   unless FileTest.exist?(path)
-      #     FileUtils.makedirs(path)
+      #   # bad - race condition with another process may result in an error in `mkdir`
+      #   unless Dir.exist?(path)
+      #     FileUtils.mkdir(path)
       #   end
       #
-      #   if FileTest.exist?(path)
+      #   # good - atomic and idempotent creation
+      #   FileUtils.mkdir_p(path)
+      #
+      #   # bad - race condition with another process may result in an error in `remove`
+      #   if File.exist?(path)
       #     FileUtils.remove(path)
       #   end
       #
-      #   # good
-      #   FileUtils.mkdir_p(path)
-      #
-      #   FileUtils.rm_rf(path)
+      #   # good - atomic and idempotent removal
+      #   FileUtils.rm_f(path)
       #
       class NonAtomicFileOperation < Base
         extend AutoCorrector
         include Alignment
         include RangeHelp
 
-        MSG = 'Remove unnecessary existence checks `%<receiver>s.%<method_name>s`.'
-        MAKE_METHODS = %i[makedirs mkdir mkdir_p mkpath].freeze
-        REMOVE_METHODS = %i[remove remove_dir remove_entry remove_entry_secure delete unlink
-                            remove_file rm rm_f rm_r rm_rf rmdir rmtree safe_unlink].freeze
-        RESTRICT_ON_SEND = (MAKE_METHODS + REMOVE_METHODS).freeze
+        MSG_REMOVE_FILE_EXIST_CHECK = 'Remove unnecessary existence check ' \
+                                      '`%<receiver>s.%<method_name>s`.'
+        MSG_CHANGE_FORCE_METHOD = 'Use atomic file operation method `FileUtils.%<method_name>s`.'
+        MAKE_FORCE_METHODS = %i[makedirs mkdir_p mkpath].freeze
+        MAKE_METHODS = %i[mkdir].freeze
+        REMOVE_RECURSIVE_METHODS = %i[rm_r rmtree].freeze
+        REMOVE_FORCE_METHODS = %i[rm_f rm_rf].freeze
+        REMOVE_METHODS = %i[remove remove_dir remove_entry remove_entry_secure
+                            delete unlink remove_file rm rmdir safe_unlink].freeze
+        RESTRICT_ON_SEND = (MAKE_METHODS + MAKE_FORCE_METHODS + REMOVE_METHODS +
+          REMOVE_FORCE_METHODS + REMOVE_RECURSIVE_METHODS).freeze
 
         # @!method send_exist_node(node)
         def_node_search :send_exist_node, <<-PATTERN
@@ -87,39 +95,60 @@ def allowable_use_with_if?(if_node)
         end
 
         def register_offense(node, exist_node)
+          unless force_method?(node)
+            add_offense(node,
+                        message: format(MSG_CHANGE_FORCE_METHOD,
+                                        method_name: replacement_method(node)))
+          end
+
           range = range_between(node.parent.loc.keyword.begin_pos,
                                 exist_node.loc.expression.end_pos)
-
-          add_offense(range, message: message(exist_node)) do |corrector|
+          add_offense(range, message: message_remove_file_exist_check(exist_node)) do |corrector|
             autocorrect(corrector, node, range)
           end
         end
 
-        def message(node)
+        def message_remove_file_exist_check(node)
           receiver, method_name = receiver_and_method_name(node)
-          format(MSG, receiver: receiver, method_name: method_name)
+          format(MSG_REMOVE_FILE_EXIST_CHECK, receiver: receiver, method_name: method_name)
         end
 
         def autocorrect(corrector, node, range)
           corrector.remove(range)
+          autocorrect_replace_method(corrector, node)
+          corrector.remove(node.parent.loc.end) if node.parent.multiline?
+        end
+
+        def autocorrect_replace_method(corrector, node)
+          return if force_method?(node)
+
           corrector.replace(node.child_nodes.first.loc.name, 'FileUtils')
           corrector.replace(node.loc.selector, replacement_method(node))
-          corrector.remove(node.parent.loc.end) if node.parent.multiline?
         end
 
         def replacement_method(node)
-          return node.method_name if force_option?(node)
-
           if MAKE_METHODS.include?(node.method_name)
             'mkdir_p'
-          elsif REMOVE_METHODS.include?(node.method_name)
+          elsif REMOVE_RECURSIVE_METHODS.include?(node.method_name)
             'rm_rf'
+          elsif REMOVE_METHODS.include?(node.method_name)
+            'rm_f'
+          else
+            node.method_name
           end
         end
 
+        def force_method?(node)
+          force_method_name?(node) || force_option?(node)
+        end
+
         def force_option?(node)
           node.arguments.any? { |arg| force?(arg) }
         end
+
+        def force_method_name?(node)
+          (MAKE_FORCE_METHODS + REMOVE_FORCE_METHODS).include?(node.method_name)
+        end
       end
     end
   end

spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb

@@ -1,33 +1,85 @@
 # frozen_string_literal: true
 
 RSpec.describe RuboCop::Cop::Lint::NonAtomicFileOperation, :config do
-  %i[makedirs mkdir mkdir_p mkpath].each do |make_method|
-    it 'registers an offense when use `FileTest.exist?` before creating file' do
+  it 'registers an offense when use `FileTest.exist?` before creating file' do
+    expect_offense(<<~RUBY)
+      unless FileTest.exist?(path)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
+        FileUtils.mkdir(path)
+        ^^^^^^^^^^^^^^^^^^^^^ Use atomic file operation method `FileUtils.mkdir_p`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(path)
+
+    RUBY
+  end
+
+  %i[makedirs mkdir_p mkpath].each do |make_method|
+    it 'registers an offense when use `FileTest.exist?` before force creating file' do
       expect_offense(<<~RUBY)
         unless FileTest.exist?(path)
-        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
           FileUtils.#{make_method}(path)
         end
       RUBY
 
       expect_correction(<<~RUBY)
 
-        #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(path)
+        #{trailing_whitespace}#{trailing_whitespace}FileUtils.#{make_method}(path)
 
       RUBY
     end
   end
 
   %i[remove remove_dir remove_entry remove_entry_secure delete unlink
-     remove_file rm rm_f rm_r rm_rf rmdir rmtree safe_unlink].each do |remove_method|
+     remove_file rm rmdir safe_unlink].each do |remove_method|
     it 'registers an offense when use `FileTest.exist?` before remove file' do
+      expect_offense(<<~RUBY, remove_method: remove_method)
+        if FileTest.exist?(path)
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
+          FileUtils.#{remove_method}(path)
+          ^^^^^^^^^^^{remove_method}^^^^^^ Use atomic file operation method `FileUtils.rm_f`.
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+
+        #{trailing_whitespace}#{trailing_whitespace}FileUtils.rm_f(path)
+
+      RUBY
+    end
+  end
+
+  %i[rm_f rm_rf].each do |remove_method|
+    it 'registers an offense when use `FileTest.exist?` before force remove file' do
       expect_offense(<<~RUBY)
         if FileTest.exist?(path)
-        ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
           FileUtils.#{remove_method}(path)
         end
       RUBY
 
+      expect_correction(<<~RUBY)
+
+        #{trailing_whitespace}#{trailing_whitespace}FileUtils.#{remove_method}(path)
+
+      RUBY
+    end
+  end
+
+  %i[rm_r rmtree].each do |remove_method|
+    it 'registers an offense when use `FileTest.exist?` before remove recursive file' do
+      expect_offense(<<~RUBY, remove_method: remove_method)
+        if FileTest.exist?(path)
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
+          FileUtils.#{remove_method}(path)
+          ^^^^^^^^^^^{remove_method}^^^^^^ Use atomic file operation method `FileUtils.rm_rf`.
+        end
+      RUBY
+
       expect_correction(<<~RUBY)
 
         #{trailing_whitespace}#{trailing_whitespace}FileUtils.rm_rf(path)
@@ -39,7 +91,7 @@
   it 'registers an offense when use `FileTest.exist?` before creating file with an option `force: true`' do
     expect_offense(<<~RUBY)
       unless FileTest.exists?(path)
-      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exists?`.
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exists?`.
         FileUtils.makedirs(path, force: true)
       end
     RUBY
@@ -62,52 +114,53 @@
   it 'registers an offense when use `FileTest.exist?` before creating file with an option not `force`' do
     expect_offense(<<~RUBY)
       unless FileTest.exists?(path)
-      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exists?`.
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exists?`.
         FileUtils.makedirs(path, verbose: true)
       end
     RUBY
 
     expect_correction(<<~RUBY)
 
-      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(path, verbose: true)
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.makedirs(path, verbose: true)
 
     RUBY
   end
 
   it 'registers an offense when use `FileTest.exists?` before creating file' do
     expect_offense(<<~RUBY)
       unless FileTest.exists?(path)
-      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exists?`.
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exists?`.
         FileUtils.makedirs(path)
       end
     RUBY
 
     expect_correction(<<~RUBY)
 
-      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(path)
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.makedirs(path)
 
     RUBY
   end
 
   it 'registers an offense when use `FileTest.exist?` with negated `if` before creating file' do
     expect_offense(<<~RUBY)
       if !FileTest.exist?(path)
-      ^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+      ^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
         FileUtils.makedirs(path)
       end
     RUBY
 
     expect_correction(<<~RUBY)
 
-      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(path)
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.makedirs(path)
 
     RUBY
   end
 
   it 'registers an offense when use file existence checks `unless` by postfix before creating file' do
     expect_offense(<<~RUBY)
-      FileUtils.makedirs(path) unless FileTest.exist?(path)
-                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+      FileUtils.mkdir(path) unless FileTest.exist?(path)
+                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
+      ^^^^^^^^^^^^^^^^^^^^^ Use atomic file operation method `FileUtils.mkdir_p`.
     RUBY
 
     expect_correction(<<~RUBY)
@@ -118,11 +171,12 @@
   it 'registers an offense when use file existence checks `if` by postfix before removing file' do
     expect_offense(<<~RUBY)
       FileUtils.remove(path) if FileTest.exist?(path)
-                             ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+                             ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence check `FileTest.exist?`.
+      ^^^^^^^^^^^^^^^^^^^^^^ Use atomic file operation method `FileUtils.rm_f`.
     RUBY
 
     expect_correction(<<~RUBY)
-      FileUtils.rm_rf(path)#{trailing_whitespace}
+      FileUtils.rm_f(path)#{trailing_whitespace}
     RUBY
   end