diff --git a/changelog/fix_a_suggestions_for_safer_conversions_non_atomic_file_operation.md b/changelog/fix_a_suggestions_for_safer_conversions_non_atomic_file_operation.md new file mode 100644 index 00000000000..5ee630d3510 --- /dev/null +++ b/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][]) diff --git a/lib/rubocop/cop/lint/non_atomic_file_operation.rb b/lib/rubocop/cop/lint/non_atomic_file_operation.rb index f8d51982cf9..64e95c8175a 100644 --- a/lib/rubocop/cop/lint/non_atomic_file_operation.rb +++ b/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 `%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 + MSG_REMOVE_FILE_EXIST_CHECK = 'Remove unnecessary existence check ' \ + '`%s.%s`.' + MSG_CHANGE_FORCE_METHOD = 'Use atomic file operation method `FileUtils.%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 diff --git a/spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb b/spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb index 7a0d5e66842..b93eff0253c 100644 --- a/spec/rubocop/cop/lint/non_atomic_file_operation_spec.rb +++ b/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,14 +114,14 @@ 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 @@ -77,14 +129,14 @@ 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 @@ -92,22 +144,23 @@ 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