From e17ca31b22f26209bbdf28a95e045aa0da337733 Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Fri, 13 Dec 2024 06:56:49 +0100 Subject: [PATCH 1/4] rustc_borrowck: Make suggest_ampmut() return type match its use So that it becomes easy for a later commit to return `None`. --- .../src/diagnostics/mutability_errors.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index c5ebf3c547e9d..48fc0c331b8bf 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -1150,7 +1150,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { None } None => { - let (has_sugg, decl_span, sugg) = if name != kw::SelfLower { + if name != kw::SelfLower { suggest_ampmut( self.infcx.tcx, local_decl.ty, @@ -1165,7 +1165,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { .. })) => { let sugg = suggest_ampmut_self(self.infcx.tcx, decl_span); - (true, decl_span, sugg) + Some((true, decl_span, sugg, None)) } // explicit self (eg `self: &'a Self`) _ => suggest_ampmut( @@ -1176,8 +1176,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { opt_ty_info, ), } - }; - Some((has_sugg, decl_span, sugg, None)) + } } } } @@ -1443,7 +1442,7 @@ fn suggest_ampmut<'tcx>( decl_span: Span, opt_assignment_rhs_span: Option, opt_ty_info: Option, -) -> (bool, Span, String) { +) -> Option<(bool, Span, String, Option<(Span, String)>)> { // if there is a RHS and it starts with a `&` from it, then check if it is // mutable, and if not, put suggest putting `mut ` to make it mutable. // we don't have to worry about lifetime annotations here because they are @@ -1479,7 +1478,7 @@ fn suggest_ampmut<'tcx>( // FIXME(Ezrashaw): returning is bad because we still might want to // update the annotated type, see #106857. - return (true, span, "mut ".to_owned()); + return Some((true, span, "mut ".to_owned(), None)); } } @@ -1504,18 +1503,18 @@ fn suggest_ampmut<'tcx>( && let Some(ws_pos) = src.find(char::is_whitespace) { let span = span.with_lo(span.lo() + BytePos(ws_pos as u32)).shrink_to_lo(); - (true, span, " mut".to_owned()) + Some((true, span, " mut".to_owned(), None)) // if there is already a binding, we modify it to be `mut` } else if binding_exists { // shrink the span to just after the `&` in `&variable` let span = span.with_lo(span.lo() + BytePos(1)).shrink_to_lo(); - (true, span, "mut ".to_owned()) + Some((true, span, "mut ".to_owned(), None)) } else { // otherwise, suggest that the user annotates the binding; we provide the // type of the local. let ty = decl_ty.builtin_deref(true).unwrap(); - (false, span, format!("{}mut {}", if decl_ty.is_ref() { "&" } else { "*" }, ty)) + Some((false, span, format!("{}mut {}", if decl_ty.is_ref() { "&" } else { "*" }, ty), None)) } } From d7fa8ee6802a2c1121dca0925d3db143dd1a793c Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Fri, 13 Dec 2024 06:44:52 +0100 Subject: [PATCH 2/4] Add regression test for issue 127562 The test fails in this commit. The next commit fixes it. --- ...-mut-suggestion-for-raw-pointer-issue-127562.rs | 8 ++++++++ ...-suggestion-for-raw-pointer-issue-127562.stderr | 14 ++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.rs create mode 100644 tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr diff --git a/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.rs b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.rs new file mode 100644 index 0000000000000..5425e571af061 --- /dev/null +++ b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.rs @@ -0,0 +1,8 @@ +//! Regression test for invalid suggestion for `&raw const expr` reported in +//! . + +fn main() { + let val = 2; + let ptr = &raw const val; + unsafe { *ptr = 3; } //~ ERROR cannot assign to `*ptr`, which is behind a `*const` pointer +} diff --git a/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr new file mode 100644 index 0000000000000..0da5d15cf7f82 --- /dev/null +++ b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr @@ -0,0 +1,14 @@ +error[E0594]: cannot assign to `*ptr`, which is behind a `*const` pointer + --> $DIR/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.rs:7:14 + | +LL | unsafe { *ptr = 3; } + | ^^^^^^^^ `ptr` is a `*const` pointer, so the data it refers to cannot be written + | +help: consider changing this to be a mutable pointer + | +LL | let ptr = &mut raw const val; + | +++ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0594`. From 2d2c6f2a80167d9f244d8c926fdf3b3aebf3f42f Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Fri, 13 Dec 2024 07:00:08 +0100 Subject: [PATCH 3/4] rustc_borrowck: Stop suggesting the invalid syntax `&mut raw const` A legitimate suggestion would be to change from &raw const val to &raw mut val But until we have figured out how to make that happen we should at least stop suggesting invalid syntax. --- compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs | 5 +++++ ...nvalid-mut-suggestion-for-raw-pointer-issue-127562.stderr | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 48fc0c331b8bf..044a828b34679 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -1455,6 +1455,11 @@ fn suggest_ampmut<'tcx>( && let Ok(src) = tcx.sess.source_map().span_to_snippet(assignment_rhs_span) && let Some(stripped) = src.strip_prefix('&') { + let is_raw_ref = stripped.trim_start().starts_with("raw "); + // We don't support raw refs yet + if is_raw_ref { + return None; + } let is_mut = if let Some(rest) = stripped.trim_start().strip_prefix("mut") { match rest.chars().next() { // e.g. `&mut x` diff --git a/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr index 0da5d15cf7f82..c27dcc19827d6 100644 --- a/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr +++ b/tests/ui/borrowck/no-invalid-mut-suggestion-for-raw-pointer-issue-127562.stderr @@ -3,11 +3,6 @@ error[E0594]: cannot assign to `*ptr`, which is behind a `*const` pointer | LL | unsafe { *ptr = 3; } | ^^^^^^^^ `ptr` is a `*const` pointer, so the data it refers to cannot be written - | -help: consider changing this to be a mutable pointer - | -LL | let ptr = &mut raw const val; - | +++ error: aborting due to 1 previous error From f6cb227f6151abfd44536e613cbf4b800e814a2b Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Fri, 13 Dec 2024 06:51:17 +0100 Subject: [PATCH 4/4] rustc_borrowck: Convert suggest_ampmut() 4-tuple to struct for readability --- .../src/diagnostics/mutability_errors.rs | 59 +++++++++++++++---- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 044a828b34679..4fb7b22f2896e 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -1100,12 +1100,12 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { } let decl_span = local_decl.source_info.span; - let label = match *local_decl.local_info() { + let amp_mut_sugg = match *local_decl.local_info() { LocalInfo::User(mir::BindingForm::ImplicitSelf(_)) => { let suggestion = suggest_ampmut_self(self.infcx.tcx, decl_span); let additional = local_trait.map(|span| (span, suggest_ampmut_self(self.infcx.tcx, span))); - Some((true, decl_span, suggestion, additional)) + Some(AmpMutSugg { has_sugg: true, span: decl_span, suggestion, additional }) } LocalInfo::User(mir::BindingForm::Var(mir::VarBindingForm { @@ -1165,7 +1165,12 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { .. })) => { let sugg = suggest_ampmut_self(self.infcx.tcx, decl_span); - Some((true, decl_span, sugg, None)) + Some(AmpMutSugg { + has_sugg: true, + span: decl_span, + suggestion: sugg, + additional: None, + }) } // explicit self (eg `self: &'a Self`) _ => suggest_ampmut( @@ -1186,15 +1191,24 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { .. })) => { let pattern_span: Span = local_decl.source_info.span; - suggest_ref_mut(self.infcx.tcx, pattern_span) - .map(|span| (true, span, "mut ".to_owned(), None)) + suggest_ref_mut(self.infcx.tcx, pattern_span).map(|span| AmpMutSugg { + has_sugg: true, + span, + suggestion: "mut ".to_owned(), + additional: None, + }) } _ => unreachable!(), }; - match label { - Some((true, err_help_span, suggested_code, additional)) => { + match amp_mut_sugg { + Some(AmpMutSugg { + has_sugg: true, + span: err_help_span, + suggestion: suggested_code, + additional, + }) => { let mut sugg = vec![(err_help_span, suggested_code)]; if let Some(s) = additional { sugg.push(s); @@ -1216,7 +1230,9 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { ); } } - Some((false, err_label_span, message, _)) => { + Some(AmpMutSugg { + has_sugg: false, span: err_label_span, suggestion: message, .. + }) => { let def_id = self.body.source.def_id(); let hir_id = if let Some(local_def_id) = def_id.as_local() && let Some(body) = self.infcx.tcx.hir().maybe_body_owned_by(local_def_id) @@ -1421,6 +1437,13 @@ fn suggest_ampmut_self<'tcx>(tcx: TyCtxt<'tcx>, span: Span) -> String { } } +struct AmpMutSugg { + has_sugg: bool, + span: Span, + suggestion: String, + additional: Option<(Span, String)>, +} + // When we want to suggest a user change a local variable to be a `&mut`, there // are three potential "obvious" things to highlight: // @@ -1442,7 +1465,7 @@ fn suggest_ampmut<'tcx>( decl_span: Span, opt_assignment_rhs_span: Option, opt_ty_info: Option, -) -> Option<(bool, Span, String, Option<(Span, String)>)> { +) -> Option { // if there is a RHS and it starts with a `&` from it, then check if it is // mutable, and if not, put suggest putting `mut ` to make it mutable. // we don't have to worry about lifetime annotations here because they are @@ -1483,7 +1506,12 @@ fn suggest_ampmut<'tcx>( // FIXME(Ezrashaw): returning is bad because we still might want to // update the annotated type, see #106857. - return Some((true, span, "mut ".to_owned(), None)); + return Some(AmpMutSugg { + has_sugg: true, + span, + suggestion: "mut ".to_owned(), + additional: None, + }); } } @@ -1508,18 +1536,23 @@ fn suggest_ampmut<'tcx>( && let Some(ws_pos) = src.find(char::is_whitespace) { let span = span.with_lo(span.lo() + BytePos(ws_pos as u32)).shrink_to_lo(); - Some((true, span, " mut".to_owned(), None)) + Some(AmpMutSugg { has_sugg: true, span, suggestion: " mut".to_owned(), additional: None }) // if there is already a binding, we modify it to be `mut` } else if binding_exists { // shrink the span to just after the `&` in `&variable` let span = span.with_lo(span.lo() + BytePos(1)).shrink_to_lo(); - Some((true, span, "mut ".to_owned(), None)) + Some(AmpMutSugg { has_sugg: true, span, suggestion: "mut ".to_owned(), additional: None }) } else { // otherwise, suggest that the user annotates the binding; we provide the // type of the local. let ty = decl_ty.builtin_deref(true).unwrap(); - Some((false, span, format!("{}mut {}", if decl_ty.is_ref() { "&" } else { "*" }, ty), None)) + Some(AmpMutSugg { + has_sugg: false, + span, + suggestion: format!("{}mut {}", if decl_ty.is_ref() { "&" } else { "*" }, ty), + additional: None, + }) } }