False positive of NonAtomicFileOperation for FileUtils.rm_r

[AlexWayfer]

From this discussion: #10760 (comment)

Expected behavior

No offenses or no exceptions for FileUtils.rm_r.

Actual behavior

If I have the code:

FileUtils.rm_r gem_name if Dir.exist? gem_name

it causes offenses by NonAtomicFileOperation cop.

If I remove the condition — it causes the exception:

     Failure/Error: FileUtils.rm_r gem_name
     
     Errno::ENOENT:
       No such file or directory @ apply2files - foo_bar

Even with require 'fileutils'.

Steps to reproduce the problem

The code is above.

RuboCop version

$ [bundle exec] rubocop -V
1.31.2 (using Parser 3.1.2.0, rubocop-ast 1.19.1, running on ruby 3.1.2 x86_64-linux)
  - rubocop-performance 1.14.2
  - rubocop-rspec 2.12.1

[ydah]

The offense message is confusing, so we improved the message at #10781

For the following code:

# frozen_string_literal: true

FileUtils.rm_r gem_name if Dir.exist? gem_name

The following message is output:

:
test.rb:3:1: W: Lint/NonAtomicFileOperation: Use atomic file operation method FileUtils.rm_rf.
FileUtils.rm_r gem_name if Dir.exist? gem_name
^^^^^^^^^^^^^^^^^^^^^^^
test.rb:3:25: W: [Correctable] Lint/NonAtomicFileOperation: Remove unnecessary existence check Dir.exist?.
FileUtils.rm_r gem_name if Dir.exist? gem_name
                        ^^^^^^^^^^^^^^^^^^^^^^

Expected changes:

# frozen_string_literal: true

- FileUtils.rm_r gem_name if Dir.exist? gem_name
+ FileUtils.rm_rf gem_name

[AlexWayfer]

@ydah thank you, it's a good change. But… --force option is not only for cases where files (or dirs) don't exist. It'll also delete all protected files, probably something else. It's not an equal I beleive.

[ydah]

@AlexWayfer Yes, I also certainly don't think they are strictly equivalent. The reason for adding this cop is that these can cause problems that are difficult to reproduce, especially when frequent file operations are performed in parallel, as in test runs using parallel_rspec.
However, it might be nice to be able to options for cases allowed where atomic file deletion cannot be safely.

[AlexWayfer]

@AlexWayfer Yes, I also certainly don't think they are strictly equivalent. The reason for adding this cop is that these can cause problems that are difficult to reproduce, especially when frequent file operations are performed in parallel, as in test runs using parallel_rspec. However, it might be nice to be able to options for cases allowed where atomic file deletion cannot be safely.

Maybe I'd even preffer a cop which will offense if I have rm_r (without f) without check for existance, to avoid the error.

There are just different approaches. And the current default behavior I consider as very agressive and inaccurate.