From c748f32ee45ed42eaab4d6a62dc64720b5096c68 Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Sun, 13 Dec 2020 00:11:15 -0500 Subject: [PATCH] Fix incorrect use mut diagnostics --- compiler/rustc_middle/src/ty/mod.rs | 9 +++++ .../borrow_check/diagnostics/move_errors.rs | 4 ++- .../diagnostics/mutability_errors.rs | 32 +++++++++++++---- .../src/borrow_check/diagnostics/var_name.rs | 7 ++-- compiler/rustc_mir/src/borrow_check/mod.rs | 30 ++++++---------- compiler/rustc_mir/src/borrow_check/nll.rs | 2 +- .../rustc_mir/src/borrow_check/path_utils.rs | 2 +- .../src/borrow_check/type_check/mod.rs | 8 +++-- .../diagnostics/cant-mutate-imm-borrow.rs | 1 - .../diagnostics/cant-mutate-imm-borrow.stderr | 14 ++------ .../diagnostics/cant-mutate-imm.rs | 35 +++++++++++++++++++ .../diagnostics/cant-mutate-imm.stderr | 30 ++++++++++++++++ .../diagnostics/mut_ref.rs | 2 -- .../diagnostics/mut_ref.stderr | 25 ++----------- 14 files changed, 129 insertions(+), 72 deletions(-) create mode 100644 src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.stderr diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index ddb78d91759..7681041863e 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -674,6 +674,15 @@ pub struct CapturedPlace<'tcx> { pub mutability: hir::Mutability, } +impl CapturedPlace<'tcx> { + pub fn get_root_variable(&self) -> hir::HirId { + match self.place.base { + HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id, + base => bug!("Expected upvar, found={:?}", base), + } + } +} + pub fn place_to_string_for_capture(tcx: TyCtxt<'tcx>, place: &HirPlace<'tcx>) -> String { let name = match place.base { HirPlaceBase::Upvar(upvar_id) => tcx.hir().name(upvar_id.var_path.hir_id).to_string(), diff --git a/compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs b/compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs index 350e0d045fa..fb7694b7d88 100644 --- a/compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs +++ b/compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs @@ -345,7 +345,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { }; let upvar = &self.upvars[upvar_field.unwrap().index()]; - let upvar_hir_id = upvar.var_hir_id; + // FIXME(project-rfc-2229#8): Improve borrow-check diagnostics in case of precise + // capture. + let upvar_hir_id = upvar.place.get_root_variable(); let upvar_name = upvar.name; let upvar_span = self.infcx.tcx.hir().span(upvar_hir_id); diff --git a/compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs b/compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs index 73196c732f5..74abe2d35ee 100644 --- a/compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs +++ b/compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs @@ -64,12 +64,29 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty )); - item_msg = format!("`{}`", access_place_desc.unwrap()); - if self.is_upvar_field_projection(access_place.as_ref()).is_some() { - reason = ", as it is not declared as mutable".to_string(); + let imm_borrow_derefed = self.upvars[upvar_index.index()] + .place + .place + .deref_tys() + .any(|ty| matches!(ty.kind(), ty::Ref(.., hir::Mutability::Not))); + + // If the place is immutable then: + // + // - Either we deref a immutable ref to get to our final place. + // - We don't capture derefs of raw ptrs + // - Or the final place is immut because the root variable of the capture + // isn't marked mut and we should suggest that to the user. + if imm_borrow_derefed { + // If we deref an immutable ref then the suggestion here doesn't help. + return; } else { - let name = self.upvars[upvar_index.index()].name; - reason = format!(", as `{}` is not declared as mutable", name); + item_msg = format!("`{}`", access_place_desc.unwrap()); + if self.is_upvar_field_projection(access_place.as_ref()).is_some() { + reason = ", as it is not declared as mutable".to_string(); + } else { + let name = self.upvars[upvar_index.index()].name; + reason = format!(", as `{}` is not declared as mutable", name); + } } } @@ -259,9 +276,12 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty )); + let captured_place = &self.upvars[upvar_index.index()].place; + err.span_label(span, format!("cannot {ACT}", ACT = act)); - let upvar_hir_id = self.upvars[upvar_index.index()].var_hir_id; + let upvar_hir_id = captured_place.get_root_variable(); + if let Some(Node::Binding(pat)) = self.infcx.tcx.hir().find(upvar_hir_id) { if let hir::PatKind::Binding( hir::BindingAnnotation::Unannotated, diff --git a/compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs b/compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs index a850b85e9bb..4abc623fc5f 100644 --- a/compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs +++ b/compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs @@ -12,7 +12,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { tcx: TyCtxt<'tcx>, body: &Body<'tcx>, local_names: &IndexVec>, - upvars: &[Upvar], + upvars: &[Upvar<'tcx>], fr: RegionVid, ) -> Option<(Option, Span)> { debug!("get_var_name_and_span_for_region(fr={:?})", fr); @@ -21,6 +21,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { debug!("get_var_name_and_span_for_region: attempting upvar"); self.get_upvar_index_for_region(tcx, fr) .map(|index| { + // FIXME(project-rfc-2229#8): Use place span for diagnostics let (name, span) = self.get_upvar_name_and_span_for_region(tcx, upvars, index); (Some(name), span) }) @@ -59,10 +60,10 @@ impl<'tcx> RegionInferenceContext<'tcx> { crate fn get_upvar_name_and_span_for_region( &self, tcx: TyCtxt<'tcx>, - upvars: &[Upvar], + upvars: &[Upvar<'tcx>], upvar_index: usize, ) -> (Symbol, Span) { - let upvar_hir_id = upvars[upvar_index].var_hir_id; + let upvar_hir_id = upvars[upvar_index].place.get_root_variable(); debug!("get_upvar_name_and_span_for_region: upvar_hir_id={:?}", upvar_hir_id); let upvar_name = tcx.hir().name(upvar_hir_id); diff --git a/compiler/rustc_mir/src/borrow_check/mod.rs b/compiler/rustc_mir/src/borrow_check/mod.rs index 6e1e5c65aea..b51cba15d32 100644 --- a/compiler/rustc_mir/src/borrow_check/mod.rs +++ b/compiler/rustc_mir/src/borrow_check/mod.rs @@ -5,11 +5,10 @@ use rustc_data_structures::graph::dominators::Dominators; use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorReported}; use rustc_hir as hir; use rustc_hir::def_id::LocalDefId; -use rustc_hir::{HirId, Node}; +use rustc_hir::Node; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; use rustc_infer::infer::{InferCtxt, TyCtxtInferExt}; -use rustc_middle::hir::place::PlaceBase as HirPlaceBase; use rustc_middle::mir::{ traversal, Body, ClearCrossCrate, Local, Location, Mutability, Operand, Place, PlaceElem, PlaceRef, VarDebugInfoContents, @@ -18,7 +17,7 @@ use rustc_middle::mir::{AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind use rustc_middle::mir::{Field, ProjectionElem, Promoted, Rvalue, Statement, StatementKind}; use rustc_middle::mir::{InlineAsmOperand, Terminator, TerminatorKind}; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::{self, ParamEnv, RegionVid, TyCtxt}; +use rustc_middle::ty::{self, CapturedPlace, ParamEnv, RegionVid, TyCtxt}; use rustc_session::lint::builtin::{MUTABLE_BORROW_RESERVATION_CONFLICT, UNUSED_MUT}; use rustc_span::{Span, Symbol, DUMMY_SP}; @@ -73,16 +72,15 @@ crate use region_infer::RegionInferenceContext; // FIXME(eddyb) perhaps move this somewhere more centrally. #[derive(Debug)] -crate struct Upvar { +crate struct Upvar<'tcx> { + // FIXME(project-rfc-2229#8): ty::CapturePlace should have a to_string(), or similar + // then this should not be needed. name: Symbol, - // FIXME(project-rfc-2229#8): This should use Place or something similar - var_hir_id: HirId, + place: CapturedPlace<'tcx>, /// If true, the capture is behind a reference. by_ref: bool, - - mutability: Mutability, } const DEREF_PROJECTION: &[PlaceElem<'_>; 1] = &[ProjectionElem::Deref]; @@ -161,21 +159,13 @@ fn do_mir_borrowck<'a, 'tcx>( let upvars: Vec<_> = tables .closure_min_captures_flattened(def.did.to_def_id()) .map(|captured_place| { - let var_hir_id = match captured_place.place.base { - HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id, - _ => bug!("Expected upvar"), - }; + let var_hir_id = captured_place.get_root_variable(); let capture = captured_place.info.capture_kind; let by_ref = match capture { ty::UpvarCapture::ByValue(_) => false, ty::UpvarCapture::ByRef(..) => true, }; - Upvar { - name: tcx.hir().name(var_hir_id), - var_hir_id, - by_ref, - mutability: captured_place.mutability, - } + Upvar { name: tcx.hir().name(var_hir_id), place: captured_place.clone(), by_ref } }) .collect(); @@ -544,7 +534,7 @@ crate struct MirBorrowckCtxt<'cx, 'tcx> { dominators: Dominators, /// Information about upvars not necessarily preserved in types or MIR - upvars: Vec, + upvars: Vec>, /// Names of local (user) variables (extracted from `var_debug_info`). local_names: IndexVec>, @@ -2251,7 +2241,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { place={:?}", upvar, is_local_mutation_allowed, place ); - match (upvar.mutability, is_local_mutation_allowed) { + match (upvar.place.mutability, is_local_mutation_allowed) { ( Mutability::Not, LocalMutationIsAllowed::No diff --git a/compiler/rustc_mir/src/borrow_check/nll.rs b/compiler/rustc_mir/src/borrow_check/nll.rs index 359c5f261a4..a0265b20d12 100644 --- a/compiler/rustc_mir/src/borrow_check/nll.rs +++ b/compiler/rustc_mir/src/borrow_check/nll.rs @@ -165,7 +165,7 @@ pub(in crate::borrow_check) fn compute_regions<'cx, 'tcx>( flow_inits: &mut ResultsCursor<'cx, 'tcx, MaybeInitializedPlaces<'cx, 'tcx>>, move_data: &MoveData<'tcx>, borrow_set: &BorrowSet<'tcx>, - upvars: &[Upvar], + upvars: &[Upvar<'tcx>], ) -> NllOutput<'tcx> { let mut all_facts = AllFacts::enabled(infcx.tcx).then_some(AllFacts::default()); diff --git a/compiler/rustc_mir/src/borrow_check/path_utils.rs b/compiler/rustc_mir/src/borrow_check/path_utils.rs index fa3ae2367e0..80de3b4e363 100644 --- a/compiler/rustc_mir/src/borrow_check/path_utils.rs +++ b/compiler/rustc_mir/src/borrow_check/path_utils.rs @@ -143,7 +143,7 @@ pub(super) fn borrow_of_local_data(place: Place<'_>) -> bool { /// of a closure type. pub(crate) fn is_upvar_field_projection( tcx: TyCtxt<'tcx>, - upvars: &[Upvar], + upvars: &[Upvar<'tcx>], place_ref: PlaceRef<'tcx>, body: &Body<'tcx>, ) -> Option { diff --git a/compiler/rustc_mir/src/borrow_check/type_check/mod.rs b/compiler/rustc_mir/src/borrow_check/type_check/mod.rs index fb9820e853f..24bbd2b8c49 100644 --- a/compiler/rustc_mir/src/borrow_check/type_check/mod.rs +++ b/compiler/rustc_mir/src/borrow_check/type_check/mod.rs @@ -132,7 +132,7 @@ pub(crate) fn type_check<'mir, 'tcx>( flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>, move_data: &MoveData<'tcx>, elements: &Rc, - upvars: &[Upvar], + upvars: &[Upvar<'tcx>], ) -> MirTypeckResults<'tcx> { let implicit_region_bound = infcx.tcx.mk_region(ty::ReVar(universal_regions.fr_fn_body)); let mut constraints = MirTypeckRegionConstraints { @@ -821,7 +821,7 @@ struct BorrowCheckContext<'a, 'tcx> { all_facts: &'a mut Option, borrow_set: &'a BorrowSet<'tcx>, constraints: &'a mut MirTypeckRegionConstraints<'tcx>, - upvars: &'a [Upvar], + upvars: &'a [Upvar<'tcx>], } crate struct MirTypeckResults<'tcx> { @@ -2490,7 +2490,9 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { body, ); let category = if let Some(field) = field { - ConstraintCategory::ClosureUpvar(self.borrowck_context.upvars[field.index()].var_hir_id) + let var_hir_id = self.borrowck_context.upvars[field.index()].place.get_root_variable(); + // FIXME(project-rfc-2229#8): Use Place for better diagnostics + ConstraintCategory::ClosureUpvar(var_hir_id) } else { ConstraintCategory::Boring }; diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.rs b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.rs index dd018a0defa..1ea38e260b6 100644 --- a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.rs +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.rs @@ -14,7 +14,6 @@ fn main() { let mut c = || { //~^ ERROR: cannot borrow `z.0.0.0` as mutable, as it is behind a `&` reference z.0.0.0 = format!("X1"); - //~^ ERROR: cannot assign to `z`, as it is not declared as mutable }; c(); diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.stderr b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.stderr index 948e2b731da..861bc44b78d 100644 --- a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.stderr +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm-borrow.stderr @@ -7,15 +7,6 @@ LL | #![feature(capture_disjoint_fields)] = note: `#[warn(incomplete_features)]` on by default = note: see issue #53488 for more information -error[E0594]: cannot assign to `z`, as it is not declared as mutable - --> $DIR/cant-mutate-imm-borrow.rs:16:9 - | -LL | let z = (&mut y, "Z"); - | - help: consider changing this to be mutable: `mut z` -... -LL | z.0.0.0 = format!("X1"); - | ^^^^^^^ cannot assign - error[E0596]: cannot borrow `z.0.0.0` as mutable, as it is behind a `&` reference --> $DIR/cant-mutate-imm-borrow.rs:14:17 | @@ -25,7 +16,6 @@ LL | LL | z.0.0.0 = format!("X1"); | - mutable borrow occurs due to use of `z.0.0.0` in closure -error: aborting due to 2 previous errors; 1 warning emitted +error: aborting due to previous error; 1 warning emitted -Some errors have detailed explanations: E0594, E0596. -For more information about an error, try `rustc --explain E0594`. +For more information about this error, try `rustc --explain E0596`. diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs new file mode 100644 index 00000000000..997ecc7dddd --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs @@ -0,0 +1,35 @@ +#![feature(capture_disjoint_fields)] +//~^ WARNING: the feature `capture_disjoint_fields` is incomplete + +// Ensure that diagnostics for mutability error (because the root variable +// isn't mutable) work with `capture_disjoint_fields` enabled. + +fn mut_error_struct() { + let x = (10, 10); + let y = (x, 10); + let z = (y, 10); + + let mut c = || { + z.0.0.0 = 20; + //~^ ERROR: cannot assign to `z`, as it is not declared as mutable + }; + + c(); +} + +fn mut_error_box() { + let x = (10, 10); + let bx = Box::new(x); + + let mut c = || { + bx.0 = 20; + //~^ ERROR: cannot assign to `bx`, as it is not declared as mutable + }; + + c(); +} + +fn main() { + mut_error_struct(); + mut_error_box(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.stderr b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.stderr new file mode 100644 index 00000000000..5e15635ac6e --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.stderr @@ -0,0 +1,30 @@ +warning: the feature `capture_disjoint_fields` is incomplete and may not be safe to use and/or cause compiler crashes + --> $DIR/cant-mutate-imm.rs:1:12 + | +LL | #![feature(capture_disjoint_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(incomplete_features)]` on by default + = note: see issue #53488 for more information + +error[E0594]: cannot assign to `z`, as it is not declared as mutable + --> $DIR/cant-mutate-imm.rs:13:9 + | +LL | let z = (y, 10); + | - help: consider changing this to be mutable: `mut z` +... +LL | z.0.0.0 = 20; + | ^^^^^^^^^^^^ cannot assign + +error[E0594]: cannot assign to `bx`, as it is not declared as mutable + --> $DIR/cant-mutate-imm.rs:25:9 + | +LL | let bx = Box::new(x); + | -- help: consider changing this to be mutable: `mut bx` +... +LL | bx.0 = 20; + | ^^^^^^^^^ cannot assign + +error: aborting due to 2 previous errors; 1 warning emitted + +For more information about this error, try `rustc --explain E0594`. diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.rs b/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.rs index 732c4729824..676fde558df 100644 --- a/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.rs +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.rs @@ -13,7 +13,6 @@ fn imm_mut_ref() { let c = || { //~^ ERROR: cannot borrow `**ref_mref_x` as mutable, as it is behind a `&` reference **ref_mref_x = y; - //~^ERROR: cannot assign to `ref_mref_x`, as it is not declared as mutable }; c(); @@ -28,7 +27,6 @@ fn mut_imm_ref() { let c = || { //~^ ERROR: cannot borrow `**mref_ref_x` as mutable, as it is behind a `&` reference **mref_ref_x = y; - //~^ERROR: cannot assign to `mref_ref_x`, as it is not declared as mutable }; c(); diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.stderr b/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.stderr index 42b3c5090ac..8cb2ed2235d 100644 --- a/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.stderr +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/mut_ref.stderr @@ -7,15 +7,6 @@ LL | #![feature(capture_disjoint_fields)] = note: `#[warn(incomplete_features)]` on by default = note: see issue #53488 for more information -error[E0594]: cannot assign to `ref_mref_x`, as it is not declared as mutable - --> $DIR/mut_ref.rs:15:9 - | -LL | let ref_mref_x = &mref_x; - | ---------- help: consider changing this to be mutable: `mut ref_mref_x` -... -LL | **ref_mref_x = y; - | ^^^^^^^^^^^^ cannot assign - error[E0596]: cannot borrow `**ref_mref_x` as mutable, as it is behind a `&` reference --> $DIR/mut_ref.rs:13:13 | @@ -28,17 +19,8 @@ LL | LL | **ref_mref_x = y; | ---------- mutable borrow occurs due to use of `**ref_mref_x` in closure -error[E0594]: cannot assign to `mref_ref_x`, as it is not declared as mutable - --> $DIR/mut_ref.rs:30:9 - | -LL | let mref_ref_x = &mut ref_x; - | ---------- help: consider changing this to be mutable: `mut mref_ref_x` -... -LL | **mref_ref_x = y; - | ^^^^^^^^^^^^ cannot assign - error[E0596]: cannot borrow `**mref_ref_x` as mutable, as it is behind a `&` reference - --> $DIR/mut_ref.rs:28:13 + --> $DIR/mut_ref.rs:27:13 | LL | let c = || { | ^^ cannot borrow as mutable @@ -46,7 +28,6 @@ LL | LL | **mref_ref_x = y; | ---------- mutable borrow occurs due to use of `**mref_ref_x` in closure -error: aborting due to 4 previous errors; 1 warning emitted +error: aborting due to 2 previous errors; 1 warning emitted -Some errors have detailed explanations: E0594, E0596. -For more information about an error, try `rustc --explain E0594`. +For more information about this error, try `rustc --explain E0596`.