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
Draft

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

Changes from 1 commit
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
Loading