diff --git a/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs b/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs index 45a8ef0adb6a7..6b582dfaaac1d 100644 --- a/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs +++ b/compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs @@ -13,7 +13,7 @@ use rustc_infer::infer::NllRegionVariableOrigin; use rustc_middle::middle::resolve_bound_vars::ObjectLifetimeDefault; use rustc_middle::mir::{ Body, CallSource, CastKind, ConstraintCategory, FakeReadCause, Local, LocalInfo, Location, - Operand, Place, Rvalue, Statement, StatementKind, TerminatorKind, + Operand, Place, Rvalue, Statement, StatementKind, TailResultIgnored, TerminatorKind, }; use rustc_middle::ty::adjustment::PointerCoercion; use rustc_middle::ty::{self, RegionVid, Ty, TyCtxt}; @@ -247,7 +247,8 @@ impl<'tcx> BorrowExplanation<'tcx> { err.span_label(body.source_info(drop_loc).span, message); if let LocalInfo::BlockTailTemp(info) = local_decl.local_info() { - if info.tail_result_is_ignored { + // #133941: Don't suggest adding a semicolon if one already exists. + if info.tail_result_is_ignored == TailResultIgnored::TrueNoSemi { // #85581: If the first mutable borrow's scope contains // the second borrow, this suggestion isn't helpful. if !multiple_borrow_span.is_some_and(|(old, new)| { diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 56340ff009553..eb0c5ec197e7c 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -944,6 +944,20 @@ mod binding_form_impl { } } +/// Describes whether the value of a block's tail expression is +/// ignored by the block's expression context, e.g. `let _ = { ...; tail }`, +/// as well as whether the block was followed by a trailing semicolon; this +/// is used to provide context for writing diagnostics. +#[derive(Copy, Clone, Debug, PartialEq, Eq, TyEncodable, TyDecodable, HashStable)] +pub enum TailResultIgnored { + /// The result is ignored, and the block has a trailing semicolon. + TrueWithSemi, + /// The result is ignored, and the block doesn't have a trailing semicolon. + TrueNoSemi, + /// The result is not ignored. + False, +} + /// `BlockTailInfo` is attached to the `LocalDecl` for temporaries /// created during evaluation of expressions in a block tail /// expression; that is, a block like `{ STMT_1; STMT_2; EXPR }`. @@ -954,12 +968,8 @@ mod binding_form_impl { /// one might revise the code to satisfy the borrow checker's rules. #[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable)] pub struct BlockTailInfo { - /// If `true`, then the value resulting from evaluating this tail - /// expression is ignored by the block's expression context. - /// - /// Examples include `{ ...; tail };` and `let _ = { ...; tail };` - /// but not e.g., `let _x = { ...; tail };` - pub tail_result_is_ignored: bool, + /// Whether the result of the tail expression is ignored. + pub tail_result_is_ignored: TailResultIgnored, /// `Span` of the tail expression. pub span: Span, diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs index 86014c34b4584..52143515bb047 100644 --- a/compiler/rustc_middle/src/thir.rs +++ b/compiler/rustc_middle/src/thir.rs @@ -216,6 +216,10 @@ pub enum StmtKind<'tcx> { /// The expression being evaluated in this statement. expr: ExprId, + + /// Whether the statement originally ended with a semicolon in the source + /// code, e.g. `while let ... = ... {};` vs `while let ... = ... {}`. + semi: bool, }, /// A `let` binding. diff --git a/compiler/rustc_middle/src/thir/visit.rs b/compiler/rustc_middle/src/thir/visit.rs index 64bac12b2666a..6f3ef3cb5fcea 100644 --- a/compiler/rustc_middle/src/thir/visit.rs +++ b/compiler/rustc_middle/src/thir/visit.rs @@ -187,7 +187,7 @@ pub fn walk_stmt<'thir, 'tcx: 'thir, V: Visitor<'thir, 'tcx>>( stmt: &'thir Stmt<'tcx>, ) { match &stmt.kind { - StmtKind::Expr { expr, scope: _ } => visitor.visit_expr(&visitor.thir()[*expr]), + StmtKind::Expr { expr, scope: _, semi: _ } => visitor.visit_expr(&visitor.thir()[*expr]), StmtKind::Let { initializer, remainder_scope: _, diff --git a/compiler/rustc_mir_build/src/build/block.rs b/compiler/rustc_mir_build/src/build/block.rs index 89e64015bc439..1e210364c98d7 100644 --- a/compiler/rustc_mir_build/src/build/block.rs +++ b/compiler/rustc_mir_build/src/build/block.rs @@ -69,8 +69,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { for stmt in stmts { let Stmt { ref kind } = this.thir[*stmt]; match kind { - StmtKind::Expr { scope, expr } => { - this.block_context.push(BlockFrame::Statement { ignores_expr_result: true }); + StmtKind::Expr { scope, expr, semi } => { + this.block_context + .push(BlockFrame::Statement { ignores_expr_result: true, semi: *semi }); let si = (*scope, source_info); block = this .in_scope(si, LintLevel::Inherited, |this| { @@ -156,7 +157,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // └────────────────┘ let ignores_expr_result = matches!(pattern.kind, PatKind::Wild); - this.block_context.push(BlockFrame::Statement { ignores_expr_result }); + this.block_context + .push(BlockFrame::Statement { ignores_expr_result, semi: true }); // Lower the `else` block first because its parent scope is actually // enclosing the rest of the `let .. else ..` parts. @@ -251,7 +253,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { span: _, } => { let ignores_expr_result = matches!(pattern.kind, PatKind::Wild); - this.block_context.push(BlockFrame::Statement { ignores_expr_result }); + this.block_context + .push(BlockFrame::Statement { ignores_expr_result, semi: true }); // Enter the remainder scope, i.e., the bindings' destruction scope. this.push_scope((*remainder_scope, source_info)); @@ -329,8 +332,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let destination_ty = destination.ty(&this.local_decls, tcx).ty; if let Some(expr_id) = expr { let expr = &this.thir[expr_id]; - let tail_result_is_ignored = - destination_ty.is_unit() || this.block_context.currently_ignores_tail_results(); + let tail_result_is_ignored = match this.block_context.currently_ignores_tail_results() { + TailResultIgnored::TrueWithSemi => TailResultIgnored::TrueWithSemi, + TailResultIgnored::TrueNoSemi => TailResultIgnored::TrueNoSemi, + TailResultIgnored::False => { + if destination_ty.is_unit() { + TailResultIgnored::TrueNoSemi + } else { + TailResultIgnored::False + } + } + }; this.block_context .push(BlockFrame::TailExpr { tail_result_is_ignored, span: expr.span }); diff --git a/compiler/rustc_mir_build/src/build/expr/stmt.rs b/compiler/rustc_mir_build/src/build/expr/stmt.rs index 15ee6dd014ce9..d34d95605af6c 100644 --- a/compiler/rustc_mir_build/src/build/expr/stmt.rs +++ b/compiler/rustc_mir_build/src/build/expr/stmt.rs @@ -163,10 +163,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { _ => break, } } - this.block_context.push(BlockFrame::TailExpr { - tail_result_is_ignored: true, - span: expr.span, - }); + + let tail_result_is_ignored = + this.block_context.currently_ignores_tail_results(); + + this.block_context + .push(BlockFrame::TailExpr { tail_result_is_ignored, span: expr.span }); Some(expr.span) } else { None diff --git a/compiler/rustc_mir_build/src/build/mod.rs b/compiler/rustc_mir_build/src/build/mod.rs index f43c29d8f5d68..0df6f5c6adbe5 100644 --- a/compiler/rustc_mir_build/src/build/mod.rs +++ b/compiler/rustc_mir_build/src/build/mod.rs @@ -107,6 +107,10 @@ enum BlockFrame { /// If true, then statement discards result from evaluating /// the expression (such as examples 1 and 2 above). ignores_expr_result: bool, + + /// If true, then statement was originally written with a trailing + /// semicolon. + semi: bool, }, /// Evaluation is currently within the tail expression of a block. @@ -117,7 +121,7 @@ enum BlockFrame { /// the result of evaluating the block's tail expression. /// /// Example: `let _ = { STMT_1; EXPR };` - tail_result_is_ignored: bool, + tail_result_is_ignored: TailResultIgnored, /// `Span` of the tail expression. span: Span, @@ -287,24 +291,31 @@ impl BlockContext { } /// Looks at the topmost frame on the BlockContext and reports - /// whether its one that would discard a block tail result. + /// whether it's one that would discard a block tail result. /// /// Unlike `currently_within_ignored_tail_expression`, this does /// *not* skip over `SubExpr` frames: here, we want to know /// whether the block result itself is discarded. - fn currently_ignores_tail_results(&self) -> bool { + fn currently_ignores_tail_results(&self) -> TailResultIgnored { match self.0.last() { // no context: conservatively assume result is read - None => false, + None => TailResultIgnored::False, // sub-expression: block result feeds into some computation - Some(BlockFrame::SubExpr) => false, + Some(BlockFrame::SubExpr) => TailResultIgnored::False, // otherwise: use accumulated is_ignored state. - Some( - BlockFrame::TailExpr { tail_result_is_ignored: ignored, .. } - | BlockFrame::Statement { ignores_expr_result: ignored }, - ) => *ignored, + Some(BlockFrame::TailExpr { tail_result_is_ignored: ignored, .. }) => *ignored, + + Some(BlockFrame::Statement { ignores_expr_result: ignored, semi }) => { + if !ignored { + TailResultIgnored::False + } else if *semi { + TailResultIgnored::TrueWithSemi + } else { + TailResultIgnored::TrueNoSemi + } + } } } } diff --git a/compiler/rustc_mir_build/src/thir/cx/block.rs b/compiler/rustc_mir_build/src/thir/cx/block.rs index 069c2e7881ea6..c04eca5f05f8f 100644 --- a/compiler/rustc_mir_build/src/thir/cx/block.rs +++ b/compiler/rustc_mir_build/src/thir/cx/block.rs @@ -47,7 +47,7 @@ impl<'tcx> Cx<'tcx> { .filter_map(|(index, stmt)| { let hir_id = stmt.hir_id; match stmt.kind { - hir::StmtKind::Expr(expr) | hir::StmtKind::Semi(expr) => { + hir::StmtKind::Expr(expr) => { let stmt = Stmt { kind: StmtKind::Expr { scope: region::Scope { @@ -55,6 +55,20 @@ impl<'tcx> Cx<'tcx> { data: region::ScopeData::Node, }, expr: self.mirror_expr(expr), + semi: false, + }, + }; + Some(self.thir.stmts.push(stmt)) + } + hir::StmtKind::Semi(expr) => { + let stmt = Stmt { + kind: StmtKind::Expr { + scope: region::Scope { + id: hir_id.local_id, + data: region::ScopeData::Node, + }, + expr: self.mirror_expr(expr), + semi: true, }, }; Some(self.thir.stmts.push(stmt)) diff --git a/compiler/rustc_mir_build/src/thir/print.rs b/compiler/rustc_mir_build/src/thir/print.rs index 2bcdb67c58a98..9834705ca11fa 100644 --- a/compiler/rustc_mir_build/src/thir/print.rs +++ b/compiler/rustc_mir_build/src/thir/print.rs @@ -128,11 +128,12 @@ impl<'a, 'tcx> ThirPrinter<'a, 'tcx> { print_indented!(self, "Stmt {", depth_lvl); match kind { - StmtKind::Expr { scope, expr } => { + StmtKind::Expr { scope, expr, semi } => { print_indented!(self, "kind: Expr {", depth_lvl + 1); print_indented!(self, format!("scope: {:?}", scope), depth_lvl + 2); print_indented!(self, "expr:", depth_lvl + 2); self.print_expr(*expr, depth_lvl + 3); + print_indented!(self, format!("semi: {}", semi), depth_lvl + 2); print_indented!(self, "}", depth_lvl + 1); } StmtKind::Let { diff --git a/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.rs b/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.rs new file mode 100644 index 0000000000000..6a98690b62eb2 --- /dev/null +++ b/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.rs @@ -0,0 +1,31 @@ +// Regression test for #133941: Don't suggest adding a semicolon during borrowck +// errors when one already exists. + +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() { + //~^ NOTE first mutable borrow occurs here + //~| a temporary with access to the first borrow is created here ... + foo.g(); //~ ERROR cannot borrow `foo` as mutable more than once at a time + //~^ second mutable borrow occurs here + }; + //~^ ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option>` +} diff --git a/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.stderr b/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.stderr new file mode 100644 index 0000000000000..c4b9b19e5d176 --- /dev/null +++ b/tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.stderr @@ -0,0 +1,18 @@ +error[E0499]: cannot borrow `foo` as mutable more than once at a time + --> $DIR/do-not-suggest-duplicate-semicolons-issue-133941.rs:27:9 + | +LL | while let Some(_) = 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: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0499`.