about summary refs log tree commit diff
diff options
context:
space:
mode:
authorJason Newcomb <jsnewcomb@pm.me>2024-11-30 17:08:08 +0000
committerGitHub <noreply@github.com>2024-11-30 17:08:08 +0000
commit650e0c8d3deeffc38cb7ca3f1eb4c7ac42b46d48 (patch)
treec66c06301cf2640567ccc198a47cbc43b70adf0e
parentaf1f78af05af5ff7dd2d8b31386f96ce9bfc7a8a (diff)
parentd1cab0860a3b8d28f84a96661a1c6177a5e3fbba (diff)
downloadrust-650e0c8d3deeffc38cb7ca3f1eb4c7ac42b46d48.tar.gz
rust-650e0c8d3deeffc38cb7ca3f1eb4c7ac42b46d48.zip
Fix `shadow_unrelated`'s behaviour with closures (#13677)
Fixes https://github.com/rust-lang/rust-clippy/issues/10780

We correctly no longer give a warning when a closure is passed to a
method, where one of the arguments to that method uses the variable
which would be shadowed by an argument to that closure.
Uses is defined loosely as any expression used in the calling expression
mentions the shadowee binding (except for the closure itself):

```rust
#![deny(clippy::shadow_unrelated)]
let x = Some(1);
let y = x.map(|x| x + 1);
```
will now succeed.

See https://github.com/linebender/xilem/pull/745 - without this change,
all of the `expect(shadow_unrelated)` in the repository are met; with
it, none of them are.

changelog: [`shadow_unrelated`]: Don't treat closures arguments as
unrelated when the calling function uses them
-rw-r--r--clippy_lints/src/shadow.rs58
-rw-r--r--tests/ui/shadow.rs14
-rw-r--r--tests/ui/shadow.stderr26
3 files changed, 89 insertions, 9 deletions
diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs
index 7ae0310b6d9..49d22377119 100644
--- a/clippy_lints/src/shadow.rs
+++ b/clippy_lints/src/shadow.rs
@@ -1,6 +1,9 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::path_to_local_id;
 use clippy_utils::source::snippet;
-use clippy_utils::visitors::is_local_used;
+use clippy_utils::visitors::{Descend, Visitable, for_each_expr};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::def::Res;
 use rustc_hir::def_id::LocalDefId;
@@ -175,9 +178,31 @@ fn is_shadow(cx: &LateContext<'_>, owner: LocalDefId, first: ItemLocalId, second
     false
 }
 
+/// Checks if the given local is used, except for in child expression of `except`.
+///
+/// This is a version of [`is_local_used`](clippy_utils::visitors::is_local_used), used to
+/// implement the fix for <https://github.com/rust-lang/rust-clippy/issues/10780>.
+pub fn is_local_used_except<'tcx>(
+    cx: &LateContext<'tcx>,
+    visitable: impl Visitable<'tcx>,
+    id: HirId,
+    except: Option<HirId>,
+) -> bool {
+    for_each_expr(cx, visitable, |e| {
+        if except.is_some_and(|it| it == e.hir_id) {
+            ControlFlow::Continue(Descend::No)
+        } else if path_to_local_id(e, id) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(Descend::Yes)
+        }
+    })
+    .is_some()
+}
+
 fn lint_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, shadowed: HirId, span: Span) {
     let (lint, msg) = match find_init(cx, pat.hir_id) {
-        Some(expr) if is_self_shadow(cx, pat, expr, shadowed) => {
+        Some((expr, _)) if is_self_shadow(cx, pat, expr, shadowed) => {
             let msg = format!(
                 "`{}` is shadowed by itself in `{}`",
                 snippet(cx, pat.span, "_"),
@@ -185,7 +210,7 @@ fn lint_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, shadowed: HirId, span: Span)
             );
             (SHADOW_SAME, msg)
         },
-        Some(expr) if is_local_used(cx, expr, shadowed) => {
+        Some((expr, except)) if is_local_used_except(cx, expr, shadowed, except) => {
             let msg = format!("`{}` is shadowed", snippet(cx, pat.span, "_"));
             (SHADOW_REUSE, msg)
         },
@@ -232,15 +257,32 @@ fn is_self_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, mut expr: &Expr<'_>, hir_
 
 /// Finds the "init" expression for a pattern: `let <pat> = <init>;` (or `if let`) or
 /// `match <init> { .., <pat> => .., .. }`
-fn find_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Expr<'tcx>> {
-    for (_, node) in cx.tcx.hir().parent_iter(hir_id) {
+///
+/// For closure arguments passed to a method call, returns the method call, and the `HirId` of the
+/// closure (which will later be skipped). This is for <https://github.com/rust-lang/rust-clippy/issues/10780>
+fn find_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<(&'tcx Expr<'tcx>, Option<HirId>)> {
+    for (hir_id, node) in cx.tcx.hir().parent_iter(hir_id) {
         let init = match node {
-            Node::Arm(_) | Node::Pat(_) => continue,
+            Node::Arm(_) | Node::Pat(_) | Node::Param(_) => continue,
             Node::Expr(expr) => match expr.kind {
-                ExprKind::Match(e, _, _) | ExprKind::Let(&LetExpr { init: e, .. }) => Some(e),
+                ExprKind::Match(e, _, _) | ExprKind::Let(&LetExpr { init: e, .. }) => Some((e, None)),
+                // If we're a closure argument, then a parent call is also an associated item.
+                ExprKind::Closure(_) => {
+                    if let Some((_, node)) = cx.tcx.hir().parent_iter(hir_id).next() {
+                        match node {
+                            Node::Expr(expr) => match expr.kind {
+                                ExprKind::MethodCall(_, _, _, _) | ExprKind::Call(_, _) => Some((expr, Some(hir_id))),
+                                _ => None,
+                            },
+                            _ => None,
+                        }
+                    } else {
+                        None
+                    }
+                },
                 _ => None,
             },
-            Node::LetStmt(local) => local.init,
+            Node::LetStmt(local) => local.init.map(|init| (init, None)),
             _ => None,
         };
         return init;
diff --git a/tests/ui/shadow.rs b/tests/ui/shadow.rs
index 258dba9dd83..30b96277f99 100644
--- a/tests/ui/shadow.rs
+++ b/tests/ui/shadow.rs
@@ -119,4 +119,18 @@ fn ice_8748() {
     }];
 }
 
+// https://github.com/rust-lang/rust-clippy/issues/10780
+fn shadow_closure() {
+    // These are not shadow_unrelated; but they are correctly shadow_reuse
+    let x = Some(1);
+    #[allow(clippy::shadow_reuse)]
+    let y = x.map(|x| x + 1);
+    let z = x.map(|x| x + 1);
+    let a: Vec<Option<u8>> = [100u8, 120, 140]
+        .iter()
+        .map(|i| i.checked_mul(2))
+        .map(|i| i.map(|i| i - 10))
+        .collect();
+}
+
 fn main() {}
diff --git a/tests/ui/shadow.stderr b/tests/ui/shadow.stderr
index fdd149a2216..5d2d7060450 100644
--- a/tests/ui/shadow.stderr
+++ b/tests/ui/shadow.stderr
@@ -280,5 +280,29 @@ note: previous binding is here
 LL |         let x = 1;
    |             ^
 
-error: aborting due to 23 previous errors
+error: `x` is shadowed
+  --> tests/ui/shadow.rs:128:20
+   |
+LL |     let z = x.map(|x| x + 1);
+   |                    ^
+   |
+note: previous binding is here
+  --> tests/ui/shadow.rs:125:9
+   |
+LL |     let x = Some(1);
+   |         ^
+
+error: `i` is shadowed
+  --> tests/ui/shadow.rs:132:25
+   |
+LL |         .map(|i| i.map(|i| i - 10))
+   |                         ^
+   |
+note: previous binding is here
+  --> tests/ui/shadow.rs:132:15
+   |
+LL |         .map(|i| i.map(|i| i - 10))
+   |               ^
+
+error: aborting due to 25 previous errors