From 43b5d725d0b458e317c52ef200d323998fa0c20f Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 1 Oct 2018 17:46:04 +0200 Subject: [PATCH 1/2] Improve implicit self mutability suggestions. This commit adds an `ImplicitSelfKind` to the HIR and the MIR that keeps track of whether a implicit self argument is immutable by-value, mutable by-value, immutable reference or mutable reference so that the addition of the `mut` keyword can be suggested for the immutable by-value case. --- src/librustc/hir/lowering.rs | 30 ++++++++++++--- src/librustc/hir/mod.rs | 31 ++++++++++++++-- src/librustc/ich/impls_hir.rs | 10 ++++- src/librustc/mir/mod.rs | 37 +++++++++++++++---- src/librustc_borrowck/borrowck/mod.rs | 2 +- .../borrow_check/mutability_errors.rs | 2 +- src/librustc_mir/build/mod.rs | 18 +++++---- src/test/incremental/hashes/trait_defs.rs | 2 +- src/test/ui/nll/issue-51191.rs | 37 +++++++++++++++++++ src/test/ui/nll/issue-51191.stderr | 37 +++++++++++++++++++ 10 files changed, 180 insertions(+), 26 deletions(-) create mode 100644 src/test/ui/nll/issue-51191.rs create mode 100644 src/test/ui/nll/issue-51191.stderr diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 62b06f54301f3..7f04f58dbb2a4 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -2011,11 +2011,31 @@ impl<'a> LoweringContext<'a> { inputs, output, variadic: decl.variadic, - has_implicit_self: decl.inputs.get(0).map_or(false, |arg| match arg.ty.node { - TyKind::ImplicitSelf => true, - TyKind::Rptr(_, ref mt) => mt.ty.node.is_implicit_self(), - _ => false, - }), + implicit_self: decl.inputs.get(0).map_or( + hir::ImplicitSelfKind::None, + |arg| { + let is_mutable_pat = match arg.pat.node { + PatKind::Ident(BindingMode::ByValue(mt), _, _) | + PatKind::Ident(BindingMode::ByRef(mt), _, _) => + mt == Mutability::Mutable, + _ => false, + }; + + match arg.ty.node { + TyKind::ImplicitSelf if is_mutable_pat => hir::ImplicitSelfKind::Mut, + TyKind::ImplicitSelf => hir::ImplicitSelfKind::Imm, + // Given we are only considering `ImplicitSelf` types, we needn't consider + // the case where we have a mutable pattern to a reference as that would + // no longer be an `ImplicitSelf`. + TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() && + mt.mutbl == ast::Mutability::Mutable => + hir::ImplicitSelfKind::MutRef, + TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() => + hir::ImplicitSelfKind::ImmRef, + _ => hir::ImplicitSelfKind::None, + } + }, + ), }) } diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 088bee3811680..54568dd8f5ff3 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -1769,9 +1769,34 @@ pub struct FnDecl { pub inputs: HirVec, pub output: FunctionRetTy, pub variadic: bool, - /// True if this function has an `self`, `&self` or `&mut self` receiver - /// (but not a `self: Xxx` one). - pub has_implicit_self: bool, + /// Does the function have an implicit self? + pub implicit_self: ImplicitSelfKind, +} + +/// Represents what type of implicit self a function has, if any. +#[derive(Clone, Copy, RustcEncodable, RustcDecodable, Debug)] +pub enum ImplicitSelfKind { + /// Represents a `fn x(self);`. + Imm, + /// Represents a `fn x(mut self);`. + Mut, + /// Represents a `fn x(&self);`. + ImmRef, + /// Represents a `fn x(&mut self);`. + MutRef, + /// Represents when a function does not have a self argument or + /// when a function has a `self: X` argument. + None +} + +impl ImplicitSelfKind { + /// Does this represent an implicit self? + pub fn has_implicit_self(&self) -> bool { + match *self { + ImplicitSelfKind::None => false, + _ => true, + } + } } /// Is the trait definition an auto trait? diff --git a/src/librustc/ich/impls_hir.rs b/src/librustc/ich/impls_hir.rs index bc2eb5f442b47..2097e4faf0773 100644 --- a/src/librustc/ich/impls_hir.rs +++ b/src/librustc/ich/impls_hir.rs @@ -349,7 +349,7 @@ impl_stable_hash_for!(struct hir::FnDecl { inputs, output, variadic, - has_implicit_self + implicit_self }); impl_stable_hash_for!(enum hir::FunctionRetTy { @@ -357,6 +357,14 @@ impl_stable_hash_for!(enum hir::FunctionRetTy { Return(t) }); +impl_stable_hash_for!(enum hir::ImplicitSelfKind { + Imm, + Mut, + ImmRef, + MutRef, + None +}); + impl_stable_hash_for!(struct hir::TraitRef { // Don't hash the ref_id. It is tracked via the thing it is used to access ref_id -> _, diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 3e6246f2ea8d8..ca67b0f04de14 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -583,11 +583,27 @@ pub enum BindingForm<'tcx> { /// This is a binding for a non-`self` binding, or a `self` that has an explicit type. Var(VarBindingForm<'tcx>), /// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit. - ImplicitSelf, + ImplicitSelf(ImplicitSelfKind), /// Reference used in a guard expression to ensure immutability. RefForGuard, } +/// Represents what type of implicit self a function has, if any. +#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)] +pub enum ImplicitSelfKind { + /// Represents a `fn x(self);`. + Imm, + /// Represents a `fn x(mut self);`. + Mut, + /// Represents a `fn x(&self);`. + ImmRef, + /// Represents a `fn x(&mut self);`. + MutRef, + /// Represents when a function does not have a self argument or + /// when a function has a `self: X` argument. + None +} + CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, } impl_stable_hash_for!(struct self::VarBindingForm<'tcx> { @@ -597,6 +613,14 @@ impl_stable_hash_for!(struct self::VarBindingForm<'tcx> { pat_span }); +impl_stable_hash_for!(enum self::ImplicitSelfKind { + Imm, + Mut, + ImmRef, + MutRef, + None +}); + mod binding_form_impl { use ich::StableHashingContext; use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableHasherResult}; @@ -612,7 +636,7 @@ mod binding_form_impl { match self { Var(binding) => binding.hash_stable(hcx, hasher), - ImplicitSelf => (), + ImplicitSelf(kind) => kind.hash_stable(hcx, hasher), RefForGuard => (), } } @@ -775,10 +799,9 @@ impl<'tcx> LocalDecl<'tcx> { pat_span: _, }))) => true, - // FIXME: might be able to thread the distinction between - // `self`/`mut self`/`&self`/`&mut self` into the - // `BindingForm::ImplicitSelf` variant, (and then return - // true here for solely the first case). + Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(ImplicitSelfKind::Imm))) + => true, + _ => false, } } @@ -795,7 +818,7 @@ impl<'tcx> LocalDecl<'tcx> { pat_span: _, }))) => true, - Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf)) => true, + Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(_))) => true, _ => false, } diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs index fb8744e4d96d6..321257aefdb5a 100644 --- a/src/librustc_borrowck/borrowck/mod.rs +++ b/src/librustc_borrowck/borrowck/mod.rs @@ -1221,7 +1221,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { if let Some(i) = arg_pos { // The argument's `Ty` (Some(&fn_like.decl().inputs[i]), - i == 0 && fn_like.decl().has_implicit_self) + i == 0 && fn_like.decl().implicit_self.has_implicit_self()) } else { (None, false) } diff --git a/src/librustc_mir/borrow_check/mutability_errors.rs b/src/librustc_mir/borrow_check/mutability_errors.rs index 30555dbf26082..1d33d9cc0c452 100644 --- a/src/librustc_mir/borrow_check/mutability_errors.rs +++ b/src/librustc_mir/borrow_check/mutability_errors.rs @@ -316,7 +316,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> { { let local_decl = &self.mir.local_decls[*local]; let suggestion = match local_decl.is_user_variable.as_ref().unwrap() { - ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf) => { + ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf(_)) => { Some(suggest_ampmut_self(self.infcx.tcx, local_decl)) } diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index f75ce7b08162d..3dbd3bbb41573 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -125,8 +125,14 @@ pub fn mir_build<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Mir<'t let ty_hir_id = fn_decl.inputs[index].hir_id; let ty_span = tcx.hir.span(tcx.hir.hir_to_node_id(ty_hir_id)); opt_ty_info = Some(ty_span); - self_arg = if index == 0 && fn_decl.has_implicit_self { - Some(ImplicitSelfBinding) + self_arg = if index == 0 && fn_decl.implicit_self.has_implicit_self() { + match fn_decl.implicit_self { + hir::ImplicitSelfKind::Imm => Some(ImplicitSelfKind::Imm), + hir::ImplicitSelfKind::Mut => Some(ImplicitSelfKind::Mut), + hir::ImplicitSelfKind::ImmRef => Some(ImplicitSelfKind::ImmRef), + hir::ImplicitSelfKind::MutRef => Some(ImplicitSelfKind::MutRef), + _ => None, + } } else { None }; @@ -508,12 +514,10 @@ fn should_abort_on_panic<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, /////////////////////////////////////////////////////////////////////////// /// the main entry point for building MIR for a function -struct ImplicitSelfBinding; - struct ArgInfo<'gcx>(Ty<'gcx>, Option, Option<&'gcx hir::Pat>, - Option); + Option); fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>, fn_id: ast::NodeId, @@ -797,8 +801,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { PatternKind::Binding { mutability, var, mode: BindingMode::ByValue, .. } => { self.local_decls[local].mutability = mutability; self.local_decls[local].is_user_variable = - if let Some(ImplicitSelfBinding) = self_binding { - Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf)) + if let Some(kind) = self_binding { + Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(*kind))) } else { let binding_mode = ty::BindingMode::BindByValue(mutability.into()); Some(ClearCrossCrate::Set(BindingForm::Var(VarBindingForm { diff --git a/src/test/incremental/hashes/trait_defs.rs b/src/test/incremental/hashes/trait_defs.rs index b089d7d1eaee1..e7d25d07d1205 100644 --- a/src/test/incremental/hashes/trait_defs.rs +++ b/src/test/incremental/hashes/trait_defs.rs @@ -270,7 +270,7 @@ trait TraitChangeModeSelfOwnToMut: Sized { #[rustc_clean(label="Hir", cfg="cfail2")] #[rustc_clean(label="Hir", cfg="cfail3")] trait TraitChangeModeSelfOwnToMut: Sized { - #[rustc_clean(label="Hir", cfg="cfail2")] + #[rustc_dirty(label="Hir", cfg="cfail2")] #[rustc_clean(label="Hir", cfg="cfail3")] #[rustc_dirty(label="HirBody", cfg="cfail2")] #[rustc_clean(label="HirBody", cfg="cfail3")] diff --git a/src/test/ui/nll/issue-51191.rs b/src/test/ui/nll/issue-51191.rs new file mode 100644 index 0000000000000..3dd6c629fc7b8 --- /dev/null +++ b/src/test/ui/nll/issue-51191.rs @@ -0,0 +1,37 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] + +struct Struct; + +impl Struct { + fn bar(self: &mut Self) { + (&mut self).bar(); //~ ERROR cannot borrow + } + + fn imm(self) { + (&mut self).bar(); //~ ERROR cannot borrow + } + + fn mtbl(mut self) { + (&mut self).bar(); + } + + fn immref(&self) { + (&mut self).bar(); //~ ERROR cannot borrow + } + + fn mtblref(&mut self) { + (&mut self).bar(); + } +} + +fn main () {} diff --git a/src/test/ui/nll/issue-51191.stderr b/src/test/ui/nll/issue-51191.stderr new file mode 100644 index 0000000000000..e56ddb1ed5304 --- /dev/null +++ b/src/test/ui/nll/issue-51191.stderr @@ -0,0 +1,37 @@ +error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable + --> $DIR/issue-51191.rs:17:9 + | +LL | fn bar(self: &mut Self) { + | ---- help: consider changing this to be mutable: `mut self` +LL | (&mut self).bar(); //~ ERROR cannot borrow + | ^^^^^^^^^^^ cannot borrow as mutable + +error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable + --> $DIR/issue-51191.rs:21:9 + | +LL | fn imm(self) { + | ---- help: consider changing this to be mutable: `mut self` +LL | (&mut self).bar(); //~ ERROR cannot borrow + | ^^^^^^^^^^^ cannot borrow as mutable + +error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable + --> $DIR/issue-51191.rs:29:9 + | +LL | (&mut self).bar(); //~ ERROR cannot borrow + | ^^^^^^^^^^^ cannot borrow as mutable + +error[E0596]: cannot borrow data in a `&` reference as mutable + --> $DIR/issue-51191.rs:29:9 + | +LL | (&mut self).bar(); //~ ERROR cannot borrow + | ^^^^^^^^^^^ cannot borrow as mutable + +error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable + --> $DIR/issue-51191.rs:33:9 + | +LL | (&mut self).bar(); + | ^^^^^^^^^^^ cannot borrow as mutable + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0596`. From 2be306939dae233ae641ebd692ef45840bb419cb Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 1 Oct 2018 19:20:57 +0200 Subject: [PATCH 2/2] Improve mutability error suggestions. This commit improves mutability error suggestions by suggesting the removal of `&mut` where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`. --- .../borrow_check/mutability_errors.rs | 35 +++++++++++++++++++ .../ui/did_you_mean/issue-31424.nll.stderr | 12 ++++--- src/test/ui/nll/issue-51191.rs | 11 ++++-- src/test/ui/nll/issue-51191.stderr | 28 ++++++++------- 4 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/borrow_check/mutability_errors.rs b/src/librustc_mir/borrow_check/mutability_errors.rs index 1d33d9cc0c452..ba625fb08c82c 100644 --- a/src/librustc_mir/borrow_check/mutability_errors.rs +++ b/src/librustc_mir/borrow_check/mutability_errors.rs @@ -16,6 +16,7 @@ use rustc::mir::TerminatorKind; use rustc::ty::{self, Const, DefIdTree, TyS, TyKind, TyCtxt}; use rustc_data_structures::indexed_vec::Idx; use syntax_pos::Span; +use syntax_pos::symbol::keywords; use dataflow::move_paths::InitLocation; use borrow_check::MirBorrowckCtxt; @@ -217,6 +218,40 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> { debug!("report_mutability_error: act={:?}, acted_on={:?}", act, acted_on); match the_place_err { + // Suggest removing a `&mut` from the use of a mutable reference. + Place::Local(local) + if { + self.mir.local_decls.get(*local).map(|local_decl| { + if let ClearCrossCrate::Set( + mir::BindingForm::ImplicitSelf(kind) + ) = local_decl.is_user_variable.as_ref().unwrap() { + // Check if the user variable is a `&mut self` and we can therefore + // suggest removing the `&mut`. + // + // Deliberately fall into this case for all implicit self types, + // so that we don't fall in to the next case with them. + *kind == mir::ImplicitSelfKind::MutRef + } else if Some(keywords::SelfValue.name()) == local_decl.name { + // Otherwise, check if the name is the self kewyord - in which case + // we have an explicit self. Do the same thing in this case and check + // for a `self: &mut Self` to suggest removing the `&mut`. + if let ty::TyKind::Ref( + _, _, hir::Mutability::MutMutable + ) = local_decl.ty.sty { + true + } else { + false + } + } else { + false + } + }).unwrap_or(false) + } => + { + err.span_label(span, format!("cannot {ACT}", ACT = act)); + err.span_label(span, "try removing `&mut` here"); + }, + // We want to suggest users use `let mut` for local (user // variable) mutations... Place::Local(local) if self.mir.local_decls[*local].can_be_made_mutable() => { diff --git a/src/test/ui/did_you_mean/issue-31424.nll.stderr b/src/test/ui/did_you_mean/issue-31424.nll.stderr index eae834e2b1aa8..15139e4e8ae36 100644 --- a/src/test/ui/did_you_mean/issue-31424.nll.stderr +++ b/src/test/ui/did_you_mean/issue-31424.nll.stderr @@ -2,15 +2,19 @@ error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable --> $DIR/issue-31424.rs:17:9 | LL | (&mut self).bar(); //~ ERROR cannot borrow - | ^^^^^^^^^^^ cannot borrow as mutable + | ^^^^^^^^^^^ + | | + | cannot borrow as mutable + | try removing `&mut` here error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable --> $DIR/issue-31424.rs:23:9 | -LL | fn bar(self: &mut Self) { - | ---- help: consider changing this to be mutable: `mut self` LL | (&mut self).bar(); //~ ERROR cannot borrow - | ^^^^^^^^^^^ cannot borrow as mutable + | ^^^^^^^^^^^ + | | + | cannot borrow as mutable + | try removing `&mut` here error: aborting due to 2 previous errors diff --git a/src/test/ui/nll/issue-51191.rs b/src/test/ui/nll/issue-51191.rs index 3dd6c629fc7b8..87ec3e5df0b81 100644 --- a/src/test/ui/nll/issue-51191.rs +++ b/src/test/ui/nll/issue-51191.rs @@ -14,11 +14,13 @@ struct Struct; impl Struct { fn bar(self: &mut Self) { - (&mut self).bar(); //~ ERROR cannot borrow + (&mut self).bar(); + //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596] } fn imm(self) { - (&mut self).bar(); //~ ERROR cannot borrow + (&mut self).bar(); + //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596] } fn mtbl(mut self) { @@ -26,11 +28,14 @@ impl Struct { } fn immref(&self) { - (&mut self).bar(); //~ ERROR cannot borrow + (&mut self).bar(); + //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596] + //~^^ ERROR cannot borrow data in a `&` reference as mutable [E0596] } fn mtblref(&mut self) { (&mut self).bar(); + //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596] } } diff --git a/src/test/ui/nll/issue-51191.stderr b/src/test/ui/nll/issue-51191.stderr index e56ddb1ed5304..c5b5218f173ac 100644 --- a/src/test/ui/nll/issue-51191.stderr +++ b/src/test/ui/nll/issue-51191.stderr @@ -1,36 +1,40 @@ error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable --> $DIR/issue-51191.rs:17:9 | -LL | fn bar(self: &mut Self) { - | ---- help: consider changing this to be mutable: `mut self` -LL | (&mut self).bar(); //~ ERROR cannot borrow - | ^^^^^^^^^^^ cannot borrow as mutable +LL | (&mut self).bar(); + | ^^^^^^^^^^^ + | | + | cannot borrow as mutable + | try removing `&mut` here error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable - --> $DIR/issue-51191.rs:21:9 + --> $DIR/issue-51191.rs:22:9 | LL | fn imm(self) { | ---- help: consider changing this to be mutable: `mut self` -LL | (&mut self).bar(); //~ ERROR cannot borrow +LL | (&mut self).bar(); | ^^^^^^^^^^^ cannot borrow as mutable error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable - --> $DIR/issue-51191.rs:29:9 + --> $DIR/issue-51191.rs:31:9 | -LL | (&mut self).bar(); //~ ERROR cannot borrow +LL | (&mut self).bar(); | ^^^^^^^^^^^ cannot borrow as mutable error[E0596]: cannot borrow data in a `&` reference as mutable - --> $DIR/issue-51191.rs:29:9 + --> $DIR/issue-51191.rs:31:9 | -LL | (&mut self).bar(); //~ ERROR cannot borrow +LL | (&mut self).bar(); | ^^^^^^^^^^^ cannot borrow as mutable error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable - --> $DIR/issue-51191.rs:33:9 + --> $DIR/issue-51191.rs:37:9 | LL | (&mut self).bar(); - | ^^^^^^^^^^^ cannot borrow as mutable + | ^^^^^^^^^^^ + | | + | cannot borrow as mutable + | try removing `&mut` here error: aborting due to 5 previous errors