Skip to content

Commit

Permalink
Fix shadow_unrelated's behaviour with closures
Browse files Browse the repository at this point in the history
Fixes #10780
  • Loading branch information
DJMcNab committed Nov 12, 2024
1 parent f58088b commit d1cab08
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 d1cab08

Please sign in to comment.