diff --git a/changelog/new_add_new_non_atomic_file_operation_cop.md b/changelog/new_add_new_non_atomic_file_operation_cop.md new file mode 100644 index 00000000000..f54fbbb0df3 --- /dev/null +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 8f1354619cf..9407053228b 100644 --- a/config/default.yml +++ b/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: '<>' + SafeAutoCorrect: false + Lint/NonDeterministicRequireOrder: Description: 'Always sort arrays returned by Dir.glob when requiring files.' Enabled: true diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 09898671488..83d25c8d8e6 100644 --- a/lib/rubocop.rb +++ b/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' diff --git a/lib/rubocop/cop/generator.rb b/lib/rubocop/cop/generator.rb index 01dab875907..ee1e581d2fc 100644 --- a/lib/rubocop/cop/generator.rb +++ b/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}" diff --git a/lib/rubocop/cop/lint/non_atomic_file_operation.rb b/lib/rubocop/cop/lint/non_atomic_file_operation.rb new file mode 100644 index 00000000000..bae3b2accf7 --- /dev/null +++ b/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 `%s.%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 diff --git a/lib/rubocop/formatter/formatter_set.rb b/lib/rubocop/formatter/formatter_set.rb index 3716aea00c6..021a003adcd 100644 --- a/lib/rubocop/formatter/formatter_set.rb +++ b/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 diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index 6fa1e21ec44..515f1e684e3 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/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 diff --git a/spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb b/spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb new file mode 100644 index 00000000000..5fa6a9381af --- /dev/null +++ b/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 diff --git a/spec/rubocop/remote_config_spec.rb b/spec/rubocop/remote_config_spec.rb index 7ba78f70010..e2c8b731b58 100644 --- a/spec/rubocop/remote_config_spec.rb +++ b/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 diff --git a/spec/support/file_helper.rb b/spec/support/file_helper.rb index d30f417c750..de2298bbfe0 100644 --- a/spec/support/file_helper.rb +++ b/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 diff --git a/tasks/changelog.rb b/tasks/changelog.rb index 0c03a6fce9d..912a8d7179a 100644 --- a/tasks/changelog.rb +++ b/tasks/changelog.rb @@ -25,7 +25,7 @@ def initialize(type:, body: last_commit_title, ref_type: nil, ref_id: nil, user: end def write - Dir.mkdir(ENTRIES_PATH) unless Dir.exist?(ENTRIES_PATH) + FileUtils.mkdir_p(ENTRIES_PATH) File.write(path, content) path end