Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Lint/NonAtomicFileOperation cop #10696

Merged
merged 1 commit into from Jun 27, 2022

Conversation

ydah
Copy link
Contributor

@ydah ydah commented Jun 6, 2022

This PR is add new Lint/NonAtomicFileOperation cop

Checks for non-atomic file operations.
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)

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • [-] Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@ydah ydah force-pushed the add_unnecessary_file_check branch 2 times, most recently from c2a3cf2 to 270ed56 Compare Jun 17, 2022
@ydah ydah changed the title WIP: Add new Lint/UnnecessaryFileCheck cop WIP: Add new Lint/RedundantFileExistenceCheck cop Jun 17, 2022
@ydah ydah force-pushed the add_unnecessary_file_check branch 2 times, most recently from 2dfe2da to 870f5c6 Compare Jun 17, 2022
@ydah ydah changed the title WIP: Add new Lint/RedundantFileExistenceCheck cop WIP: Add new Lint/RedundantFileExistenceCheck cop Jun 17, 2022
@ydah ydah changed the title WIP: Add new Lint/RedundantFileExistenceCheck cop Add new Lint/RedundantFileExistenceCheck cop Jun 17, 2022
@ydah ydah marked this pull request as ready for review Jun 17, 2022
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 20, 2022

In this case the Redundant check is actually Dangerous if you ask me, so perhaps we can rename the cop accordingly. Something like NonAtomicFileOperation might also be appropriate IMO. @rubocop/rubocop-core what do you think?

P.S. Naming is still hard! :D

@ydah ydah force-pushed the add_unnecessary_file_check branch from 870f5c6 to 5226c6f Compare Jun 22, 2022
@ydah ydah changed the title Add new Lint/RedundantFileExistenceCheck cop Add new Lint/NonAtomicFileOperation cop Jun 22, 2022
@ydah
Copy link
Contributor Author

@ydah ydah commented Jun 22, 2022

Naming always annoys me for a long time...
I feel that NonAtomicFileOperation looks good and I changed it. What do you think?

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 23, 2022

It's fine by me, but I'll leave this open for a couple of days to see if some of the other maintainers comes up with a better name.

It'd be nice if we also mention this in the Ruby style guide, as I doubt many people are aware of this problem.

config/default.yml Outdated Show resolved Hide resolved
@ydah ydah force-pushed the add_unnecessary_file_check branch from 5226c6f to b4e768f Compare Jun 26, 2022
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
```ruby
# 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)
```
@ydah ydah force-pushed the add_unnecessary_file_check branch from b4e768f to 857709b Compare Jun 26, 2022
@bbatsov bbatsov merged commit f9febbe into rubocop:master Jun 27, 2022
29 of 30 checks passed
@mathieujobin
Copy link

@mathieujobin mathieujobin commented Jun 30, 2022

this new cop makes my rubocop crash...
on both 1.30.0 and 1.30.1

image

@koic
Copy link
Member

@koic koic commented Jun 30, 2022

@mathieujobin Can you open a new issue with the bug report template?
https://github.com/rubocop/rubocop/issues/new/choose

@ydah ydah deleted the add_unnecessary_file_check branch Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants