Skip to content

Fix false positive in suspicious_open_options and make paths work #1

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

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

y21
Copy link

@y21 y21 commented Jan 13, 2024

Implements the last few things I had in mind for rust-clippy#11608

This already looked pretty good. The fix for the false positive I had mentioned was basically 90% done. Only the matching needed to be fixed and some paths were incorrect.

Summarising my changes:

  • A few touch ups in the documentation, like adding inline codeblocks for code snippets and adding linebreaks where the text was a bit too "dense"
  • Making the documentation example no_run. At some point in the last months (after this branched off master) a change was made that changed the default test mode to no_run, because almost all of the doc tests don't need to be run (only compiled).
  • Updating paths (and reordering them a bit in alphabetic order).
    • This was the reason why it didn't detect tokio items. The re-exported path is tokio::fs::OpenOptions, but clippy's path methods compare them to the path where they're defined. The real definition path is tokio::fs::open_options::OpenOptions.
  • Bail on unknown trait method calls when collecting options (see the last example for a potential false positive)
  • Make the logic checking of the final OpenOptions::new() call in the chain work
  • Splitting up the help messages, so that there's only one sentence per help

Regarding

In particular, we should be able to eventually uncomment the expected error line in tests/ui/open_options.rs:32

This is a case where it's ok to lint, but we currently aren't because it requires tracking calls across statements. I think it's doable, but imo we could/should land it as is with that false negative and fix that as a followup.
In particular, we'd need to be careful here because of control flow. For example:

options.read(true);
if condition {
  options.read(false);
}

... shouldn't be linted since that conditionally overwrites it (and a few other cases).

@y21
Copy link
Author

y21 commented Jan 15, 2024

( @atwam I don't know if you get notifications on this repo so I'm pinging you, just in case so it doesn't go unnoticed :) )

Copy link
Owner

@atwam atwam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for cleaning up my code and fixing the remaining issues with it!.

@atwam atwam merged commit 2328d98 into atwam:suspicious-open-options Jan 15, 2024
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.

2 participants