Skip to content

Don't suggest a semicolon when one already exists #134247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs
Original file line number Diff line number Diff line change
@@ -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)| {
22 changes: 16 additions & 6 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -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,
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/thir/visit.rs
Original file line number Diff line number Diff line change
@@ -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: _,
24 changes: 18 additions & 6 deletions compiler/rustc_mir_build/src/build/block.rs
Original file line number Diff line number Diff line change
@@ -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 });

10 changes: 6 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/stmt.rs
Original file line number Diff line number Diff line change
@@ -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
29 changes: 20 additions & 9 deletions compiler/rustc_mir_build/src/build/mod.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
}
}
}
16 changes: 15 additions & 1 deletion compiler/rustc_mir_build/src/thir/cx/block.rs
Original file line number Diff line number Diff line change
@@ -47,14 +47,28 @@ 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 {
id: hir_id.local_id,
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))
3 changes: 2 additions & 1 deletion compiler/rustc_mir_build/src/thir/print.rs
Original file line number Diff line number Diff line change
@@ -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 {
Original file line number Diff line number Diff line change
@@ -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<Bar<'_>> {
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<Bar<'_>>`
}
Original file line number Diff line number Diff line change
@@ -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<Bar<'_>>`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0499`.