new lint: unnecessary_path_exists

CHANGELOG.md

@@ -7562,6 +7562,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_rest_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_rest_pattern
 [`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

clippy_dev/src/setup/vscode.rs

@@ -19,6 +19,11 @@ pub fn install_tasks(force_override: bool) {
     }
 }
 
+// `vs_dir_path.exists()` isn't redundant here: the `else` branch below relies on it to tell
+// "doesn't exist yet" apart from "exists but isn't a directory", which `is_dir()` alone can't do
+// (it returns `false` for both). A `fs::metadata()`-based rewrite could distinguish all three
+// states and close this for real, but that's a separate change from the lint this PR adds.
+#[expect(clippy::unnecessary_path_exists)]
 fn check_install_precondition(force_override: bool) -> bool {
     let vs_dir_path = Path::new(VSCODE_DIR);
     if vs_dir_path.exists() {

clippy_lints/src/declared_lints.rs

@@ -510,6 +510,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,

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),

clippy_lints/src/methods/unnecessary_path_exists.rs

@@ -0,0 +1,212 @@
+use super::UNNECESSARY_PATH_EXISTS;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::MaybeDef;
+use clippy_utils::visitors::for_each_expr_without_closures;
+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, Symbol, SyntaxContext};
+use std::ops::ControlFlow;
+
+/// `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, fs_method_name)) = find_fs_call(cx, then, recv, ctxt)
+    {
+        // `is_dir`/`is_file` return `bool`, not `Result`, so there's no error to hand off to —
+        // point at `metadata()` instead, which folds the existence and type checks into one call.
+        let help = match fs_method_name {
+            sym::is_dir | sym::is_file => {
+                "the `exists()` check is redundant and creates a TOCTOU race condition; \
+                 consider using `metadata()` instead, which can check existence and type in a \
+                 single filesystem operation"
+            },
+            _ => {
+                "the `exists()` check is redundant and creates a TOCTOU race condition; \
+                 consider removing it and handling the error from the filesystem operation directly"
+            },
+        };
+        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(help);
+            },
+        );
+    }
+}
+
+/// 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(<lang item Try::branch>, [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()))
+}
+
+/// `is_symlink` is deliberately excluded: unlike the other methods here, it
+/// doesn't follow the symlink, so it doesn't check the same thing `exists()`
+/// does (which does follow it) — the two calls aren't actually redundant, and
+/// `is_symlink` doesn't even return a `Result` for the "handle the error
+/// directly" suggestion to apply to.
+fn is_fs_method_name(name: Symbol) -> bool {
+    matches!(
+        name,
+        sym::canonicalize
+            | sym::is_dir
+            | sym::is_file
+            | 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)
+        .opt_parent(cx)
+        .opt_impl_ty(cx)
+        .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.
+///
+/// Bails out entirely if `path_recv` isn't a stable place (a local, optionally
+/// through field/index projections) — e.g. `dyn_path()` or `iter.next()?`
+/// aren't guaranteed to return the same thing twice, so a textually identical
+/// call proves nothing about the same path being checked twice. Also bails if
+/// that place is reassigned, or mutated through a `&mut self` method (e.g.
+/// `PathBuf::push`), before the matching call is found — either way the
+/// `exists()` result no longer describes the value being operated on.
+///
+/// Closures are not descended into: code inside a closure body doesn't run as
+/// part of this `if`, so a filesystem call written there isn't provably the
+/// redundant call this lint is looking for.
+fn find_fs_call<'tcx>(
+    cx: &LateContext<'tcx>,
+    then: &'tcx Expr<'tcx>,
+    path_recv: &'tcx Expr<'tcx>,
+    ctxt: SyntaxContext,
+) -> Option<(Span, Symbol)> {
+    let base_local = path_to_local_with_projections(path_recv)?;
+    for_each_expr_without_closures(then, |e| {
+        if let ExprKind::Assign(lhs, ..) = e.kind
+            && path_to_local_with_projections(lhs) == Some(base_local)
+        {
+            return ControlFlow::Break(None);
+        }
+        if let ExprKind::MethodCall(method_seg, recv, _, _) = e.kind
+            && path_to_local_with_projections(recv) == Some(base_local)
+        {
+            if is_fs_method_name(method_seg.ident.name)
+                && is_path_method_call(cx, e)
+                && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv)
+            {
+                return ControlFlow::Break(Some((e.span, method_seg.ident.name)));
+            }
+            // None of the tracked fs methods take `&mut self`, so this can only trigger on an
+            // unrelated mutating call (e.g. `.push()`, `.pop()`, `.clear()` on a `PathBuf`).
+            if cx.typeck_results().expr_ty_adjusted(recv).is_mutable_ptr() {
+                return ControlFlow::Break(None);
+            }
+        }
+        ControlFlow::Continue(())
+    })
+    .flatten()
+}

clippy_utils/src/sym.rs

@@ -156,6 +156,7 @@ generate! {
     build_hasher,
     by_ref,
     bytes,
+    canonicalize,
     capacity,
     cargo_clippy: "cargo-clippy",
     cast,
@@ -223,6 +224,7 @@ generate! {
     eprint_macro,
     eprintln_macro,
     err,
+    exists,
     exp,
     expect_err,
     expn_data,
@@ -376,6 +378,7 @@ generate! {
     is_diag_item,
     is_diagnostic_item,
     is_digit,
+    is_dir,
     is_empty,
     is_err,
     is_file,
@@ -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,
@@ -619,6 +626,7 @@ generate! {
     trim_start,
     trim_start_matches,
     truncate,
+    try_exists,
     try_fold,
     try_for_each,
     try_from_fn,