Skip to content

diagnostics: fix borrowck suggestions for if/while let conditionals #136402

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 2, 2025
Merged

diagnostics: fix borrowck suggestions for if/while let conditionals #136402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
110 changes: 92 additions & 18 deletions compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
46 changes: 46 additions & 0 deletions tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Bar<'_>> {
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]
}
}
89 changes: 89 additions & 0 deletions tests/ui/borrowck/already-borrowed-as-mutable-if-let-133941.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Bar<'_>>`
|
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<Bar<'_>>`
|
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<Bar<'_>>`

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<Bar<'_>>`

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<Bar<'_>>`
| | |
| | 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<Bar<'_>>`
| | |
| | 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`.
Loading