Make expr_use_ctxt always return Some unless the syntax context changes.

This commit is contained in:
Jason Newcomb 2023-11-14 22:41:38 -05:00
parent b587871e27
commit cdc4c697b5
7 changed files with 112 additions and 122 deletions

View file

@ -1,5 +1,6 @@
#![feature(array_chunks)] #![feature(array_chunks)]
#![feature(box_patterns)] #![feature(box_patterns)]
#![feature(control_flow_enum)]
#![feature(if_let_guard)] #![feature(if_let_guard)]
#![feature(let_chains)] #![feature(let_chains)]
#![feature(lint_reasons)] #![feature(lint_reasons)]
@ -2508,26 +2509,30 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {
&& item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests") && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests")
} }
/// Walks the HIR tree from the given expression, up to the node where the value produced by the /// Walks up the HIR tree from the given expression in an attempt to find where the value is
/// expression is consumed. Calls the function for every node encountered this way until it returns /// consumed.
/// `Some`.
/// ///
/// This allows walking through `if`, `match`, `break`, block expressions to find where the value /// Termination has three conditions:
/// produced by the expression is consumed. /// - The given function returns `Break`. This function will return the value.
/// - The consuming node is found. This function will return `Continue(use_node, child_id)`.
/// - No further parent nodes are found. This will trigger a debug assert or return `None`.
///
/// This allows walking through `if`, `match`, `break`, and block expressions to find where the
/// value produced by the expression is consumed.
pub fn walk_to_expr_usage<'tcx, T>( pub fn walk_to_expr_usage<'tcx, T>(
cx: &LateContext<'tcx>, cx: &LateContext<'tcx>,
e: &Expr<'tcx>, e: &Expr<'tcx>,
mut f: impl FnMut(Node<'tcx>, HirId) -> Option<T>, mut f: impl FnMut(HirId, Node<'tcx>, HirId) -> ControlFlow<T>,
) -> Option<T> { ) -> Option<ControlFlow<T, (Node<'tcx>, HirId)>> {
let map = cx.tcx.hir(); let map = cx.tcx.hir();
let mut iter = map.parent_iter(e.hir_id); let mut iter = map.parent_iter(e.hir_id);
let mut child_id = e.hir_id; let mut child_id = e.hir_id;
while let Some((parent_id, parent)) = iter.next() { while let Some((parent_id, parent)) = iter.next() {
if let Some(x) = f(parent, child_id) { if let ControlFlow::Break(x) = f(parent_id, parent, child_id) {
return Some(x); return Some(ControlFlow::Break(x));
} }
let parent = match parent { let parent_expr = match parent {
Node::Expr(e) => e, Node::Expr(e) => e,
Node::Block(Block { expr: Some(body), .. }) | Node::Arm(Arm { body, .. }) if body.hir_id == child_id => { Node::Block(Block { expr: Some(body), .. }) | Node::Arm(Arm { body, .. }) if body.hir_id == child_id => {
child_id = parent_id; child_id = parent_id;
@ -2537,18 +2542,19 @@ pub fn walk_to_expr_usage<'tcx, T>(
child_id = parent_id; child_id = parent_id;
continue; continue;
}, },
_ => return None, _ => return Some(ControlFlow::Continue((parent, child_id))),
}; };
match parent.kind { match parent_expr.kind {
ExprKind::If(child, ..) | ExprKind::Match(child, ..) if child.hir_id != child_id => child_id = parent_id, ExprKind::If(child, ..) | ExprKind::Match(child, ..) if child.hir_id != child_id => child_id = parent_id,
ExprKind::Break(Destination { target_id: Ok(id), .. }, _) => { ExprKind::Break(Destination { target_id: Ok(id), .. }, _) => {
child_id = id; child_id = id;
iter = map.parent_iter(id); iter = map.parent_iter(id);
}, },
ExprKind::Block(..) => child_id = parent_id, ExprKind::Block(..) | ExprKind::DropTemps(_) => child_id = parent_id,
_ => return None, _ => return Some(ControlFlow::Continue((parent, child_id))),
} }
} }
debug_assert!(false, "no parent node found for `{child_id:?}`");
None None
} }
@ -2592,6 +2598,8 @@ pub enum ExprUseNode<'tcx> {
Callee, Callee,
/// Access of a field. /// Access of a field.
FieldAccess(Ident), FieldAccess(Ident),
Expr,
Other,
} }
impl<'tcx> ExprUseNode<'tcx> { impl<'tcx> ExprUseNode<'tcx> {
/// Checks if the value is returned from the function. /// Checks if the value is returned from the function.
@ -2668,144 +2676,104 @@ impl<'tcx> ExprUseNode<'tcx> {
let sig = cx.tcx.fn_sig(id).skip_binder(); let sig = cx.tcx.fn_sig(id).skip_binder();
Some(DefinedTy::Mir(cx.tcx.param_env(id).and(sig.input(i)))) Some(DefinedTy::Mir(cx.tcx.param_env(id).and(sig.input(i))))
}, },
Self::Local(_) | Self::FieldAccess(..) | Self::Callee => None, Self::Local(_) | Self::FieldAccess(..) | Self::Callee | Self::Expr | Self::Other => None,
} }
} }
} }
/// Gets the context an expression's value is used in. /// Gets the context an expression's value is used in.
#[expect(clippy::too_many_lines)]
pub fn expr_use_ctxt<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> Option<ExprUseCtxt<'tcx>> { pub fn expr_use_ctxt<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> Option<ExprUseCtxt<'tcx>> {
let mut adjustments = [].as_slice(); let mut adjustments = [].as_slice();
let mut is_ty_unified = false; let mut is_ty_unified = false;
let mut moved_before_use = false; let mut moved_before_use = false;
let ctxt = e.span.ctxt(); let ctxt = e.span.ctxt();
walk_to_expr_usage(cx, e, &mut |parent, child_id| { walk_to_expr_usage(cx, e, &mut |parent_id, parent, child_id| {
// LocalTableInContext returns the wrong lifetime, so go use `expr_adjustments` instead.
if adjustments.is_empty() if adjustments.is_empty()
&& let Node::Expr(e) = cx.tcx.hir().get(child_id) && let Node::Expr(e) = cx.tcx.hir().get(child_id)
{ {
adjustments = cx.typeck_results().expr_adjustments(e); adjustments = cx.typeck_results().expr_adjustments(e);
} }
match parent { if !cx.tcx.hir().opt_span(parent_id).is_some_and(|x| x.ctxt() == ctxt) {
Node::Local(l) if l.span.ctxt() == ctxt => Some(ExprUseCtxt { return ControlFlow::Break(());
node: ExprUseNode::Local(l), }
adjustments, if let Node::Expr(e) = parent {
is_ty_unified, match e.kind {
moved_before_use, ExprKind::If(e, _, _) | ExprKind::Match(e, _, _) if e.hir_id != child_id => {
}), is_ty_unified = true;
moved_before_use = true;
},
ExprKind::Block(_, Some(_)) | ExprKind::Break(..) => {
is_ty_unified = true;
moved_before_use = true;
},
ExprKind::Block(..) => moved_before_use = true,
_ => {},
}
}
ControlFlow::Continue(())
})?
.continue_value()
.map(|(use_node, child_id)| {
let node = match use_node {
Node::Local(l) => ExprUseNode::Local(l),
Node::ExprField(field) => ExprUseNode::Field(field),
Node::Item(&Item { Node::Item(&Item {
kind: ItemKind::Static(..) | ItemKind::Const(..), kind: ItemKind::Static(..) | ItemKind::Const(..),
owner_id, owner_id,
span,
.. ..
}) })
| Node::TraitItem(&TraitItem { | Node::TraitItem(&TraitItem {
kind: TraitItemKind::Const(..), kind: TraitItemKind::Const(..),
owner_id, owner_id,
span,
.. ..
}) })
| Node::ImplItem(&ImplItem { | Node::ImplItem(&ImplItem {
kind: ImplItemKind::Const(..), kind: ImplItemKind::Const(..),
owner_id, owner_id,
span,
.. ..
}) if span.ctxt() == ctxt => Some(ExprUseCtxt { }) => ExprUseNode::ConstStatic(owner_id),
node: ExprUseNode::ConstStatic(owner_id),
adjustments,
is_ty_unified,
moved_before_use,
}),
Node::Item(&Item { Node::Item(&Item {
kind: ItemKind::Fn(..), kind: ItemKind::Fn(..),
owner_id, owner_id,
span,
.. ..
}) })
| Node::TraitItem(&TraitItem { | Node::TraitItem(&TraitItem {
kind: TraitItemKind::Fn(..), kind: TraitItemKind::Fn(..),
owner_id, owner_id,
span,
.. ..
}) })
| Node::ImplItem(&ImplItem { | Node::ImplItem(&ImplItem {
kind: ImplItemKind::Fn(..), kind: ImplItemKind::Fn(..),
owner_id, owner_id,
span,
.. ..
}) if span.ctxt() == ctxt => Some(ExprUseCtxt { }) => ExprUseNode::Return(owner_id),
node: ExprUseNode::Return(owner_id),
adjustments,
is_ty_unified,
moved_before_use,
}),
Node::ExprField(field) if field.span.ctxt() == ctxt => Some(ExprUseCtxt { Node::Expr(use_expr) => match use_expr.kind {
node: ExprUseNode::Field(field), ExprKind::Ret(_) => ExprUseNode::Return(OwnerId {
adjustments, def_id: cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()),
is_ty_unified,
moved_before_use,
}),
Node::Expr(parent) if parent.span.ctxt() == ctxt => match parent.kind {
ExprKind::Ret(_) => Some(ExprUseCtxt {
node: ExprUseNode::Return(OwnerId {
def_id: cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()),
}),
adjustments,
is_ty_unified,
moved_before_use,
}), }),
ExprKind::Closure(closure) => Some(ExprUseCtxt { ExprKind::Closure(closure) => ExprUseNode::Return(OwnerId { def_id: closure.def_id }),
node: ExprUseNode::Return(OwnerId { def_id: closure.def_id }), ExprKind::Call(func, args) => match args.iter().position(|arg| arg.hir_id == child_id) {
adjustments, Some(i) => ExprUseNode::FnArg(func, i),
is_ty_unified, None => ExprUseNode::Callee,
moved_before_use,
}),
ExprKind::Call(func, args) => Some(ExprUseCtxt {
node: match args.iter().position(|arg| arg.hir_id == child_id) {
Some(i) => ExprUseNode::FnArg(func, i),
None => ExprUseNode::Callee,
},
adjustments,
is_ty_unified,
moved_before_use,
}),
ExprKind::MethodCall(name, _, args, _) => Some(ExprUseCtxt {
node: ExprUseNode::MethodArg(
parent.hir_id,
name.args,
args.iter().position(|arg| arg.hir_id == child_id).map_or(0, |i| i + 1),
),
adjustments,
is_ty_unified,
moved_before_use,
}),
ExprKind::Field(child, name) if child.hir_id == e.hir_id => Some(ExprUseCtxt {
node: ExprUseNode::FieldAccess(name),
adjustments,
is_ty_unified,
moved_before_use,
}),
ExprKind::If(e, _, _) | ExprKind::Match(e, _, _) if e.hir_id != child_id => {
is_ty_unified = true;
moved_before_use = true;
None
}, },
ExprKind::Block(_, Some(_)) | ExprKind::Break(..) => { ExprKind::MethodCall(name, _, args, _) => ExprUseNode::MethodArg(
is_ty_unified = true; use_expr.hir_id,
moved_before_use = true; name.args,
None args.iter().position(|arg| arg.hir_id == child_id).map_or(0, |i| i + 1),
}, ),
ExprKind::Block(..) => { ExprKind::Field(child, name) if child.hir_id == e.hir_id => ExprUseNode::FieldAccess(name),
moved_before_use = true; _ => ExprUseNode::Expr,
None
},
_ => None,
}, },
_ => None, _ => ExprUseNode::Other,
};
ExprUseCtxt {
node,
adjustments,
is_ty_unified,
moved_before_use,
} }
}) })
} }

View file

@ -1,6 +1,11 @@
//@aux-build:proc_macro_derive.rs //@aux-build:proc_macro_derive.rs
#![warn(clippy::ignored_unit_patterns)] #![warn(clippy::ignored_unit_patterns)]
#![allow(clippy::let_unit_value, clippy::redundant_pattern_matching, clippy::single_match)] #![allow(
clippy::let_unit_value,
clippy::redundant_pattern_matching,
clippy::single_match,
clippy::needless_borrow
)]
fn foo() -> Result<(), ()> { fn foo() -> Result<(), ()> {
unimplemented!() unimplemented!()

View file

@ -1,6 +1,11 @@
//@aux-build:proc_macro_derive.rs //@aux-build:proc_macro_derive.rs
#![warn(clippy::ignored_unit_patterns)] #![warn(clippy::ignored_unit_patterns)]
#![allow(clippy::let_unit_value, clippy::redundant_pattern_matching, clippy::single_match)] #![allow(
clippy::let_unit_value,
clippy::redundant_pattern_matching,
clippy::single_match,
clippy::needless_borrow
)]
fn foo() -> Result<(), ()> { fn foo() -> Result<(), ()> {
unimplemented!() unimplemented!()

View file

@ -1,5 +1,5 @@
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:11:12 --> $DIR/ignored_unit_patterns.rs:16:12
| |
LL | Ok(_) => {}, LL | Ok(_) => {},
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
@ -8,49 +8,49 @@ LL | Ok(_) => {},
= help: to override `-D warnings` add `#[allow(clippy::ignored_unit_patterns)]` = help: to override `-D warnings` add `#[allow(clippy::ignored_unit_patterns)]`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:12:13 --> $DIR/ignored_unit_patterns.rs:17:13
| |
LL | Err(_) => {}, LL | Err(_) => {},
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:14:15 --> $DIR/ignored_unit_patterns.rs:19:15
| |
LL | if let Ok(_) = foo() {} LL | if let Ok(_) = foo() {}
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:16:28 --> $DIR/ignored_unit_patterns.rs:21:28
| |
LL | let _ = foo().map_err(|_| todo!()); LL | let _ = foo().map_err(|_| todo!());
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:22:16 --> $DIR/ignored_unit_patterns.rs:27:16
| |
LL | Ok(_) => {}, LL | Ok(_) => {},
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:24:17 --> $DIR/ignored_unit_patterns.rs:29:17
| |
LL | Err(_) => {}, LL | Err(_) => {},
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:36:9 --> $DIR/ignored_unit_patterns.rs:41:9
| |
LL | let _ = foo().unwrap(); LL | let _ = foo().unwrap();
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:45:13 --> $DIR/ignored_unit_patterns.rs:50:13
| |
LL | (1, _) => unimplemented!(), LL | (1, _) => unimplemented!(),
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`
error: matching over `()` is more explicit error: matching over `()` is more explicit
--> $DIR/ignored_unit_patterns.rs:52:13 --> $DIR/ignored_unit_patterns.rs:57:13
| |
LL | for (x, _) in v { LL | for (x, _) in v {
| ^ help: use `()` instead of `_`: `()` | ^ help: use `()` instead of `_`: `()`

View file

@ -131,6 +131,9 @@ fn main() {
0 0
} }
} }
// issue #11786
let x: (&str,) = ("",);
} }
#[allow(clippy::needless_borrowed_reference)] #[allow(clippy::needless_borrowed_reference)]

View file

@ -131,6 +131,9 @@ fn main() {
0 0
} }
} }
// issue #11786
let x: (&str,) = (&"",);
} }
#[allow(clippy::needless_borrowed_reference)] #[allow(clippy::needless_borrowed_reference)]

View file

@ -121,41 +121,47 @@ error: this expression creates a reference which is immediately dereferenced by
LL | (&&5).foo(); LL | (&&5).foo();
| ^^^^^ help: change this to: `(&5)` | ^^^^^ help: change this to: `(&5)`
error: this expression creates a reference which is immediately dereferenced by the compiler
--> $DIR/needless_borrow.rs:136:23
|
LL | let x: (&str,) = (&"",);
| ^^^ help: change this to: `""`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:175:13 --> $DIR/needless_borrow.rs:178:13
| |
LL | (&self.f)() LL | (&self.f)()
| ^^^^^^^^^ help: change this to: `(self.f)` | ^^^^^^^^^ help: change this to: `(self.f)`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:184:13 --> $DIR/needless_borrow.rs:187:13
| |
LL | (&mut self.f)() LL | (&mut self.f)()
| ^^^^^^^^^^^^^ help: change this to: `(self.f)` | ^^^^^^^^^^^^^ help: change this to: `(self.f)`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:221:22 --> $DIR/needless_borrow.rs:224:22
| |
LL | let _ = &mut (&mut { x.u }).x; LL | let _ = &mut (&mut { x.u }).x;
| ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:228:22 --> $DIR/needless_borrow.rs:231:22
| |
LL | let _ = &mut (&mut { x.u }).x; LL | let _ = &mut (&mut { x.u }).x;
| ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:232:22 --> $DIR/needless_borrow.rs:235:22
| |
LL | let _ = &mut (&mut x.u).x; LL | let _ = &mut (&mut x.u).x;
| ^^^^^^^^^^ help: change this to: `x.u` | ^^^^^^^^^^ help: change this to: `x.u`
error: this expression borrows a value the compiler would automatically borrow error: this expression borrows a value the compiler would automatically borrow
--> $DIR/needless_borrow.rs:233:22 --> $DIR/needless_borrow.rs:236:22
| |
LL | let _ = &mut (&mut { x.u }).x; LL | let _ = &mut (&mut { x.u }).x;
| ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }`
error: aborting due to 26 previous errors error: aborting due to 27 previous errors