Add suspicious_open_options lint.

[atwam]

changelog: [suspicious_open_options]: Checks for the suspicious use of std::fs::OpenOptions::create() without an explicit OpenOptions::truncate().

create() alone will either create a new file or open an existing file. If the file already exists, it will be overwritten when written to, but the file will not be truncated by default. If less data is written to the file than it already contains, the remainder of the file will remain unchanged, and the end of the file will contain old data.

In most cases, one should either use create_new to ensure the file is created from scratch, or ensure truncate is called so that the truncation behaviour is explicit. truncate(true) will ensure the file is entirely overwritten with new data, whereas truncate(false) will explicitely keep the default behavior.

use std::fs::OpenOptions;

OpenOptions::new().create(true).truncate(true);

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[Alexendoo]

Want to take this one @y21?

I like the idea of the lint, we had a bug from this ourselves - #11505

[atwam]

Agree, was bitten by a similar bug yesterday, which made me realize that this was also the cause for another transient bug we had before (a test which was generating random data of varying length, and would actually fail if data was not fully overwriting the previous version). Upon close inspection, found 10-ish instances of a missing truncate in our codebase.

It currently only checks for the std::fs::OpenOptions path, but some libraries also have similar construct. Not sure how/whether to handle these as well, open to suggestions.

[y21]

I also agree this is a really good lint to have, can't say I wasn't bitten by this 😁. Will take a look soon.

It currently only checks for the std::fs::OpenOptions path, but some libraries also have similar construct. Not sure how/whether to handle these as well, open to suggestions.

IME, we tend to be careful with external crates and usually we try to either do it in a "crate-agnostic" way (e.g., by adding an attribute that any crate can apply to their items, #[clippy::has_significant_drop] is a good example) or just limit it to std. However, tokio might just be fine in this case because there's already existing lints that look for the tokio-equivalent of a problematic use of std functions (unused_io_amount comes to mind), so I would say linting them is fine too, provided tokio's OpenOptions has the same semantics. To do that I imagine we just need to add an OR to the match_type(.., &paths::OPEN_OPTIONS) check to also look for tokio's OpenOptions path.

[y21]

This already looks really good and it's a neat lint to have, just a few things.
Also, one potential false positive I can see is that, since it currently assumes it'll find all options in the .open(..) receiver chain, it will incorrectly lint if the chain is broken up into multiple statements:

let mut opts = OpenOptions::new();
opts.truncate(true);
opts.create(true).open("foo");

I suppose we could fix that by looking for a call to OpenOptions::new() in the receiver chain and only lint if it's in there (in this example it wouldn't be)

[y21]

Maybe we can have a help message here? Something like

if you intended to truncate the file, removing the old file contents, call .truncate(true)
if you intended to overwrite the old file contents and leave the rest as is, call .truncate(false)

Even better if it has a proper suggestion, but just a simple help message works too. For that we could use span_lint_and_then to add help messages to the diagnostic

[atwam]

Great idea. I have changed to add a better help message. I did spend some time trying to add a suggestion, but despite spending an hour on it, I could not make it work. cargo test seems to output the right suggestion, trying to run cargo bless generates the right stuff in the .fixed file, but still fails, and so does cargo test afterwards. I just get a cryptic rustfix failed with exit status: 1.

Happy to hear your suggestions (ahah), or if you feel like uncommenting the one line and making it work...

[y21]

The suggestion looked good to me (I think you removed them in the last commit?). I believe the problem with the test is that we can't generally mix warnings that have suggestions with warnings that don't in the same file.

In this case, uitest recognizes that there are warnings with suggestions, so it creates a .fixed file with those suggestions applied, but the other lines will still emit a warning in the .fixed file, which it then rejects.

We can fix this by moving the tests with a suggestion to its own file (e.g. open_options_fixable.rs, or suspicious_open_options.rs)

[y21]

I like this change, using a map instead of the code duplication with locals 👍

[Alexendoo]

I suppose we could fix that by looking for a call to OpenOptions::new() in the receiver chain and only lint if it's in there (in this example it wouldn't be)

File::options() would also be equivalent to OpenOptions::new()

[bors]

☔ The latest upstream changes (presumably #11694) made this pull request unmergeable. Please resolve the merge conflicts.

[atwam]

Fixed conflicts, and re-added the suggestion (thanks @y21 for your hint, splitting the tests into two files indeed worked).

As for also matching File::options() or a multi-statement calls on OpenOptions, I have to admit I am not well versed enough in clippy's matching to know how to do that. There is this promising for_each_local_use_after_expr, but I don't know how to fit this here.

[y21]

As for also matching File::options() or a multi-statement calls on OpenOptions, I have to admit I am not well versed enough in clippy's matching to know how to do that. There is this promising for_each_local_use_after_expr, but I don't know how to fit this here.

The easiest approach here would be to simply avoid linting in case of opts.create(true).open("foo"). That is, if the "last" receiver of the method call chain (opts) is not a call to OpenOptions::new() or File::options().
This will have false negatives, but it will avoid the false positives.

I think this would be totally fine for the initial implementation, and improving on this could be made (by anyone) in followup PRs, though of course you can also try to improve it here if you want. I'm not sure how common it is to build up an OpenOptions across statements, so these FNs might not really be so bad.

One way we could implement this is by having the get_open_options function return a bool, indicating if the last receiver was a valid function call.
Since this function is recursive, the argument expression will eventually be either File::options()/OpenOptions::new(), or something unknown like opts.

So we would need to add an else if branch here to look for ExprKind::Call(callee, _). The callee should be an ExprKind::Path, that QPath can be passed to cx.qpath_res(qpath).opt_def_id() to get the DefId of the callee, then we could use match_any_def_paths to check if one of these specific functions was called. If it was, then we return true.

Then we also need a catch-all else that returns false for all other kinds of unknown expressions.

Pseudo Rust code

fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: ...) -> bool {

  if let ExprKind::MethodCall(path, receiver, arguments, span) = argument.kind {
    // ... old code ...

    get_open_options(cx, receiver, options)
  } else if let ExprKind::Call(callee, _) = argument.kind
      && let ExprKind::Path(path) = callee.kind
      && let Some(did) = cx.qpath_res(path, callee.hir_id).opt_def_id()
  {
    match_any_def_paths(cx, did, &[&paths::OPEN_OPTIONS_NEW, &paths::FILE_OPTIONS]).is_some()
  } else {
    false
  }
}

We could then use this bool in check_open_options to avoid linting suspicious_open_options if said bool is false.
The existing nonsensical_open_options lint doesn't look sensitive to this false positive, since it doesn't rely on a method not having been called, so that could continue linting as before.

[bors]

☔ The latest upstream changes (presumably #11823) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

Hi @atwam 👋 , I'm currently going through my old PRs, do you need help with this?

[atwam]

Hum, I could fixthe conflicts, but I'm afraid your latest suggestions go beyond my knowledge of the lib, and I'm a bit under the water, time-wise, to spend enough time to understand and implement your suggestions.
Happy to solve current conflicts if you want to consider I integrating as-is.

[y21]

Hum, I could fixthe conflicts, but I'm afraid your latest suggestions go beyond my knowledge of the lib, and I'm a bit under the water, time-wise, to spend enough time to understand and implement your suggestions.

Ok, no worries! Maybe what we can do is, you could solve the conflicts and I can submit a PR against your fork with the FP fixed so that the fix gets picked up by this PR?

[atwam]

A'ight @y21, I've tried fixing stuff... it's not done yet.

• Conflicts were solved. There are a bunch of new ineffective_open_options checks, which were now raising issues. These should be fine by now. Made me realize that there was no point raising an issue on create(true).append(true), where the intent is clear.
• I have partially implemented your suggestion, but then it's a half-solution. Again, I feel like I don't know the various bits enough to properly understand how matching is/could be done. On top of that, there's the issue that only tokio still has a Path for checking, fs::OpenOptions is now a type diagnostic item, not sure how to test for that.

So take the current version (which tests fine, but has some commented out code that clippy complained about) as needing... much polishing.
In particular, we should be able to eventually uncomment the expected error line in tests/ui/open_options.rs:32.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• 2328d98

[y21]

Looks good to me! Great work! CC @Alexendoo

[Alexendoo]

Thanks! @bors r=y21

[bors]

📌 Commit f09cd88 has been approved by y21

It is now in the queue for this repository.

[bors]

⌛ Testing commit f09cd88 with merge e248bd3...

[bors]

💔 Test failed - checks-action_test

[y21]

This looks like https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Sporadic.20macos.20failures again?

[Alexendoo]

Let's see, @bors retry

[bors]

⌛ Testing commit f09cd88 with merge 5f3a060...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 5f3a060 to master...