Fix false positive of borrow_deref_ref
#14967
Conversation
rustbot
added
the
S-waiting-on-review
clippy_lints/src/borrow_deref_ref.rs
Outdated
| @@ -76,6 +76,9 @@ impl<'tcx> LateLintPass<'tcx> for BorrowDerefRef { | |||
| && let e_ty = cx.typeck_results().expr_ty_adjusted(e) | |||
| // check if the reference is coercing to a mutable reference | |||
| && (!matches!(e_ty.kind(), ty::Ref(_, _, Mutability::Mut)) || is_mutable(cx, deref_target)) | |||
| // If the new borrow might be itself borrowed mutably, do not propose to use | |||
| // the original reference instead. | |||
| && !potentially_bound_to_mutable_ref(cx, e) | |||
There was a problem hiding this comment.
Maybe worth making an exception and still lint if deref_target is not a place expr, so it's consistent with the check for &mut &* a few lines above? That way we can still emit a warning for let ref mut x = &*"string", let ref mut x = &*foo()
There was a problem hiding this comment.
Indeed, deref_target has to designate a temporary value if it is reborrowed mutably.
clippy_lints/src/borrow_deref_ref.rs
Outdated
| matches!(expr_use_ctxt(cx, expr).use_node(cx), | ||
| ExprUseNode::LetStmt(let_stmt) | ||
| if let_stmt.pat.contains_explicit_ref_binding() == Some(Mutability::Mut)) |
There was a problem hiding this comment.
Do you have an example where walking up to the use node here would help over just checking if the direct parent hir node is a let stmt node? It feels like it's doing more work while also introducing FNs (e.g. let y = &1; let ref mut x = { &*y }; was warned before but no longer is) and not actually preventing any FPs I could think of.
AFAIK, expr_use_ctxt/walk_to_expr_usage exclusively only walks through blocks, and in those cases the &*x/x reference is always copied, so the let ref mut would always mutably borrow a temporary reference either way, regardless of whether it's &*x or x?
There was a problem hiding this comment.
You're right, I couldn't produce a counter-example where walking up would help, and using the parent removes the FN. I've added this as a test as well.
samueltardieu
force-pushed
the
issue-14934
branch
from
26ca250 to
681b46e
Compare
If a reborrow is itself borrowed mutably, do not propose to replace it by the original reference.
samueltardieu
force-pushed
the
issue-14934
branch
from
681b46e to
322e139
Compare
If a reborrow is itself borrowed mutably, do not propose to replace it by the original reference.
Fixes: #14934
changelog: [
borrow_deref_ref]: do not propose replacing a reborrow by the original reference if the reborrow is itself mutably borrowed