From ecb2d5c43dc0f8bed05e23b6d84396457a376d4e Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 1 Feb 2025 14:35:18 -0700 Subject: [PATCH] diagnostics: fix borrowck suggestions for if/while let conditionals This code detects the case where one of the borrows is inside the let init expr while the other end is not. If that happens, we don't want to suggest adding a semicolon, because it won't work. --- .../src/diagnostics/explain_borrow.rs | 110 +++++++++++++++--- ...ready-borrowed-as-mutable-if-let-133941.rs | 46 ++++++++ ...y-borrowed-as-mutable-if-let-133941.stderr | 89 ++++++++++++++ 3 files changed, 227 insertions(+), 18 deletions(-) create mode 100644 tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.rs create mode 100644 tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs b/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs index 2656e0bb6a452..841cb782bc3bd 100644 --- a/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs +++ b/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs @@ -248,7 +248,98 @@ impl<'tcx> BorrowExplanation<'tcx> { ); err.span_label(body.source_info(drop_loc).span, message); - if let LocalInfo::BlockTailTemp(info) = local_decl.local_info() { + struct FindLetExpr<'hir> { + span: Span, + result: Option<(Span, &'hir hir::Pat<'hir>, &'hir hir::Expr<'hir>)>, + tcx: TyCtxt<'hir>, + } + + impl<'hir> rustc_hir::intravisit::Visitor<'hir> for FindLetExpr<'hir> { + type NestedFilter = rustc_middle::hir::nested_filter::OnlyBodies; + fn nested_visit_map(&mut self) -> Self::Map { + self.tcx.hir() + } + fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) { + if let hir::ExprKind::If(cond, _conseq, _alt) + | hir::ExprKind::Loop( + hir::Block { + expr: + Some(&hir::Expr { + kind: hir::ExprKind::If(cond, _conseq, _alt), + .. + }), + .. + }, + _, + hir::LoopSource::While, + _, + ) = expr.kind + && let hir::ExprKind::Let(hir::LetExpr { + init: let_expr_init, + span: let_expr_span, + pat: let_expr_pat, + .. + }) = cond.kind + && let_expr_init.span.contains(self.span) + { + self.result = + Some((*let_expr_span, let_expr_pat, let_expr_init)) + } else { + hir::intravisit::walk_expr(self, expr); + } + } + } + + if let &LocalInfo::IfThenRescopeTemp { if_then } = local_decl.local_info() + && let hir::Node::Expr(expr) = tcx.hir_node(if_then) + && let hir::ExprKind::If(cond, conseq, alt) = expr.kind + && let hir::ExprKind::Let(&hir::LetExpr { + span: _, + pat, + init, + // FIXME(#101728): enable rewrite when type ascription is + // stabilized again. + ty: None, + recovered: _, + }) = cond.kind + && pat.span.can_be_used_for_suggestions() + && let Ok(pat) = tcx.sess.source_map().span_to_snippet(pat.span) + { + suggest_rewrite_if_let(tcx, expr, &pat, init, conseq, alt, err); + } else if let Some((old, new)) = multiple_borrow_span + && let def_id = body.source.def_id() + && let Some(node) = tcx.hir().get_if_local(def_id) + && let Some(body_id) = node.body_id() + && let hir_body = tcx.hir().body(body_id) + && let mut expr_finder = (FindLetExpr { span: old, result: None, tcx }) + && let Some((let_expr_span, let_expr_pat, let_expr_init)) = { + expr_finder.visit_expr(hir_body.value); + expr_finder.result + } + && !let_expr_span.contains(new) + { + // #133941: The `old` expression is at the conditional part of an + // if/while let expression. Adding a semicolon won't work. + // Instead, try suggesting the `matches!` macro or a temporary. + if let_expr_pat + .walk_short(|pat| !matches!(pat.kind, hir::PatKind::Binding(..))) + { + if let Ok(pat_snippet) = + tcx.sess.source_map().span_to_snippet(let_expr_pat.span) + && let Ok(init_snippet) = + tcx.sess.source_map().span_to_snippet(let_expr_init.span) + { + err.span_suggestion_verbose( + let_expr_span, + "consider using the `matches!` macro", + format!("matches!({init_snippet}, {pat_snippet})"), + Applicability::MaybeIncorrect, + ); + } else { + err.note("consider using the `matches!` macro"); + } + } + } else if let LocalInfo::BlockTailTemp(info) = local_decl.local_info() { if info.tail_result_is_ignored { // #85581: If the first mutable borrow's scope contains // the second borrow, this suggestion isn't helpful. @@ -281,23 +372,6 @@ impl<'tcx> BorrowExplanation<'tcx> { Applicability::MaybeIncorrect, ); }; - } else if let &LocalInfo::IfThenRescopeTemp { if_then } = - local_decl.local_info() - && let hir::Node::Expr(expr) = tcx.hir_node(if_then) - && let hir::ExprKind::If(cond, conseq, alt) = expr.kind - && let hir::ExprKind::Let(&hir::LetExpr { - span: _, - pat, - init, - // FIXME(#101728): enable rewrite when type ascription is - // stabilized again. - ty: None, - recovered: _, - }) = cond.kind - && pat.span.can_be_used_for_suggestions() - && let Ok(pat) = tcx.sess.source_map().span_to_snippet(pat.span) - { - suggest_rewrite_if_let(tcx, expr, &pat, init, conseq, alt, err); } } } diff --git a/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.rs b/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.rs new file mode 100644 index 0000000000000..934aac8383db1 --- /dev/null +++ b/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.rs @@ -0,0 +1,46 @@ +// https://github.com/rust-lang/rust/issues/133941 +use std::marker::PhantomData; + +struct Bar<'a>(PhantomData<&'a mut i32>); + +impl<'a> Drop for Bar<'a> { + fn drop(&mut self) {} +} + +struct Foo(); + +impl Foo { + fn f(&mut self) -> Option> { + None + } + + fn g(&mut self) {} +} + +fn main() { + let mut foo = Foo(); + while let Some(_) = foo.f() { + //~^ HELP matches! + foo.g(); + //~^ ERROR [E0499] + } + if let Some(_) = foo.f() { + //~^ HELP matches! + foo.g(); + //~^ ERROR [E0499] + } + while let Some(_x) = foo.f() { + foo.g(); + //~^ ERROR [E0499] + } + if let Some(_x) = foo.f() { + foo.g(); + //~^ ERROR [E0499] + } + while let Some(_x) = {let _x = foo.f(); foo.g(); None::<()>} { + //~^ ERROR [E0499] + } + if let Some(_x) = {let _x = foo.f(); foo.g(); None::<()>} { + //~^ ERROR [E0499] + } +} diff --git a/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.stderr b/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.stderr new file mode 100644 index 0000000000000..bb21caccbaf91 --- /dev/null +++ b/tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.stderr @@ -0,0 +1,89 @@ +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:24:9 + | +LL | while let Some(_) = foo.f() { + | ------- + | | + | first mutable borrow occurs here + | a temporary with access to the first borrow is created here ... +LL | +LL | foo.g(); + | ^^^ second mutable borrow occurs here +LL | +LL | } + | - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option>` + | +help: consider using the `matches!` macro + | +LL | while matches!(foo.f(), Some(_)) { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:29:9 + | +LL | if let Some(_) = foo.f() { + | ------- + | | + | first mutable borrow occurs here + | a temporary with access to the first borrow is created here ... +LL | +LL | foo.g(); + | ^^^ second mutable borrow occurs here +LL | +LL | } + | - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option>` + | +help: consider using the `matches!` macro + | +LL | if matches!(foo.f(), Some(_)) { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:33:9 + | +LL | while let Some(_x) = foo.f() { + | ------- + | | + | first mutable borrow occurs here + | a temporary with access to the first borrow is created here ... +LL | foo.g(); + | ^^^ second mutable borrow occurs here +LL | +LL | } + | - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option>` + +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:37:9 + | +LL | if let Some(_x) = foo.f() { + | ------- + | | + | first mutable borrow occurs here + | a temporary with access to the first borrow is created here ... +LL | foo.g(); + | ^^^ second mutable borrow occurs here +LL | +LL | } + | - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option>` + +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:40:45 + | +LL | while let Some(_x) = {let _x = foo.f(); foo.g(); None::<()>} { + | --- ^^^ - first borrow might be used here, when `_x` is dropped and runs the destructor for type `Option>` + | | | + | | second mutable borrow occurs here + | first mutable borrow occurs here + +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/already-borrowed-as-mutable-if-let-133941.rs:43:42 + | +LL | if let Some(_x) = {let _x = foo.f(); foo.g(); None::<()>} { + | --- ^^^ - first borrow might be used here, when `_x` is dropped and runs the destructor for type `Option>` + | | | + | | second mutable borrow occurs here + | first mutable borrow occurs here + +error: aborting due to 6 previous errors + +For more information about this error, try `rustc --explain E0499`.