From da522a0610cae3fc0fcca06040071f3a1f5cf1e3 Mon Sep 17 00:00:00 2001 From: David Hernandez Date: Tue, 30 Jun 2026 23:54:23 -0700 Subject: [PATCH 1/5] add unnecessary_path_exists lint to detect TOCTOU after Path::exists --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unnecessary_path_exists.rs | 232 +++++++++++++++++++ tests/ui/unnecessary_path_exists.rs | 169 ++++++++++++++ tests/ui/unnecessary_path_exists.stderr | 238 ++++++++++++++++++++ 6 files changed, 643 insertions(+) create mode 100644 clippy_lints/src/unnecessary_path_exists.rs create mode 100644 tests/ui/unnecessary_path_exists.rs create mode 100644 tests/ui/unnecessary_path_exists.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 689d56d0fb7c..b34ba15417b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7449,6 +7449,7 @@ Released 2018-09-13 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation [`unnecessary_option_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_option_map_or_else [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings +[`unnecessary_path_exists`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_path_exists [`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 51a848d022d8..2635f7793b6f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -780,6 +780,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_mut_passed::UNNECESSARY_MUT_PASSED_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, + crate::unnecessary_path_exists::UNNECESSARY_PATH_EXISTS_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, crate::unnecessary_semicolon::UNNECESSARY_SEMICOLON_INFO, crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0514c86d0bec..810d624ebbbe 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -377,6 +377,7 @@ mod unnecessary_literal_bound; mod unnecessary_map_on_constructor; mod unnecessary_mut_passed; mod unnecessary_owned_empty_strings; +mod unnecessary_path_exists; mod unnecessary_self_imports; mod unnecessary_semicolon; mod unnecessary_struct_initialization; @@ -862,6 +863,7 @@ rustc_lint::late_lint_methods!( ManualAssertEq: manual_assert_eq::ManualAssertEq = manual_assert_eq::ManualAssertEq, WithCapacityZero: with_capacity_zero::WithCapacityZero = with_capacity_zero::WithCapacityZero, RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, + UnnecessaryPathExists: unnecessary_path_exists::UnnecessaryPathExists = unnecessary_path_exists::UnnecessaryPathExists, // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_lints/src/unnecessary_path_exists.rs b/clippy_lints/src/unnecessary_path_exists.rs new file mode 100644 index 000000000000..417498de5732 --- /dev/null +++ b/clippy_lints/src/unnecessary_path_exists.rs @@ -0,0 +1,232 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::res::MaybeDef; +use clippy_utils::{SpanlessEq, higher, path_to_local_with_projections, sym}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, PatKind, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::{Span, SyntaxContext}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `Path::exists` immediately before a filesystem + /// operation on the same path. + /// + /// ### Why is this bad? + /// Calling `exists()` and then performing a filesystem operation on the same + /// path is a classic Time-Of-Check to Time-Of-Use (TOCTOU) race condition. + /// Between the two calls another process can add, remove, or replace the + /// file, making the result of `exists()` stale. The filesystem operation + /// itself will indicate whether the path exists via its return value, making + /// the prior `exists()` check both redundant and dangerous. + /// + /// ### Example + /// ```rust,no_run + /// # use std::path::Path; + /// # fn example(path: &Path) { + /// if path.exists() { + /// let metadata = path.metadata().unwrap(); + /// // use metadata ... + /// } + /// # } + /// ``` + /// Use instead: + /// ```rust,no_run + /// # use std::path::Path; + /// # fn example(path: &Path) { + /// if let Ok(metadata) = path.metadata() { + /// // use metadata ... + /// } + /// # } + /// ``` + /// + /// ### Known problems + /// - Does not detect `std::fs` free functions used inside the block + /// (e.g. `fs::read(path)`, `fs::File::open(path)`), only method calls on + /// the path receiver itself. + /// - Does not detect `Path::try_exists()` (stabilized in Rust 1.63): the `?` + /// operator in the condition desugars to a `Match` node, so the condition + /// is not seen as a simple `.exists()` call. + /// - For the stored-bool variant (`let b = path.exists(); /* other stmts */; + /// if b { ... }`), only detects when the `if` immediately follows the `let`. + #[clippy::version = "1.98.0"] + pub UNNECESSARY_PATH_EXISTS, + nursery, + "calling `Path::exists` before a filesystem operation creates a TOCTOU race" +} + +declare_lint_pass!(UnnecessaryPathExists => [UNNECESSARY_PATH_EXISTS]); + +/// `Path`/`PathBuf` methods that each initiate a fresh syscall. +const FS_METHODS: &[&str] = &[ + "canonicalize", + "is_dir", + "is_file", + "is_symlink", + "metadata", + "read_dir", + "read_link", + "symlink_metadata", +]; + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryPathExists { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + + if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr) + && let Some((path_recv, exists_span)) = extract_exists_receiver(cx, cond) + && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, expr.span.ctxt()) + { + emit_lint(cx, exists_span, fs_call_span); + } + } + + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { + if block.span.from_expansion() { + return; + } + + let stmts = block.stmts; + for (idx, stmt) in stmts.iter().enumerate() { + // Match `let b = path.exists()` + let StmtKind::Let(local) = stmt.kind else { + continue; + }; + let Some(init) = local.init else { continue }; + let Some((path_recv, exists_span)) = extract_exists_receiver(cx, init) else { + continue; + }; + // Simple binding pattern: `let b = ...` (not destructuring) + let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else { + continue; + }; + + // Find the immediately following expression + let next_expr = if idx + 1 < stmts.len() { + match stmts[idx + 1].kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => e, + _ => continue, + } + } else if let Some(e) = block.expr { + e + } else { + continue; + }; + + if let Some(higher::If { cond, then, .. }) = higher::If::hir(next_expr) + && path_to_local_with_projections(cond) == Some(binding_id) + && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, next_expr.span.ctxt()) + { + emit_lint(cx, exists_span, fs_call_span); + } + } + } +} + +fn emit_lint(cx: &LateContext<'_>, exists_span: Span, fs_call_span: Span) { + span_lint_and_then( + cx, + UNNECESSARY_PATH_EXISTS, + exists_span, + "unnecessary `Path::exists` before a filesystem operation on the same path", + |diag| { + diag.span_note(fs_call_span, "the filesystem operation is here"); + diag.help( + "the `exists()` check is redundant and creates a TOCTOU race condition; \ + consider removing it and handling the error from the filesystem operation directly", + ); + }, + ); +} + +/// Walks a condition expression to find a `.exists()` call on a `Path`/`PathBuf` +/// receiver. Returns the receiver expression and the span of the `.exists()` call. +/// Recurses through `&&` chains so compound conditions are handled. +fn extract_exists_receiver<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, Span)> { + match expr.kind { + ExprKind::MethodCall(seg, recv, [], _) + if seg.ident.name.as_str() == "exists" && !expr.span.from_expansion() => + { + let ty = cx.typeck_results().expr_ty(recv).peel_refs(); + if ty.is_diag_item(cx, sym::Path) || ty.is_diag_item(cx, sym::PathBuf) { + Some((recv, expr.span)) + } else { + None + } + }, + ExprKind::Binary(op, lhs, rhs) if op.node == BinOpKind::And => { + extract_exists_receiver(cx, lhs).or_else(|| extract_exists_receiver(cx, rhs)) + }, + _ => None, + } +} + +/// Searches the `then` block of the `if` for the first filesystem method call +/// on the same receiver as the `exists()` check. +fn find_fs_call<'tcx>( + cx: &LateContext<'tcx>, + then: &'tcx Expr<'tcx>, + path_recv: &'tcx Expr<'tcx>, + ctxt: SyntaxContext, +) -> Option { + let ExprKind::Block(block, _) = then.kind else { + return None; + }; + for stmt in block.stmts { + if let Some(span) = find_fs_call_in_stmt(cx, stmt, path_recv, ctxt) { + return Some(span); + } + } + block.expr.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt)) +} + +fn find_fs_call_in_stmt<'tcx>( + cx: &LateContext<'tcx>, + stmt: &'tcx Stmt<'tcx>, + path_recv: &'tcx Expr<'tcx>, + ctxt: SyntaxContext, +) -> Option { + match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => find_fs_call_in_expr(cx, e, path_recv, ctxt), + StmtKind::Let(local) => local + .init + .and_then(|init| find_fs_call_in_expr(cx, init, path_recv, ctxt)), + StmtKind::Item(_) => None, + } +} + +/// Peels through method chains (e.g. `.metadata().unwrap()`) and the `?` operator +/// desugaring to find a filesystem method call on `path_recv`. +fn find_fs_call_in_expr<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + path_recv: &'tcx Expr<'tcx>, + ctxt: SyntaxContext, +) -> Option { + match expr.kind { + 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 (recv_ty.is_diag_item(cx, sym::Path) || recv_ty.is_diag_item(cx, sym::PathBuf)) + && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) + { + return Some(expr.span); + } + } + // Peel through chains like `.metadata().unwrap()` or `.metadata().ok()` + find_fs_call_in_expr(cx, recv, path_recv, ctxt) + }, + // The `?` operator desugars to: + // Match(Call(TryTraitBranch, [inner_expr]), ..., TryDesugar) + // so we extract `inner_expr` and keep searching. + ExprKind::Match(scrutinee, _, MatchSource::TryDesugar(_)) => { + if let ExprKind::Call(_, [inner_expr]) = scrutinee.kind { + find_fs_call_in_expr(cx, inner_expr, path_recv, ctxt) + } else { + None + } + }, + _ => None, + } +} diff --git a/tests/ui/unnecessary_path_exists.rs b/tests/ui/unnecessary_path_exists.rs new file mode 100644 index 000000000000..73cfbc2beffa --- /dev/null +++ b/tests/ui/unnecessary_path_exists.rs @@ -0,0 +1,169 @@ +#![warn(clippy::unnecessary_path_exists)] +#![allow(unused)] + +use std::path::{Path, PathBuf}; + +fn check_path(path: &Path) { + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.is_file(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.is_dir(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.is_symlink(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.canonicalize().unwrap(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.read_dir().unwrap(); + } + + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.symlink_metadata().unwrap(); + } + + // has an else branch — TOCTOU race still present + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } else { + println!("path does not exist"); + } + + // no filesystem operation in body — no lint + if path.exists() { + println!("path exists"); + } +} + +fn check_pathbuf(path: PathBuf) { + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } +} + +fn check_different_receiver(path: &Path, other: &Path) { + // different receiver — no lint + if path.exists() { + let _ = other.metadata().unwrap(); + } +} + +fn check_with_result(path: &Path) -> std::io::Result<()> { + // ? operator + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata()?; + } + Ok(()) +} + +fn check_statement_forms(path: &Path) { + // no let binding + if path.exists() { + //~^ unnecessary_path_exists + path.metadata().ok(); + } + + // fs op not the first statement + if path.exists() { + //~^ unnecessary_path_exists + println!("checking path"); + let _ = path.metadata().unwrap(); + } + + // deeper method chain + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().ok().is_some(); + } +} + +fn check_compound_condition(path: &Path) { + let condition = true; + + // exists() on the left side of && + if path.exists() && condition { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } + + // exists() on the right side of && + if condition && path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } +} + +fn check_stored_bool(path: &Path) { + // stored bool, immediately followed by if + let exists = path.exists(); + //~^ unnecessary_path_exists + if exists { + let _ = path.metadata().unwrap(); + } + + // stored bool with PathBuf + let path2 = PathBuf::from("test"); + let exists2 = path2.exists(); + //~^ unnecessary_path_exists + if exists2 { + let _ = path2.metadata().unwrap(); + } + + // stored bool with else branch + let exists3 = path.exists(); + //~^ unnecessary_path_exists + if exists3 { + let _ = path.metadata().unwrap(); + } else { + println!("not found"); + } + + // stored bool with compound condition — no lint (condition is not the plain local) + let exists4 = path.exists(); + if exists4 && true { + let _ = path.metadata().unwrap(); + } +} + +fn check_stored_bool_not_immediate(path: &Path) { + // intervening statement — do not lint + let exists = path.exists(); + println!("something in between"); + if exists { + let _ = path.metadata().unwrap(); + } +} + +fn check_false_positives(path: &Path) { + // non-fs method — no lint + if path.exists() { + let _ = path.display().to_string(); + } + + // free function call, not a method on the receiver — no lint + if path.exists() { + let _ = std::fs::read(path); + } +} + +fn main() {} diff --git a/tests/ui/unnecessary_path_exists.stderr b/tests/ui/unnecessary_path_exists.stderr new file mode 100644 index 000000000000..82c050991742 --- /dev/null +++ b/tests/ui/unnecessary_path_exists.stderr @@ -0,0 +1,238 @@ +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:7:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:9:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + = note: `-D clippy::unnecessary-path-exists` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_path_exists)]` + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:12:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:14:17 + | +LL | let _ = path.is_file(); + | ^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:17:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:19:17 + | +LL | let _ = path.is_dir(); + | ^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:22:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:24:17 + | +LL | let _ = path.is_symlink(); + | ^^^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:27:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:29:17 + | +LL | let _ = path.canonicalize().unwrap(); + | ^^^^^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:32:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:34:17 + | +LL | let _ = path.read_dir().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:37:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:39:17 + | +LL | let _ = path.symlink_metadata().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:43:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:45:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:57:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:59:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:72:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:74:17 + | +LL | let _ = path.metadata()?; + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:81:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:83:9 + | +LL | path.metadata().ok(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:87:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:90:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:94:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:96:17 + | +LL | let _ = path.metadata().ok().is_some(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:104:8 + | +LL | if path.exists() && condition { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:106:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:110:21 + | +LL | if condition && path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:112:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:118:18 + | +LL | let exists = path.exists(); + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:121:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:126:19 + | +LL | let exists2 = path2.exists(); + | ^^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:129:17 + | +LL | let _ = path2.metadata().unwrap(); + | ^^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:133:19 + | +LL | let exists3 = path.exists(); + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:136:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: aborting due to 18 previous errors + From e090028dc376794daf7a42054254305dc3b9dc8c Mon Sep 17 00:00:00 2001 From: David Hernandez Date: Wed, 1 Jul 2026 01:14:23 -0700 Subject: [PATCH 2/5] fix dogfood: use sym::exists and opt_diag_name --- clippy_lints/src/unnecessary_path_exists.rs | 6 +++--- clippy_utils/src/sym.rs | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/unnecessary_path_exists.rs b/clippy_lints/src/unnecessary_path_exists.rs index 417498de5732..8f44e7d86e61 100644 --- a/clippy_lints/src/unnecessary_path_exists.rs +++ b/clippy_lints/src/unnecessary_path_exists.rs @@ -146,10 +146,10 @@ fn emit_lint(cx: &LateContext<'_>, exists_span: Span, fs_call_span: Span) { fn extract_exists_receiver<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, Span)> { match expr.kind { ExprKind::MethodCall(seg, recv, [], _) - if seg.ident.name.as_str() == "exists" && !expr.span.from_expansion() => + if seg.ident.name == sym::exists && !expr.span.from_expansion() => { let ty = cx.typeck_results().expr_ty(recv).peel_refs(); - if ty.is_diag_item(cx, sym::Path) || ty.is_diag_item(cx, sym::PathBuf) { + if matches!(ty.opt_diag_name(cx), Some(sym::Path | sym::PathBuf)) { Some((recv, expr.span)) } else { None @@ -208,7 +208,7 @@ fn find_fs_call_in_expr<'tcx>( 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 (recv_ty.is_diag_item(cx, sym::Path) || recv_ty.is_diag_item(cx, sym::PathBuf)) + if matches!(recv_ty.opt_diag_name(cx), Some(sym::Path | sym::PathBuf)) && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) { return Some(expr.span); diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 6053788c82b1..2249cfc89708 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -222,6 +222,7 @@ generate! { eprint_macro, eprintln_macro, err, + exists, exp, expect_err, expn_data, From 91865da1a3f5c1395b40ac6c879d1f1362c4186e Mon Sep 17 00:00:00 2001 From: David Hernandez Date: Wed, 1 Jul 2026 16:34:01 -0700 Subject: [PATCH 3/5] address review: use type_dependent_def_id, symbol comparison, zip pairs --- clippy_lints/src/unnecessary_path_exists.rs | 127 +++++++++++--------- clippy_utils/src/sym.rs | 7 ++ 2 files changed, 74 insertions(+), 60 deletions(-) diff --git a/clippy_lints/src/unnecessary_path_exists.rs b/clippy_lints/src/unnecessary_path_exists.rs index 8f44e7d86e61..af23fe1b7d83 100644 --- a/clippy_lints/src/unnecessary_path_exists.rs +++ b/clippy_lints/src/unnecessary_path_exists.rs @@ -4,6 +4,7 @@ use clippy_utils::{SpanlessEq, higher, path_to_local_with_projections, sym}; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, PatKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; +use rustc_span::symbol::Symbol; use rustc_span::{Span, SyntaxContext}; declare_clippy_lint! { @@ -56,18 +57,6 @@ declare_clippy_lint! { declare_lint_pass!(UnnecessaryPathExists => [UNNECESSARY_PATH_EXISTS]); -/// `Path`/`PathBuf` methods that each initiate a fresh syscall. -const FS_METHODS: &[&str] = &[ - "canonicalize", - "is_dir", - "is_file", - "is_symlink", - "metadata", - "read_dir", - "read_link", - "symlink_metadata", -]; - impl<'tcx> LateLintPass<'tcx> for UnnecessaryPathExists { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { if expr.span.from_expansion() { @@ -87,40 +76,36 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryPathExists { return; } - let stmts = block.stmts; - for (idx, stmt) in stmts.iter().enumerate() { - // Match `let b = path.exists()` - let StmtKind::Let(local) = stmt.kind else { - continue; - }; - let Some(init) = local.init else { continue }; - let Some((path_recv, exists_span)) = extract_exists_receiver(cx, init) else { - continue; - }; - // Simple binding pattern: `let b = ...` (not destructuring) - let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else { - continue; - }; - - // Find the immediately following expression - let next_expr = if idx + 1 < stmts.len() { - match stmts[idx + 1].kind { - StmtKind::Expr(e) | StmtKind::Semi(e) => e, - _ => continue, - } - } else if let Some(e) = block.expr { - e - } else { - continue; - }; - - if let Some(higher::If { cond, then, .. }) = higher::If::hir(next_expr) - && path_to_local_with_projections(cond) == Some(binding_id) - && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, next_expr.span.ctxt()) - { - emit_lint(cx, exists_span, fs_call_span); + for (stmt, next_stmt) in block.stmts.iter().zip(block.stmts.iter().skip(1)) { + if let StmtKind::Expr(next_expr) | StmtKind::Semi(next_expr) = next_stmt.kind { + check_stored_bool_pair(cx, stmt, next_expr); } } + if let Some(last_stmt) = block.stmts.last() + && let Some(next_expr) = block.expr + { + check_stored_bool_pair(cx, last_stmt, next_expr); + } + } +} + +fn check_stored_bool_pair<'tcx>(cx: &LateContext<'tcx>, let_stmt: &'tcx Stmt<'tcx>, next_expr: &'tcx Expr<'tcx>) { + let StmtKind::Let(local) = let_stmt.kind else { + return; + }; + let Some(init) = local.init else { return }; + let Some((path_recv, exists_span)) = extract_exists_receiver(cx, init) else { + return; + }; + let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else { + return; + }; + + if let Some(higher::If { cond, then, .. }) = higher::If::hir(next_expr) + && path_to_local_with_projections(cond) == Some(binding_id) + && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, next_expr.span.ctxt()) + { + emit_lint(cx, exists_span, fs_call_span); } } @@ -140,20 +125,44 @@ fn emit_lint(cx: &LateContext<'_>, exists_span: Span, fs_call_span: Span) { ); } -/// Walks a condition expression to find a `.exists()` call on a `Path`/`PathBuf` -/// receiver. Returns the receiver expression and the span of the `.exists()` call. +/// Returns `true` if `expr` is a method call that resolves to a method defined +/// on `std::path::Path` (handles any type that derefs to `Path`, e.g. `PathBuf`). +fn is_path_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + cx.typeck_results() + .type_dependent_def_id(expr.hir_id) + .is_some_and(|def_id| { + let parent = cx.tcx.parent(def_id); + cx.tcx + .type_of(parent) + .instantiate_identity() + .skip_norm_wip() + .is_diag_item(cx, sym::Path) + }) +} + +fn is_fs_method_name(name: Symbol) -> bool { + matches!( + name, + sym::canonicalize + | sym::is_dir + | sym::is_file + | sym::is_symlink + | sym::metadata + | sym::read_dir + | sym::read_link + | sym::symlink_metadata + ) +} + +/// Walks a condition expression to find a `Path::exists` call. +/// Returns the receiver expression and the span of the `.exists()` call. /// Recurses through `&&` chains so compound conditions are handled. fn extract_exists_receiver<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, Span)> { match expr.kind { ExprKind::MethodCall(seg, recv, [], _) - if seg.ident.name == sym::exists && !expr.span.from_expansion() => + if seg.ident.name == sym::exists && !expr.span.from_expansion() && is_path_method_call(cx, expr) => { - let ty = cx.typeck_results().expr_ty(recv).peel_refs(); - if matches!(ty.opt_diag_name(cx), Some(sym::Path | sym::PathBuf)) { - Some((recv, expr.span)) - } else { - None - } + Some((recv, expr.span)) }, ExprKind::Binary(op, lhs, rhs) if op.node == BinOpKind::And => { extract_exists_receiver(cx, lhs).or_else(|| extract_exists_receiver(cx, rhs)) @@ -206,13 +215,11 @@ fn find_fs_call_in_expr<'tcx>( ) -> Option { match expr.kind { 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)) - && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) - { - return Some(expr.span); - } + if is_fs_method_name(method_seg.ident.name) + && is_path_method_call(cx, expr) + && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) + { + return Some(expr.span); } // Peel through chains like `.metadata().unwrap()` or `.metadata().ok()` find_fs_call_in_expr(cx, recv, path_recv, ctxt) diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 2249cfc89708..8dd1756e4876 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -155,6 +155,7 @@ generate! { build_hasher, by_ref, bytes, + canonicalize, capacity, cargo_clippy: "cargo-clippy", cast, @@ -376,6 +377,7 @@ generate! { is_diag_item, is_diagnostic_item, is_digit, + is_dir, is_empty, is_err, is_file, @@ -386,6 +388,7 @@ generate! { is_some, is_some_and, is_sorted_by_key, + is_symlink, isize_legacy_const_max, isize_legacy_const_min, isize_legacy_fn_max_value, @@ -427,6 +430,7 @@ generate! { mem_replace, mem_size_of, mem_size_of_val, + metadata, min, min_by, min_by_key, @@ -497,8 +501,10 @@ generate! { push_str, range_step, read, + read_dir, read_exact, read_line, + read_link, read_to_end, read_to_string, read_unaligned, @@ -593,6 +599,7 @@ generate! { subsec_nanos, sum, symbol, + symlink_metadata, take, take_while, tcx, From b1012b9f29f3fe584ec6555c751b97b7a3b78b32 Mon Sep 17 00:00:00 2001 From: David Hernandez Date: Mon, 6 Jul 2026 19:27:53 -0700 Subject: [PATCH 4/5] address review: move unnecessary_path_exists to methods/, use suspicious category Dispatches from the existing method-call visitor instead of a standalone pass that scanned every expr/block, and adds try_exists() support via the `?` operator's TryDesugar match arm. --- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/methods/mod.rs | 53 ++++ .../src/methods/unnecessary_path_exists.rs | 211 ++++++++++++++++ clippy_lints/src/unnecessary_path_exists.rs | 239 ------------------ clippy_utils/src/sym.rs | 1 + tests/ui/unnecessary_path_exists.rs | 48 +++- tests/ui/unnecessary_path_exists.stderr | 113 ++++++--- 8 files changed, 389 insertions(+), 280 deletions(-) create mode 100644 clippy_lints/src/methods/unnecessary_path_exists.rs delete mode 100644 clippy_lints/src/unnecessary_path_exists.rs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2635f7793b6f..714a677d5c87 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -506,6 +506,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::UNNECESSARY_MAP_OR_INFO, crate::methods::UNNECESSARY_MIN_OR_MAX_INFO, crate::methods::UNNECESSARY_OPTION_MAP_OR_ELSE_INFO, + crate::methods::UNNECESSARY_PATH_EXISTS_INFO, crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO, crate::methods::UNNECESSARY_SORT_BY_INFO, crate::methods::UNNECESSARY_TO_OWNED_INFO, @@ -780,7 +781,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_mut_passed::UNNECESSARY_MUT_PASSED_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, - crate::unnecessary_path_exists::UNNECESSARY_PATH_EXISTS_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, crate::unnecessary_semicolon::UNNECESSARY_SEMICOLON_INFO, crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 810d624ebbbe..0514c86d0bec 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -377,7 +377,6 @@ mod unnecessary_literal_bound; mod unnecessary_map_on_constructor; mod unnecessary_mut_passed; mod unnecessary_owned_empty_strings; -mod unnecessary_path_exists; mod unnecessary_self_imports; mod unnecessary_semicolon; mod unnecessary_struct_initialization; @@ -863,7 +862,6 @@ rustc_lint::late_lint_methods!( ManualAssertEq: manual_assert_eq::ManualAssertEq = manual_assert_eq::ManualAssertEq, WithCapacityZero: with_capacity_zero::WithCapacityZero = with_capacity_zero::WithCapacityZero, RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, - UnnecessaryPathExists: unnecessary_path_exists::UnnecessaryPathExists = unnecessary_path_exists::UnnecessaryPathExists, // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 38a3b1705ad1..6b650576e063 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -141,6 +141,7 @@ mod unnecessary_literal_unwrap; mod unnecessary_map_or; mod unnecessary_map_or_else; mod unnecessary_min_or_max; +mod unnecessary_path_exists; mod unnecessary_sort_by; mod unnecessary_to_owned; mod unnecessary_unwrap_unchecked; @@ -4513,6 +4514,54 @@ declare_clippy_lint! { "making no use of the \"map closure\" when calling `.map_or_else(|| 2 * k, |n| n)`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `Path::exists` immediately before a filesystem + /// operation on the same path. + /// + /// ### Why is this bad? + /// Calling `exists()` and then performing a filesystem operation on the same + /// path is a classic Time-Of-Check to Time-Of-Use (TOCTOU) race condition. + /// Between the two calls another process can add, remove, or replace the + /// file, making the result of `exists()` stale. The filesystem operation + /// itself will indicate whether the path exists via its return value, making + /// the prior `exists()` check both redundant and dangerous. + /// + /// ### Example + /// ```rust,no_run + /// # use std::path::Path; + /// # fn example(path: &Path) { + /// if path.exists() { + /// let metadata = path.metadata().unwrap(); + /// // use metadata ... + /// } + /// # } + /// ``` + /// Use instead: + /// ```rust,no_run + /// # use std::path::Path; + /// # fn example(path: &Path) { + /// if let Ok(metadata) = path.metadata() { + /// // use metadata ... + /// } + /// # } + /// ``` + /// + /// ### Known problems + /// - Does not detect `std::fs` free functions used inside the block + /// (e.g. `fs::read(path)`, `fs::File::open(path)`), only method calls on + /// the path receiver itself. + /// - `Path::try_exists()` (stabilized in Rust 1.63) is only detected when + /// used with the `?` operator (e.g. `if path.try_exists()? { ... }`); + /// `.unwrap()`/`.unwrap_or(..)` and similar are not recognized. + /// - For the stored-bool variant (`let b = path.exists(); /* other stmts */; + /// if b { ... }`), only detects when the `if` immediately follows the `let`. + #[clippy::version = "1.98.0"] + pub UNNECESSARY_PATH_EXISTS, + suspicious, + "calling `Path::exists` before a filesystem operation creates a TOCTOU race" +} + declare_clippy_lint! { /// ### What it does /// Checks for usage of `.map_or_else()` "map closure" for `Result` type. @@ -5068,6 +5117,7 @@ impl_lint_pass!(Methods => [ UNNECESSARY_MAP_OR, UNNECESSARY_MIN_OR_MAX, UNNECESSARY_OPTION_MAP_OR_ELSE, + UNNECESSARY_PATH_EXISTS, UNNECESSARY_RESULT_MAP_OR_ELSE, UNNECESSARY_SORT_BY, UNNECESSARY_TO_OWNED, @@ -5421,6 +5471,9 @@ impl Methods { } path_ends_with_ext::check(cx, recv, arg, expr, self.msrv, &self.allowed_dotfiles); }, + (sym::exists | sym::try_exists, []) => { + unnecessary_path_exists::check(cx, expr, recv); + }, (sym::expect, [_]) => { match method_call(recv) { Some((sym::ok, recv_inner, [], _, _)) => ok_expect::check(cx, expr, recv, recv_inner), diff --git a/clippy_lints/src/methods/unnecessary_path_exists.rs b/clippy_lints/src/methods/unnecessary_path_exists.rs new file mode 100644 index 000000000000..22dd8f0c907a --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_path_exists.rs @@ -0,0 +1,211 @@ +use super::UNNECESSARY_PATH_EXISTS; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::res::MaybeDef; +use clippy_utils::{SpanlessEq, get_enclosing_block, get_parent_expr, higher, path_to_local_with_projections, sym}; +use rustc_hir::{BinOpKind, Expr, ExprKind, MatchSource, Node, PatKind, StmtKind}; +use rustc_lint::LateContext; +use rustc_span::{Span, SyntaxContext}; + +/// `expr` is a `.exists()` call on `recv`. Find out whether it's used either +/// directly (or through a chain of `&&`) as an `if` condition, or stored in a +/// `let` binding that's immediately checked by the following `if`, and if so +/// look for a redundant filesystem operation in the `then` branch. +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, recv: &'tcx Expr<'tcx>) { + if is_path_method_call(cx, expr) + && let Some((then, ctxt)) = if_then_from_condition(cx, expr).or_else(|| if_then_from_stored_bool(cx, expr)) + && let Some(fs_call_span) = find_fs_call(cx, then, recv, ctxt) + { + span_lint_and_then( + cx, + UNNECESSARY_PATH_EXISTS, + expr.span, + "unnecessary `Path::exists` before a filesystem operation on the same path", + |diag| { + diag.span_note(fs_call_span, "the filesystem operation is here"); + diag.help( + "the `exists()` check is redundant and creates a TOCTOU race condition; \ + consider removing it and handling the error from the filesystem operation directly", + ); + }, + ); + } +} + +/// If `current` is the operand of a `?` operator (i.e. `current?`), returns the +/// `Match` expression that the desugaring produces, so callers can keep +/// climbing from there. `EXPR?` lowers to +/// `Match(Call(, [EXPR]), _, TryDesugar(call_hir_id))`, +/// so this is recognized structurally via `MatchSource::TryDesugar`, not by +/// name/string matching on the call. +fn peel_try_desugar<'tcx>(cx: &LateContext<'tcx>, current: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + let call_expr = get_parent_expr(cx, current)?; + let ExprKind::Call(_, [arg]) = call_expr.kind else { + return None; + }; + if arg.hir_id != current.hir_id { + return None; + } + let match_expr = get_parent_expr(cx, call_expr)?; + if let ExprKind::Match(_, _, MatchSource::TryDesugar(scrutinee_id)) = match_expr.kind + && scrutinee_id == call_expr.hir_id + { + Some(match_expr) + } else { + None + } +} + +/// Repeatedly applies [`peel_try_desugar`], returning the outermost expression +/// once no more `?` layers can be peeled. +fn peel_try_desugars<'tcx>(cx: &LateContext<'tcx>, mut current: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { + while let Some(match_expr) = peel_try_desugar(cx, current) { + current = match_expr; + } + current +} + +/// Climbs through any enclosing `&&` chain (peeling a leading `?`, e.g. from +/// `path.try_exists()?`, first) looking for an enclosing `if` whose condition +/// is exactly the expression we climbed to. +fn if_then_from_condition<'tcx>( + cx: &LateContext<'tcx>, + exists_expr: &'tcx Expr<'tcx>, +) -> Option<(&'tcx Expr<'tcx>, SyntaxContext)> { + let mut current = peel_try_desugars(cx, exists_expr); + loop { + let parent = get_parent_expr(cx, current)?; + match parent.kind { + ExprKind::Binary(op, lhs, rhs) + if op.node == BinOpKind::And && (lhs.hir_id == current.hir_id || rhs.hir_id == current.hir_id) => + { + current = parent; + }, + _ => { + let higher::If { cond, then, .. } = higher::If::hir(parent)?; + return (cond.hir_id == current.hir_id && !parent.span.from_expansion()) + .then(|| (then, parent.span.ctxt())); + }, + } + } +} + +/// Handles `let b = path.exists(); if b { ... }` (or the `try_exists()?` +/// equivalent), where the `if` immediately follows the `let` in the same +/// block. +fn if_then_from_stored_bool<'tcx>( + cx: &LateContext<'tcx>, + exists_expr: &'tcx Expr<'tcx>, +) -> Option<(&'tcx Expr<'tcx>, SyntaxContext)> { + let outer = peel_try_desugars(cx, exists_expr); + let Node::LetStmt(local) = cx.tcx.parent_hir_node(outer.hir_id) else { + return None; + }; + let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else { + return None; + }; + + let block = get_enclosing_block(cx, local.hir_id)?; + if block.span.from_expansion() { + return None; + } + let idx = block + .stmts + .iter() + .position(|stmt| matches!(stmt.kind, StmtKind::Let(l) if l.hir_id == local.hir_id))?; + let next_expr = match block.stmts.get(idx + 1) { + Some(stmt) => match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e), + StmtKind::Let(_) | StmtKind::Item(_) => None, + }, + None => block.expr, + }?; + + let higher::If { cond, then, .. } = higher::If::hir(next_expr)?; + (path_to_local_with_projections(cond) == Some(binding_id)).then(|| (then, next_expr.span.ctxt())) +} + +fn is_fs_method_name(name: rustc_span::Symbol) -> bool { + matches!( + name, + sym::canonicalize + | sym::is_dir + | sym::is_file + | sym::is_symlink + | sym::metadata + | sym::read_dir + | sym::read_link + | sym::symlink_metadata + ) +} + +/// Returns `true` if `expr` is a method call that resolves to a method defined +/// on `std::path::Path` (handles any type that derefs to `Path`, e.g. `PathBuf`). +fn is_path_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + cx.typeck_results() + .type_dependent_def_id(expr.hir_id) + .is_some_and(|def_id| { + let parent = cx.tcx.parent(def_id); + cx.tcx + .type_of(parent) + .instantiate_identity() + .skip_norm_wip() + .is_diag_item(cx, sym::Path) + }) +} + +/// Searches the `then` block of the `if` for the first filesystem method call +/// on the same receiver as the `exists()` check. +fn find_fs_call<'tcx>( + cx: &LateContext<'tcx>, + then: &'tcx Expr<'tcx>, + path_recv: &'tcx Expr<'tcx>, + ctxt: SyntaxContext, +) -> Option { + let ExprKind::Block(block, _) = then.kind else { + return None; + }; + for stmt in block.stmts { + let candidate = match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e), + StmtKind::Let(local) => local.init, + StmtKind::Item(_) => None, + }; + if let Some(span) = candidate.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt)) { + return Some(span); + } + } + block.expr.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt)) +} + +/// Peels through method chains (e.g. `.metadata().unwrap()`) and the `?` operator +/// desugaring to find a filesystem method call on `path_recv`. +fn find_fs_call_in_expr<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + path_recv: &'tcx Expr<'tcx>, + ctxt: SyntaxContext, +) -> Option { + match expr.kind { + ExprKind::MethodCall(method_seg, recv, _, _) => { + if is_fs_method_name(method_seg.ident.name) + && is_path_method_call(cx, expr) + && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) + { + return Some(expr.span); + } + // Peel through chains like `.metadata().unwrap()` or `.metadata().ok()` + find_fs_call_in_expr(cx, recv, path_recv, ctxt) + }, + // The `?` operator desugars to: + // Match(Call(TryTraitBranch, [inner_expr]), ..., TryDesugar) + // so we extract `inner_expr` and keep searching. + ExprKind::Match(scrutinee, _, MatchSource::TryDesugar(_)) => { + if let ExprKind::Call(_, [inner_expr]) = scrutinee.kind { + find_fs_call_in_expr(cx, inner_expr, path_recv, ctxt) + } else { + None + } + }, + _ => None, + } +} diff --git a/clippy_lints/src/unnecessary_path_exists.rs b/clippy_lints/src/unnecessary_path_exists.rs deleted file mode 100644 index af23fe1b7d83..000000000000 --- a/clippy_lints/src/unnecessary_path_exists.rs +++ /dev/null @@ -1,239 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::res::MaybeDef; -use clippy_utils::{SpanlessEq, higher, path_to_local_with_projections, sym}; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, PatKind, Stmt, StmtKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::declare_lint_pass; -use rustc_span::symbol::Symbol; -use rustc_span::{Span, SyntaxContext}; - -declare_clippy_lint! { - /// ### What it does - /// Checks for calls to `Path::exists` immediately before a filesystem - /// operation on the same path. - /// - /// ### Why is this bad? - /// Calling `exists()` and then performing a filesystem operation on the same - /// path is a classic Time-Of-Check to Time-Of-Use (TOCTOU) race condition. - /// Between the two calls another process can add, remove, or replace the - /// file, making the result of `exists()` stale. The filesystem operation - /// itself will indicate whether the path exists via its return value, making - /// the prior `exists()` check both redundant and dangerous. - /// - /// ### Example - /// ```rust,no_run - /// # use std::path::Path; - /// # fn example(path: &Path) { - /// if path.exists() { - /// let metadata = path.metadata().unwrap(); - /// // use metadata ... - /// } - /// # } - /// ``` - /// Use instead: - /// ```rust,no_run - /// # use std::path::Path; - /// # fn example(path: &Path) { - /// if let Ok(metadata) = path.metadata() { - /// // use metadata ... - /// } - /// # } - /// ``` - /// - /// ### Known problems - /// - Does not detect `std::fs` free functions used inside the block - /// (e.g. `fs::read(path)`, `fs::File::open(path)`), only method calls on - /// the path receiver itself. - /// - Does not detect `Path::try_exists()` (stabilized in Rust 1.63): the `?` - /// operator in the condition desugars to a `Match` node, so the condition - /// is not seen as a simple `.exists()` call. - /// - For the stored-bool variant (`let b = path.exists(); /* other stmts */; - /// if b { ... }`), only detects when the `if` immediately follows the `let`. - #[clippy::version = "1.98.0"] - pub UNNECESSARY_PATH_EXISTS, - nursery, - "calling `Path::exists` before a filesystem operation creates a TOCTOU race" -} - -declare_lint_pass!(UnnecessaryPathExists => [UNNECESSARY_PATH_EXISTS]); - -impl<'tcx> LateLintPass<'tcx> for UnnecessaryPathExists { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if expr.span.from_expansion() { - return; - } - - if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr) - && let Some((path_recv, exists_span)) = extract_exists_receiver(cx, cond) - && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, expr.span.ctxt()) - { - emit_lint(cx, exists_span, fs_call_span); - } - } - - fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { - if block.span.from_expansion() { - return; - } - - for (stmt, next_stmt) in block.stmts.iter().zip(block.stmts.iter().skip(1)) { - if let StmtKind::Expr(next_expr) | StmtKind::Semi(next_expr) = next_stmt.kind { - check_stored_bool_pair(cx, stmt, next_expr); - } - } - if let Some(last_stmt) = block.stmts.last() - && let Some(next_expr) = block.expr - { - check_stored_bool_pair(cx, last_stmt, next_expr); - } - } -} - -fn check_stored_bool_pair<'tcx>(cx: &LateContext<'tcx>, let_stmt: &'tcx Stmt<'tcx>, next_expr: &'tcx Expr<'tcx>) { - let StmtKind::Let(local) = let_stmt.kind else { - return; - }; - let Some(init) = local.init else { return }; - let Some((path_recv, exists_span)) = extract_exists_receiver(cx, init) else { - return; - }; - let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else { - return; - }; - - if let Some(higher::If { cond, then, .. }) = higher::If::hir(next_expr) - && path_to_local_with_projections(cond) == Some(binding_id) - && let Some(fs_call_span) = find_fs_call(cx, then, path_recv, next_expr.span.ctxt()) - { - emit_lint(cx, exists_span, fs_call_span); - } -} - -fn emit_lint(cx: &LateContext<'_>, exists_span: Span, fs_call_span: Span) { - span_lint_and_then( - cx, - UNNECESSARY_PATH_EXISTS, - exists_span, - "unnecessary `Path::exists` before a filesystem operation on the same path", - |diag| { - diag.span_note(fs_call_span, "the filesystem operation is here"); - diag.help( - "the `exists()` check is redundant and creates a TOCTOU race condition; \ - consider removing it and handling the error from the filesystem operation directly", - ); - }, - ); -} - -/// Returns `true` if `expr` is a method call that resolves to a method defined -/// on `std::path::Path` (handles any type that derefs to `Path`, e.g. `PathBuf`). -fn is_path_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - cx.typeck_results() - .type_dependent_def_id(expr.hir_id) - .is_some_and(|def_id| { - let parent = cx.tcx.parent(def_id); - cx.tcx - .type_of(parent) - .instantiate_identity() - .skip_norm_wip() - .is_diag_item(cx, sym::Path) - }) -} - -fn is_fs_method_name(name: Symbol) -> bool { - matches!( - name, - sym::canonicalize - | sym::is_dir - | sym::is_file - | sym::is_symlink - | sym::metadata - | sym::read_dir - | sym::read_link - | sym::symlink_metadata - ) -} - -/// Walks a condition expression to find a `Path::exists` call. -/// Returns the receiver expression and the span of the `.exists()` call. -/// Recurses through `&&` chains so compound conditions are handled. -fn extract_exists_receiver<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, Span)> { - match expr.kind { - ExprKind::MethodCall(seg, recv, [], _) - if seg.ident.name == sym::exists && !expr.span.from_expansion() && is_path_method_call(cx, expr) => - { - Some((recv, expr.span)) - }, - ExprKind::Binary(op, lhs, rhs) if op.node == BinOpKind::And => { - extract_exists_receiver(cx, lhs).or_else(|| extract_exists_receiver(cx, rhs)) - }, - _ => None, - } -} - -/// Searches the `then` block of the `if` for the first filesystem method call -/// on the same receiver as the `exists()` check. -fn find_fs_call<'tcx>( - cx: &LateContext<'tcx>, - then: &'tcx Expr<'tcx>, - path_recv: &'tcx Expr<'tcx>, - ctxt: SyntaxContext, -) -> Option { - let ExprKind::Block(block, _) = then.kind else { - return None; - }; - for stmt in block.stmts { - if let Some(span) = find_fs_call_in_stmt(cx, stmt, path_recv, ctxt) { - return Some(span); - } - } - block.expr.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt)) -} - -fn find_fs_call_in_stmt<'tcx>( - cx: &LateContext<'tcx>, - stmt: &'tcx Stmt<'tcx>, - path_recv: &'tcx Expr<'tcx>, - ctxt: SyntaxContext, -) -> Option { - match stmt.kind { - StmtKind::Expr(e) | StmtKind::Semi(e) => find_fs_call_in_expr(cx, e, path_recv, ctxt), - StmtKind::Let(local) => local - .init - .and_then(|init| find_fs_call_in_expr(cx, init, path_recv, ctxt)), - StmtKind::Item(_) => None, - } -} - -/// Peels through method chains (e.g. `.metadata().unwrap()`) and the `?` operator -/// desugaring to find a filesystem method call on `path_recv`. -fn find_fs_call_in_expr<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'tcx>, - path_recv: &'tcx Expr<'tcx>, - ctxt: SyntaxContext, -) -> Option { - match expr.kind { - ExprKind::MethodCall(method_seg, recv, _, _) => { - if is_fs_method_name(method_seg.ident.name) - && is_path_method_call(cx, expr) - && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv) - { - return Some(expr.span); - } - // Peel through chains like `.metadata().unwrap()` or `.metadata().ok()` - find_fs_call_in_expr(cx, recv, path_recv, ctxt) - }, - // The `?` operator desugars to: - // Match(Call(TryTraitBranch, [inner_expr]), ..., TryDesugar) - // so we extract `inner_expr` and keep searching. - ExprKind::Match(scrutinee, _, MatchSource::TryDesugar(_)) => { - if let ExprKind::Call(_, [inner_expr]) = scrutinee.kind { - find_fs_call_in_expr(cx, inner_expr, path_recv, ctxt) - } else { - None - } - }, - _ => None, - } -} diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 8dd1756e4876..c0f5ee676b60 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -626,6 +626,7 @@ generate! { trim_start, trim_start_matches, truncate, + try_exists, try_fold, try_for_each, try_from_fn, diff --git a/tests/ui/unnecessary_path_exists.rs b/tests/ui/unnecessary_path_exists.rs index 73cfbc2beffa..2cc87f7bc711 100644 --- a/tests/ui/unnecessary_path_exists.rs +++ b/tests/ui/unnecessary_path_exists.rs @@ -1,5 +1,4 @@ #![warn(clippy::unnecessary_path_exists)] -#![allow(unused)] use std::path::{Path, PathBuf}; @@ -76,6 +75,34 @@ fn check_with_result(path: &Path) -> std::io::Result<()> { Ok(()) } +fn check_try_exists(path: &Path) -> std::io::Result<()> { + // `try_exists()?` as a direct condition + if path.try_exists()? { + //~^ unnecessary_path_exists + let _ = path.metadata()?; + } + + // `try_exists()?` in a compound condition + if path.try_exists()? && true { + //~^ unnecessary_path_exists + let _ = path.metadata()?; + } + + // `try_exists()?` stored in a bool, immediately followed by if + let exists = path.try_exists()?; + //~^ unnecessary_path_exists + if exists { + let _ = path.metadata()?; + } + + // `.unwrap_or(false)` instead of `?` — not detected (known limitation) + if path.try_exists().unwrap_or(false) { + let _ = path.metadata()?; + } + + Ok(()) +} + fn check_statement_forms(path: &Path) { // no let binding if path.exists() { @@ -166,4 +193,23 @@ fn check_false_positives(path: &Path) { } } +struct Custom; + +impl Custom { + fn exists(&self) -> bool { + true + } + + fn metadata(&self) -> u32 { + 0 + } +} + +fn check_unrelated_exists_method(c: Custom) { + // `exists`/`metadata` here are unrelated to `Path` — no lint + if c.exists() { + let _ = c.metadata(); + } +} + fn main() {} diff --git a/tests/ui/unnecessary_path_exists.stderr b/tests/ui/unnecessary_path_exists.stderr index 82c050991742..2b9e8d0f3feb 100644 --- a/tests/ui/unnecessary_path_exists.stderr +++ b/tests/ui/unnecessary_path_exists.stderr @@ -1,11 +1,11 @@ error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:7:8 + --> tests/ui/unnecessary_path_exists.rs:6:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:9:17 + --> tests/ui/unnecessary_path_exists.rs:8:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ @@ -14,225 +14,264 @@ LL | let _ = path.metadata().unwrap(); = help: to override `-D warnings` add `#[allow(clippy::unnecessary_path_exists)]` error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:12:8 + --> tests/ui/unnecessary_path_exists.rs:11:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:14:17 + --> tests/ui/unnecessary_path_exists.rs:13:17 | LL | let _ = path.is_file(); | ^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:17:8 + --> tests/ui/unnecessary_path_exists.rs:16:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:19:17 + --> tests/ui/unnecessary_path_exists.rs:18:17 | LL | let _ = path.is_dir(); | ^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:22:8 + --> tests/ui/unnecessary_path_exists.rs:21:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:24:17 + --> tests/ui/unnecessary_path_exists.rs:23:17 | LL | let _ = path.is_symlink(); | ^^^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:27:8 + --> tests/ui/unnecessary_path_exists.rs:26:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:29:17 + --> tests/ui/unnecessary_path_exists.rs:28:17 | LL | let _ = path.canonicalize().unwrap(); | ^^^^^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:32:8 + --> tests/ui/unnecessary_path_exists.rs:31:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:34:17 + --> tests/ui/unnecessary_path_exists.rs:33:17 | LL | let _ = path.read_dir().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:37:8 + --> tests/ui/unnecessary_path_exists.rs:36:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:39:17 + --> tests/ui/unnecessary_path_exists.rs:38:17 | LL | let _ = path.symlink_metadata().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:43:8 + --> tests/ui/unnecessary_path_exists.rs:42:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:45:17 + --> tests/ui/unnecessary_path_exists.rs:44:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:57:8 + --> tests/ui/unnecessary_path_exists.rs:56:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:59:17 + --> tests/ui/unnecessary_path_exists.rs:58:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:72:8 + --> tests/ui/unnecessary_path_exists.rs:71:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:74:17 + --> tests/ui/unnecessary_path_exists.rs:73:17 | LL | let _ = path.metadata()?; | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:81:8 + --> tests/ui/unnecessary_path_exists.rs:80:8 + | +LL | if path.try_exists()? { + | ^^^^^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:82:17 + | +LL | let _ = path.metadata()?; + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:86:8 + | +LL | if path.try_exists()? && true { + | ^^^^^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:88:17 + | +LL | let _ = path.metadata()?; + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:92:18 + | +LL | let exists = path.try_exists()?; + | ^^^^^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:95:17 + | +LL | let _ = path.metadata()?; + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:108:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:83:9 + --> tests/ui/unnecessary_path_exists.rs:110:9 | LL | path.metadata().ok(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:87:8 + --> tests/ui/unnecessary_path_exists.rs:114:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:90:17 + --> tests/ui/unnecessary_path_exists.rs:117:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:94:8 + --> tests/ui/unnecessary_path_exists.rs:121:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:96:17 + --> tests/ui/unnecessary_path_exists.rs:123:17 | LL | let _ = path.metadata().ok().is_some(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:104:8 + --> tests/ui/unnecessary_path_exists.rs:131:8 | LL | if path.exists() && condition { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:106:17 + --> tests/ui/unnecessary_path_exists.rs:133:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:110:21 + --> tests/ui/unnecessary_path_exists.rs:137:21 | LL | if condition && path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:112:17 + --> tests/ui/unnecessary_path_exists.rs:139:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:118:18 + --> tests/ui/unnecessary_path_exists.rs:145:18 | LL | let exists = path.exists(); | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:121:17 + --> tests/ui/unnecessary_path_exists.rs:148:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:126:19 + --> tests/ui/unnecessary_path_exists.rs:153:19 | LL | let exists2 = path2.exists(); | ^^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:129:17 + --> tests/ui/unnecessary_path_exists.rs:156:17 | LL | let _ = path2.metadata().unwrap(); | ^^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:133:19 + --> tests/ui/unnecessary_path_exists.rs:160:19 | LL | let exists3 = path.exists(); | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:136:17 + --> tests/ui/unnecessary_path_exists.rs:163:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly -error: aborting due to 18 previous errors +error: aborting due to 21 previous errors From daf0c9e7a33f6e60adc456e8d18317ce10115ec5 Mon Sep 17 00:00:00 2001 From: David Hernandez Date: Tue, 7 Jul 2026 18:07:05 -0700 Subject: [PATCH 5/5] add tests for Deref, macros, and remaining false positives --- tests/ui/unnecessary_path_exists.rs | 93 ++++++++++++++++++++++ tests/ui/unnecessary_path_exists.stderr | 101 +++++++++++++++++------- 2 files changed, 165 insertions(+), 29 deletions(-) diff --git a/tests/ui/unnecessary_path_exists.rs b/tests/ui/unnecessary_path_exists.rs index 2cc87f7bc711..52ad5d54dcc1 100644 --- a/tests/ui/unnecessary_path_exists.rs +++ b/tests/ui/unnecessary_path_exists.rs @@ -38,6 +38,11 @@ fn check_path(path: &Path) { let _ = path.symlink_metadata().unwrap(); } + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.read_link().unwrap(); + } + // has an else branch — TOCTOU race still present if path.exists() { //~^ unnecessary_path_exists @@ -191,6 +196,33 @@ fn check_false_positives(path: &Path) { if path.exists() { let _ = std::fs::read(path); } + + // negated condition — the `exists()` result being true isn't what guards the `then` + // branch, so no lint + if !path.exists() { + let _ = path.metadata(); + } + + // `||` instead of `&&` — `exists()` being true doesn't guarantee the `then` branch only + // runs when the path exists, so no lint + if path.exists() || true { + let _ = path.metadata(); + } + + // fs call only in the `else` branch — the `exists()` check guards the *other* branch, + // so no lint + if path.exists() { + println!("path exists"); + } else { + let _ = path.metadata(); + } + + // `path` is shadowed between the `exists()` check and the filesystem call, so the + // filesystem call is on a different local — no lint + if path.exists() { + let path = Path::new("other"); + let _ = path.metadata(); + } } struct Custom; @@ -212,4 +244,65 @@ fn check_unrelated_exists_method(c: Custom) { } } +use std::ops::Deref; + +struct PathWrapper(PathBuf); + +impl Deref for PathWrapper { + type Target = Path; + + fn deref(&self) -> &Path { + &self.0 + } +} + +fn check_deref_to_path(path: PathWrapper) { + // `exists`/`metadata` resolve through `Deref` — still lints + if path.exists() { + //~^ unnecessary_path_exists + let _ = path.metadata().unwrap(); + } +} + +macro_rules! exists_then_metadata { + ($path:expr) => { + if $path.exists() { + let _ = $path.metadata().unwrap(); + } + }; +} + +fn check_macro_generates_whole_pattern(path: &Path) { + // the entire `if`/`exists`/`metadata` pattern comes from a macro expansion — no lint + exists_then_metadata!(path); +} + +macro_rules! path_exists { + ($path:expr) => { + $path.exists() + }; +} + +fn check_macro_generates_condition(path: &Path) { + // the `exists()` call itself comes from a macro expansion — no lint (the `Methods` lint + // pass skips any expression whose span originates from a macro) + if path_exists!(path) { + let _ = path.metadata().unwrap(); + } +} + +macro_rules! read_metadata { + ($path:expr) => { + let _ = $path.metadata().unwrap(); + }; +} + +fn check_macro_generates_fs_call(path: &Path) { + // the filesystem call comes from a macro, but the `if`/`exists()` are written directly + if path.exists() { + //~^ unnecessary_path_exists + read_metadata!(path); + } +} + fn main() {} diff --git a/tests/ui/unnecessary_path_exists.stderr b/tests/ui/unnecessary_path_exists.stderr index 2b9e8d0f3feb..2cb4342c6e7a 100644 --- a/tests/ui/unnecessary_path_exists.stderr +++ b/tests/ui/unnecessary_path_exists.stderr @@ -92,186 +92,229 @@ LL | let _ = path.symlink_metadata().unwrap(); = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:42:8 + --> tests/ui/unnecessary_path_exists.rs:41:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:44:17 + --> tests/ui/unnecessary_path_exists.rs:43:17 + | +LL | let _ = path.read_link().unwrap(); + | ^^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:47:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:49:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:56:8 + --> tests/ui/unnecessary_path_exists.rs:61:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:58:17 + --> tests/ui/unnecessary_path_exists.rs:63:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:71:8 + --> tests/ui/unnecessary_path_exists.rs:76:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:73:17 + --> tests/ui/unnecessary_path_exists.rs:78:17 | LL | let _ = path.metadata()?; | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:80:8 + --> tests/ui/unnecessary_path_exists.rs:85:8 | LL | if path.try_exists()? { | ^^^^^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:82:17 + --> tests/ui/unnecessary_path_exists.rs:87:17 | LL | let _ = path.metadata()?; | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:86:8 + --> tests/ui/unnecessary_path_exists.rs:91:8 | LL | if path.try_exists()? && true { | ^^^^^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:88:17 + --> tests/ui/unnecessary_path_exists.rs:93:17 | LL | let _ = path.metadata()?; | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:92:18 + --> tests/ui/unnecessary_path_exists.rs:97:18 | LL | let exists = path.try_exists()?; | ^^^^^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:95:17 + --> tests/ui/unnecessary_path_exists.rs:100:17 | LL | let _ = path.metadata()?; | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:108:8 + --> tests/ui/unnecessary_path_exists.rs:113:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:110:9 + --> tests/ui/unnecessary_path_exists.rs:115:9 | LL | path.metadata().ok(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:114:8 + --> tests/ui/unnecessary_path_exists.rs:119:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:117:17 + --> tests/ui/unnecessary_path_exists.rs:122:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:121:8 + --> tests/ui/unnecessary_path_exists.rs:126:8 | LL | if path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:123:17 + --> tests/ui/unnecessary_path_exists.rs:128:17 | LL | let _ = path.metadata().ok().is_some(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:131:8 + --> tests/ui/unnecessary_path_exists.rs:136:8 | LL | if path.exists() && condition { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:133:17 + --> tests/ui/unnecessary_path_exists.rs:138:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:137:21 + --> tests/ui/unnecessary_path_exists.rs:142:21 | LL | if condition && path.exists() { | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:139:17 + --> tests/ui/unnecessary_path_exists.rs:144:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:145:18 + --> tests/ui/unnecessary_path_exists.rs:150:18 | LL | let exists = path.exists(); | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:148:17 + --> tests/ui/unnecessary_path_exists.rs:153:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:153:19 + --> tests/ui/unnecessary_path_exists.rs:158:19 | LL | let exists2 = path2.exists(); | ^^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:156:17 + --> tests/ui/unnecessary_path_exists.rs:161:17 | LL | let _ = path2.metadata().unwrap(); | ^^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly error: unnecessary `Path::exists` before a filesystem operation on the same path - --> tests/ui/unnecessary_path_exists.rs:160:19 + --> tests/ui/unnecessary_path_exists.rs:165:19 | LL | let exists3 = path.exists(); | ^^^^^^^^^^^^^ | note: the filesystem operation is here - --> tests/ui/unnecessary_path_exists.rs:163:17 + --> tests/ui/unnecessary_path_exists.rs:168:17 | LL | let _ = path.metadata().unwrap(); | ^^^^^^^^^^^^^^^ = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly -error: aborting due to 21 previous errors +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:261:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:263:17 + | +LL | let _ = path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^ + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + +error: unnecessary `Path::exists` before a filesystem operation on the same path + --> tests/ui/unnecessary_path_exists.rs:302:8 + | +LL | if path.exists() { + | ^^^^^^^^^^^^^ + | +note: the filesystem operation is here + --> tests/ui/unnecessary_path_exists.rs:296:17 + | +LL | let _ = $path.metadata().unwrap(); + | ^^^^^^^^^^^^^^^^ +... +LL | read_metadata!(path); + | -------------------- in this macro invocation + = help: the `exists()` check is redundant and creates a TOCTOU race condition; consider removing it and handling the error from the filesystem operation directly + = note: this error originates in the macro `read_metadata` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 24 previous errors