new lint: unnecessary_path_exists#17331
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @samueltardieu (or someone else) some time within the next two weeks. 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 (
Why was this reviewer chosen?The reviewer was selected based on:
|
| let ty = cx.typeck_results().expr_ty(recv).peel_refs(); | ||
| if matches!(ty.opt_diag_name(cx), Some(sym::Path | sym::PathBuf)) { |
There was a problem hiding this comment.
You should rather check whether the method that is actually called is Path::exists, it would work with any type dereferencing to Path (including PathBuf).
| } | ||
|
|
||
| let stmts = block.stmts; | ||
| for (idx, stmt) in stmts.iter().enumerate() { |
There was a problem hiding this comment.
You could also zip block.stmts.iter() with block.smts.iter().skip(1).
| ) -> Option<Span> { | ||
| match expr.kind { | ||
| ExprKind::MethodCall(method_seg, recv, _, _) => { | ||
| if FS_METHODS.contains(&method_seg.ident.name.as_str()) { |
There was a problem hiding this comment.
Compare symbols, not strings.
| ExprKind::MethodCall(method_seg, recv, _, _) => { | ||
| if FS_METHODS.contains(&method_seg.ident.name.as_str()) { | ||
| let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs(); | ||
| if matches!(recv_ty.opt_diag_name(cx), Some(sym::Path | sym::PathBuf)) |
There was a problem hiding this comment.
Same comment as above: check if this is a method belonging to Path.
| @@ -0,0 +1,169 @@ | |||
| #![warn(clippy::unnecessary_path_exists)] | |||
| #![allow(unused)] | |||
There was a problem hiding this comment.
| #![allow(unused)] |
|
Reminder, once the PR becomes ready for a review, use |
|
Thanks for your valuable feedback! I've made the requested changes in the latest commit |
|
@rustbot review |
| /// if b { ... }`), only detects when the `if` immediately follows the `let`. | ||
| #[clippy::version = "1.98.0"] | ||
| pub UNNECESSARY_PATH_EXISTS, | ||
| nursery, |
There was a problem hiding this comment.
You need to choose a category, nursery is where lints go to die nowadays.
| return; | ||
| } | ||
|
|
||
| for (stmt, next_stmt) in block.stmts.iter().zip(block.stmts.iter().skip(1)) { |
There was a problem hiding this comment.
Rather than checking each and every block, it probably would be more efficient to first check for a call to the .exists() method and then analyze the surrounding block. This lint should probably be moved into the methods directory.
Fixes #17158
Adds
unnecessary_path_exists(nursery) which detects calls toPath::existsused as an
ifcondition immediately before a filesystem operation on the samepath, creating a TOCTOU race condition. The subsequent operation's return value
already indicates whether the path exists, making the
exists()check redundant.Detected patterns:
if path.exists() { path.metadata() }if path.exists() { ... } else { ... }if path.exists() && other { ... }let b = path.exists(); if b { ... }Detected fs operations:
metadata,symlink_metadata,canonicalize,read_link,read_dir,is_file,is_dir,is_symlink.changelog: new lint: [
unnecessary_path_exists]