Skip to content
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

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+69 −3
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1105b7f
Don't suggest a semicolon when one already exists
joculatrix Dec 13, 2024
9f62afa
Add regression test
joculatrix Dec 13, 2024
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
23 changes: 20 additions & 3 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 @@ -10,6 +10,7 @@ use rustc_hir as hir;
use rustc_hir::intravisit::Visitor;
use rustc_index::IndexSlice;
use rustc_infer::infer::NllRegionVariableOrigin;
use rustc_middle::dep_graph::DepContext;
use rustc_middle::middle::resolve_bound_vars::ObjectLifetimeDefault;
use rustc_middle::mir::{
Body, CallSource, CastKind, ConstraintCategory, FakeReadCause, Local, LocalInfo, Location,
Expand Down Expand Up @@ -248,11 +249,27 @@ impl<'tcx> BorrowExplanation<'tcx> {

if let LocalInfo::BlockTailTemp(info) = local_decl.local_info() {
if info.tail_result_is_ignored {
// #133941: Don't suggest adding a semicolon if we can
// confirm the expression already ends with one.
let suggest_semicolon = {
let source_map = tcx.sess().source_map();

let hi = info.span.hi();
let span =
info.span.with_lo(hi).with_hi(hi + rustc_span::BytePos(1));

match source_map.span_to_snippet(span) {
Ok(span_str) => !span_str.contains(';'),
Err(_) => true,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably want to change BlockTailInfo::tail_result_is_ignored instead. Using spans for such checks does not work in case the error involves macro calls. The current check may also have issues if something like tail_expr ; or tail_expr/* */; or tail_expr<new-line>; is used

I could imagine that we should either change tail_result_is_ignored to an enum tracking why it's ignored or alternatively change LocalInfo::BlockTailTemp to track whether the tail is an expr or a statement

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! Thanks for the suggestion - I knew someone who knew more than me would steer me in a better direction. I'll look into that. 👍

};
// #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)| {
old.to(info.span.shrink_to_hi()).contains(new)
}) {
if suggest_semicolon
&& !multiple_borrow_span.is_some_and(|(old, new)| {
old.to(info.span.shrink_to_hi()).contains(new)
})
{
err.span_suggestion_verbose(
info.span.shrink_to_hi(),
"consider adding semicolon after the expression so its \
Expand Down
31 changes: 31 additions & 0 deletions tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.rs
View file
Open in desktop
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<'_>>`
}
18 changes: 18 additions & 0 deletions tests/ui/borrowck/do-not-suggest-duplicate-semicolons-issue-133941.stderr
View file
Open in desktop
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`.
Loading