Skip to content

Commit

Permalink
Fix shadow_unrelated's behaviour with closures (#13677)
Browse files Browse the repository at this point in the history
Fixes #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 linebender/xilem#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
  • Loading branch information
Jarcho authored Nov 30, 2024
2 parents af1f78a + d1cab08 commit 650e0c8
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 9 deletions.
58 changes: 50 additions & 8 deletions clippy_lints/src/shadow.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -175,17 +178,39 @@ 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, "_"),
snippet(cx, expr.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)
},
Expand Down Expand Up @@ -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;
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/shadow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
26 changes: 25 additions & 1 deletion tests/ui/shadow.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 650e0c8

Please sign in to comment.