Add new Lint/NonAtomicFileOperation cop

changelog/new_add_new_non_atomic_file_operation_cop.md

@@ -0,0 +1 @@
+* [#10696](https://github.com/rubocop/rubocop/pull/10696): Add new `Lint/NonAtomicFileOperation` cop. ([@ydah][])

config/default.yml

@@ -1956,6 +1956,12 @@ Lint/NoReturnInBeginEndBlocks:
   Enabled: pending
   VersionAdded: '1.2'
 
+Lint/NonAtomicFileOperation:
+  Description: Checks for non-atomic file operations.
+  Enabled: pending
+  VersionAdded: '<<next>>'
+  SafeAutoCorrect: false
+
 Lint/NonDeterministicRequireOrder:
   Description: 'Always sort arrays returned by Dir.glob when requiring files.'
   Enabled: true

lib/rubocop.rb

@@ -331,6 +331,7 @@
 require_relative 'rubocop/cop/lint/nested_percent_literal'
 require_relative 'rubocop/cop/lint/next_without_accumulator'
 require_relative 'rubocop/cop/lint/no_return_in_begin_end_blocks'
+require_relative 'rubocop/cop/lint/non_atomic_file_operation'
 require_relative 'rubocop/cop/lint/non_deterministic_require_order'
 require_relative 'rubocop/cop/lint/non_local_exit_from_iterator'
 require_relative 'rubocop/cop/lint/number_conversion'

lib/rubocop/cop/generator.rb

@@ -162,7 +162,7 @@ def write_unless_file_exists(path, contents)
         end
 
         dir = File.dirname(path)
-        FileUtils.mkdir_p(dir) unless File.exist?(dir)
+        FileUtils.mkdir_p(dir)
 
         File.write(path, contents)
         output.puts "[create] #{path}"

lib/rubocop/cop/lint/non_atomic_file_operation.rb

@@ -0,0 +1,121 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Lint
+      # Checks for non-atomic file operation.
+      # And then replace it with a nearly equivalent and atomic method.
+      #
+      # These can cause problems that are difficult to reproduce,
+      # especially in cases of frequent file operations in parallel,
+      # such as test runs with parallel_rspec.
+      #
+      # For examples: creating a directory if there is none, has the following problems
+      #
+      # An exception occurs when the directory didn't exist at the time of `exist?`,
+      # but someone else created it before `mkdir` was executed.
+      #
+      # Subsequent processes are executed without the directory that should be there
+      # when the directory existed at the time of `exist?`,
+      # but someone else deleted it shortly afterwards.
+      #
+      # @safety
+      #   This cop is unsafe, because autocorrection change to atomic processing.
+      #   The atomic processing of the replacement destination is not guaranteed
+      #   to be strictly equivalent to that before the replacement.
+      #
+      # @example
+      #   # bad
+      #   unless FileTest.exist?(path)
+      #     FileUtils.makedirs(path)
+      #   end
+      #
+      #   if FileTest.exist?(path)
+      #     FileUtils.remove(path)
+      #   end
+      #
+      #   # good
+      #   FileUtils.mkdir_p(path)
+      #
+      #   FileUtils.rm_rf(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
+
+        # @!method send_exist_node(node)
+        def_node_search :send_exist_node, <<-PATTERN
+          $(send (const nil? {:FileTest :File :Dir :Shell}) {:exist? :exists?} ...)
+        PATTERN
+
+        # @!method receiver_and_method_name(node)
+        def_node_matcher :receiver_and_method_name, <<-PATTERN
+          (send (const nil? $_) $_ ...)
+        PATTERN
+
+        # @!method force?(node)
+        def_node_search :force?, <<~PATTERN
+          (pair (sym :force) (:true))
+        PATTERN
+
+        # @!method explicit_not_force?(node)
+        def_node_search :explicit_not_force?, <<~PATTERN
+          (pair (sym :force) (:false))
+        PATTERN
+
+        def on_send(node)
+          return unless node.parent&.if_type?
+          return if explicit_not_force?(node)
+          return unless (exist_node = send_exist_node(node.parent).first)
+          return unless exist_node.first_argument == node.first_argument
+
+          offense(node, exist_node)
+        end
+
+        private
+
+        def offense(node, exist_node)
+          range = range_between(node.parent.loc.keyword.begin_pos,
+                                exist_node.loc.expression.end_pos)
+
+          add_offense(range, message: message(exist_node)) do |corrector|
+            autocorrect(corrector, node, range)
+          end
+        end
+
+        def message(node)
+          receiver, method_name = receiver_and_method_name(node)
+          format(MSG, receiver: receiver, method_name: method_name)
+        end
+
+        def autocorrect(corrector, node, range)
+          corrector.remove(range)
+          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)
+            'rm_rf'
+          end
+        end
+
+        def force_option?(node)
+          node.arguments.any? { |arg| force?(arg) }
+        end
+      end
+    end
+  end
+end

lib/rubocop/formatter/formatter_set.rb

@@ -55,7 +55,7 @@ def file_finished(file, offenses)
       def add_formatter(formatter_type, output_path = nil)
         if output_path
           dir_path = File.dirname(output_path)
-          FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path)
+          FileUtils.mkdir_p(dir_path)
           output = File.open(output_path, 'w')
         else
           output = $stdout

spec/rubocop/config_loader_spec.rb

@@ -1095,7 +1095,7 @@ class Loop < Base
       end
 
       after do
-        File.unlink cache_file if File.exist? cache_file
+        FileUtils.rm_rf cache_file
       end
 
       it 'resolves the inherited config' do
@@ -1124,7 +1124,7 @@ class Loop < Base
       end
 
       after do
-        File.unlink cache_file if File.exist? cache_file
+        FileUtils.rm_rf cache_file
       end
 
       it 'creates the cached file alongside the owning file' do
@@ -1150,7 +1150,7 @@ class Loop < Base
 
       after do
         [cache_file, cache_file2].each do |f|
-          File.unlink f if File.exist? f
+          FileUtils.rm_rf f
         end
       end
 

spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb

@@ -0,0 +1,176 @@
+# 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
+      expect_offense(<<~RUBY)
+        unless FileTest.exist?(path)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+          FileUtils.#{make_method}(path)
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+
+        #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(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|
+    it 'registers an offense when use `FileTest.exist?` before remove file' do
+      expect_offense(<<~RUBY)
+        if FileTest.exist?(path)
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Remove unnecessary existence checks `FileTest.exist?`.
+          FileUtils.#{remove_method}(path)
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+
+        #{trailing_whitespace}#{trailing_whitespace}FileUtils.rm_rf(path)
+
+      RUBY
+    end
+  end
+
+  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?`.
+        FileUtils.makedirs(path, force: true)
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.makedirs(path, force: true)
+
+    RUBY
+  end
+
+  it 'does not register an offense when use `FileTest.exist?` before creating file with an option `force: false`' do
+    expect_no_offenses(<<~RUBY)
+      unless FileTest.exists?(path)
+        FileUtils.makedirs(path, force: false)
+      end
+    RUBY
+  end
+
+  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?`.
+        FileUtils.makedirs(path, verbose: true)
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(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?`.
+        FileUtils.makedirs(path)
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(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?`.
+        FileUtils.makedirs(path)
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+
+      #{trailing_whitespace}#{trailing_whitespace}FileUtils.mkdir_p(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?`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      FileUtils.mkdir_p(path)#{trailing_whitespace}
+    RUBY
+  end
+
+  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?`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      FileUtils.rm_rf(path)#{trailing_whitespace}
+    RUBY
+  end
+
+  it 'does not register an offense when not checking for the existence' do
+    expect_no_offenses(<<~RUBY)
+      FileUtils.mkdir_p(path)
+    RUBY
+  end
+
+  it 'does not register an offense when checking for the existence of different files' do
+    expect_no_offenses(<<~RUBY)
+      FileUtils.mkdir_p(y) unless FileTest.exist?(path)
+    RUBY
+  end
+
+  it 'does not register an offense when not a method of file operation' do
+    expect_no_offenses(<<~RUBY)
+      unless FileUtils.exist?(path)
+        FileUtils.options_of(:rm)
+      end
+      unless FileUtils.exist?(path)
+        NotFile.remove(path)
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when not an exist check' do
+    expect_no_offenses(<<~RUBY)
+      unless FileUtils.options_of(:rm)
+        FileUtils.mkdir_p(path)
+      end
+      if FileTest.executable?(path)
+        FileUtils.remove(path)
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when processing other than file operations' do
+    expect_no_offenses(<<~RUBY)
+      unless FileTest.exist?(path)
+        FileUtils.makedirs(path)
+        do_something
+      end
+
+      unless FileTest.exist?(path)
+        do_something
+        FileUtils.makedirs(path)
+      end
+    RUBY
+  end
+end

spec/rubocop/remote_config_spec.rb

@@ -16,7 +16,7 @@
   end
 
   after do
-    File.unlink cached_file_path if File.exist? cached_file_path
+    FileUtils.rm_rf cached_file_path
   end
 
   describe '.file' do

spec/support/file_helper.rb

@@ -7,7 +7,7 @@ def create_file(file_path, content)
     file_path = File.expand_path(file_path)
 
     dir_path = File.dirname(file_path)
-    FileUtils.makedirs dir_path unless File.exist?(dir_path)
+    FileUtils.mkdir_p dir_path
 
     File.open(file_path, 'w') do |file|
       case content
@@ -29,7 +29,7 @@ def create_link(link_path, target_path)
     link_path = File.expand_path(link_path)
 
     dir_path = File.dirname(link_path)
-    FileUtils.makedirs dir_path unless File.exist?(dir_path)
+    FileUtils.mkdir_p dir_path
 
     FileUtils.symlink(target_path, link_path)
   end