Skip to content

Commit

Permalink
Note when a a move/borrow error is caused by a deref coercion
Browse files Browse the repository at this point in the history
Fixes #73268

When a deref coercion occurs, we may end up with a move error if the
base value has been partially moved out of. However, we do not indicate
anywhere that a deref coercion is occuring, resulting in an error
message with a confusing span.

This PR adds an explicit note to move errors when a deref coercion is
involved. We mention the name of the type that the deref-coercion
resolved to, as well as the `Deref::Target` associated type being used.
  • Loading branch information
Aaron1011 committed Sep 11, 2020
1 parent a1947b3 commit d18b4bb
Show file tree
Hide file tree
Showing 23 changed files with 250 additions and 54 deletions.
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/ty/adjustment.rs
Expand Up @@ -4,6 +4,7 @@ use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
use rustc_macros::HashStable;
use rustc_span::Span;

#[derive(Clone, Copy, Debug, PartialEq, Eq, TyEncodable, TyDecodable, HashStable)]
pub enum PointerCast {
Expand Down Expand Up @@ -113,6 +114,9 @@ pub enum Adjust<'tcx> {
pub struct OverloadedDeref<'tcx> {
pub region: ty::Region<'tcx>,
pub mutbl: hir::Mutability,
/// The `Span` associated with the field access or method call
/// that triggered this overloaded deref.
pub span: Span,
}

impl<'tcx> OverloadedDeref<'tcx> {
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_middle/src/ty/structural_impls.rs
Expand Up @@ -606,8 +606,11 @@ impl<'a, 'tcx> Lift<'tcx> for ty::adjustment::Adjust<'a> {
impl<'a, 'tcx> Lift<'tcx> for ty::adjustment::OverloadedDeref<'a> {
type Lifted = ty::adjustment::OverloadedDeref<'tcx>;
fn lift_to_tcx(&self, tcx: TyCtxt<'tcx>) -> Option<Self::Lifted> {
tcx.lift(&self.region)
.map(|region| ty::adjustment::OverloadedDeref { region, mutbl: self.mutbl })
tcx.lift(&self.region).map(|region| ty::adjustment::OverloadedDeref {
region,
mutbl: self.mutbl,
span: self.span,
})
}
}

Expand Down
27 changes: 23 additions & 4 deletions compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
Expand Up @@ -66,7 +66,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let span = use_spans.args_or_use();

let move_site_vec = self.get_moved_indexes(location, mpi);
debug!("report_use_of_moved_or_uninitialized: move_site_vec={:?}", move_site_vec);
debug!(
"report_use_of_moved_or_uninitialized: move_site_vec={:?} use_spans={:?}",
move_site_vec, use_spans
);
let move_out_indices: Vec<_> =
move_site_vec.iter().map(|move_site| move_site.moi).collect();

Expand Down Expand Up @@ -229,6 +232,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);
}
}
// Deref::deref takes &self, which cannot cause a move
FnSelfUseKind::DerefCoercion { .. } => unreachable!(),
}
} else {
err.span_label(
Expand Down Expand Up @@ -355,6 +360,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
self.note_type_does_not_implement_copy(&mut err, &note_msg, ty, span, partial_str);
}

if let UseSpans::FnSelfUse {
kind: FnSelfUseKind::DerefCoercion { deref_target, deref_target_ty },
..
} = use_spans
{
err.note(&format!(
"{} occurs due to deref coercion to `{}`",
desired_action.as_noun(),
deref_target_ty
));

err.span_note(deref_target, "deref defined here");
}

if let Some((_, mut old_err)) =
self.move_error_reported.insert(move_out_indices, (used_place, err))
{
Expand Down Expand Up @@ -945,7 +964,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
name: &str,
borrow: &BorrowData<'tcx>,
drop_span: Span,
borrow_spans: UseSpans,
borrow_spans: UseSpans<'tcx>,
explanation: BorrowExplanation,
) -> DiagnosticBuilder<'cx> {
debug!(
Expand Down Expand Up @@ -1146,7 +1165,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
location: Location,
borrow: &BorrowData<'tcx>,
drop_span: Span,
borrow_spans: UseSpans,
borrow_spans: UseSpans<'tcx>,
proper_span: Span,
explanation: BorrowExplanation,
) -> DiagnosticBuilder<'cx> {
Expand Down Expand Up @@ -1274,7 +1293,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

fn report_escaping_closure_capture(
&mut self,
use_span: UseSpans,
use_span: UseSpans<'tcx>,
var_span: Span,
fr_name: &RegionName,
category: ConstraintCategory,
Expand Down
Expand Up @@ -501,7 +501,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
fn later_use_kind(
&self,
borrow: &BorrowData<'tcx>,
use_spans: UseSpans,
use_spans: UseSpans<'tcx>,
location: Location,
) -> (LaterUseKind, Span) {
match use_spans {
Expand Down
73 changes: 57 additions & 16 deletions compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs
Expand Up @@ -11,7 +11,7 @@ use rustc_middle::mir::{
PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
};
use rustc_middle::ty::print::Print;
use rustc_middle::ty::{self, DefIdTree, Ty, TyCtxt};
use rustc_middle::ty::{self, DefIdTree, Instance, Ty, TyCtxt};
use rustc_span::{
hygiene::{DesugaringKind, ForLoopLoc},
symbol::sym,
Expand Down Expand Up @@ -538,7 +538,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

/// The span(s) associated to a use of a place.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub(super) enum UseSpans {
pub(super) enum UseSpans<'tcx> {
/// The access is caused by capturing a variable for a closure.
ClosureUse {
/// This is true if the captured variable was from a generator.
Expand All @@ -558,7 +558,7 @@ pub(super) enum UseSpans {
fn_call_span: Span,
/// The definition span of the method being called
fn_span: Span,
kind: FnSelfUseKind,
kind: FnSelfUseKind<'tcx>,
},
/// This access is caused by a `match` or `if let` pattern.
PatUse(Span),
Expand All @@ -567,31 +567,44 @@ pub(super) enum UseSpans {
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub(super) enum FnSelfUseKind {
pub(super) enum FnSelfUseKind<'tcx> {
/// A normal method call of the form `receiver.foo(a, b, c)`
Normal { self_arg: Ident, implicit_into_iter: bool },
/// A call to `FnOnce::call_once`, desugared from `my_closure(a, b, c)`
FnOnceCall,
/// A call to an operator trait, desuraged from operator syntax (e.g. `a << b`)
Operator { self_arg: Ident },
DerefCoercion {
/// The `Span` of the `Target` associated type
/// in the `Deref` impl we are using.
deref_target: Span,
/// The type `T::Deref` we are dereferencing to
deref_target_ty: Ty<'tcx>,
},
}

impl UseSpans {
impl UseSpans<'_> {
pub(super) fn args_or_use(self) -> Span {
match self {
UseSpans::ClosureUse { args_span: span, .. }
| UseSpans::PatUse(span)
| UseSpans::FnSelfUse { var_span: span, .. }
| UseSpans::OtherUse(span) => span,
UseSpans::FnSelfUse {
fn_call_span, kind: FnSelfUseKind::DerefCoercion { .. }, ..
} => fn_call_span,
UseSpans::FnSelfUse { var_span, .. } => var_span,
}
}

pub(super) fn var_or_use(self) -> Span {
match self {
UseSpans::ClosureUse { var_span: span, .. }
| UseSpans::PatUse(span)
| UseSpans::FnSelfUse { var_span: span, .. }
| UseSpans::OtherUse(span) => span,
UseSpans::FnSelfUse {
fn_call_span, kind: FnSelfUseKind::DerefCoercion { .. }, ..
} => fn_call_span,
UseSpans::FnSelfUse { var_span, .. } => var_span,
}
}

Expand Down Expand Up @@ -754,7 +767,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
&self,
moved_place: PlaceRef<'tcx>, // Could also be an upvar.
location: Location,
) -> UseSpans {
) -> UseSpans<'tcx> {
use self::UseSpans::*;

let stmt = match self.body[location.block].statements.get(location.statement_index) {
Expand Down Expand Up @@ -809,36 +822,64 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
kind: TerminatorKind::Call { fn_span, from_hir_call, .. }, ..
}) = &self.body[location.block].terminator
{
let method_did = if let Some(method_did) =
let (method_did, method_substs) = if let Some(info) =
crate::util::find_self_call(self.infcx.tcx, &self.body, target_temp, location.block)
{
method_did
info
} else {
return normal_ret;
};

let tcx = self.infcx.tcx;

let parent = tcx.parent(method_did);
let is_fn_once = parent == tcx.lang_items().fn_once_trait();
let is_operator = !from_hir_call
&& parent.map_or(false, |p| tcx.lang_items().group(LangItemGroup::Op).contains(&p));
let is_deref = !from_hir_call && tcx.is_diagnostic_item(sym::deref_method, method_did);
let fn_call_span = *fn_span;

let self_arg = tcx.fn_arg_names(method_did)[0];

debug!(
"terminator = {:?} from_hir_call={:?}",
self.body[location.block].terminator, from_hir_call
);

// Check for a 'special' use of 'self' -
// an FnOnce call, an operator (e.g. `<<`), or a
// deref coercion.
let kind = if is_fn_once {
FnSelfUseKind::FnOnceCall
Some(FnSelfUseKind::FnOnceCall)
} else if is_operator {
FnSelfUseKind::Operator { self_arg }
Some(FnSelfUseKind::Operator { self_arg })
} else if is_deref {
let deref_target =
tcx.get_diagnostic_item(sym::deref_target).and_then(|deref_target| {
Instance::resolve(tcx, self.param_env, deref_target, method_substs)
.transpose()
});
if let Some(Ok(instance)) = deref_target {
let deref_target_ty = instance.ty(tcx, self.param_env);
Some(FnSelfUseKind::DerefCoercion {
deref_target: tcx.def_span(instance.def_id()),
deref_target_ty,
})
} else {
None
}
} else {
None
};

let kind = kind.unwrap_or_else(|| {
// This isn't a 'special' use of `self`
debug!("move_spans: method_did={:?}, fn_call_span={:?}", method_did, fn_call_span);
let implicit_into_iter = matches!(
fn_call_span.desugaring_kind(),
Some(DesugaringKind::ForLoop(ForLoopLoc::IntoIter))
);
FnSelfUseKind::Normal { self_arg, implicit_into_iter }
};
});

return FnSelfUse {
var_span: stmt.source_info.span,
Expand All @@ -859,7 +900,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
/// and its usage of the local assigned at `location`.
/// This is done by searching in statements succeeding `location`
/// and originating from `maybe_closure_span`.
pub(super) fn borrow_spans(&self, use_span: Span, location: Location) -> UseSpans {
pub(super) fn borrow_spans(&self, use_span: Span, location: Location) -> UseSpans<'tcx> {
use self::UseSpans::*;
debug!("borrow_spans: use_span={:?} location={:?}", use_span, location);

Expand Down Expand Up @@ -963,7 +1004,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

/// Helper to retrieve span(s) of given borrow from the current MIR
/// representation
pub(super) fn retrieve_borrow_spans(&self, borrow: &BorrowData<'_>) -> UseSpans {
pub(super) fn retrieve_borrow_spans(&self, borrow: &BorrowData<'_>) -> UseSpans<'tcx> {
let span = self.body.source_info(borrow.reserve_location).span;
self.borrow_spans(span, borrow.reserve_location)
}
Expand Down
Expand Up @@ -47,7 +47,7 @@ enum GroupedMoveError<'tcx> {
// Everything that isn't from pattern matching.
OtherIllegalMove {
original_path: Place<'tcx>,
use_spans: UseSpans,
use_spans: UseSpans<'tcx>,
kind: IllegalMoveOriginKind<'tcx>,
},
}
Expand Down Expand Up @@ -222,7 +222,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
let (mut err, err_span) = {
let (span, use_spans, original_path, kind): (
Span,
Option<UseSpans>,
Option<UseSpans<'tcx>>,
Place<'tcx>,
&IllegalMoveOriginKind<'_>,
) = match error {
Expand Down Expand Up @@ -291,7 +291,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
move_place: Place<'tcx>,
deref_target_place: Place<'tcx>,
span: Span,
use_spans: Option<UseSpans>,
use_spans: Option<UseSpans<'tcx>>,
) -> DiagnosticBuilder<'a> {
// Inspect the type of the content behind the
// borrow to provide feedback about why this
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_mir/src/borrow_check/mod.rs
Expand Up @@ -17,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, InstanceDef, RegionVid, TyCtxt};
use rustc_middle::ty::{self, InstanceDef, ParamEnv, RegionVid, TyCtxt};
use rustc_session::lint::builtin::{MUTABLE_BORROW_RESERVATION_CONFLICT, UNUSED_MUT};
use rustc_span::{Span, Symbol, DUMMY_SP};

Expand Down Expand Up @@ -287,6 +287,7 @@ fn do_mir_borrowck<'a, 'tcx>(
if let Err((move_data, move_errors)) = move_data_results {
let mut promoted_mbcx = MirBorrowckCtxt {
infcx,
param_env,
body: promoted_body,
mir_def_id: def.did,
move_data: &move_data,
Expand Down Expand Up @@ -320,6 +321,7 @@ fn do_mir_borrowck<'a, 'tcx>(

let mut mbcx = MirBorrowckCtxt {
infcx,
param_env,
body,
mir_def_id: def.did,
move_data: &mdpe.move_data,
Expand Down Expand Up @@ -473,6 +475,7 @@ fn do_mir_borrowck<'a, 'tcx>(

crate struct MirBorrowckCtxt<'cx, 'tcx> {
crate infcx: &'cx InferCtxt<'cx, 'tcx>,
param_env: ParamEnv<'tcx>,
body: &'cx Body<'tcx>,
mir_def_id: LocalDefId,
move_data: &'cx MoveData<'tcx>,
Expand Down
Expand Up @@ -101,7 +101,7 @@ impl<'a, 'tcx> Visitor<'tcx> for ConstMutationChecker<'a, 'tcx> {
.note("each usage of a `const` item creates a new temporary")
.note("the mutable reference will refer to this temporary, not the original `const` item");

if let Some(method_did) = method_did {
if let Some((method_did, _substs)) = method_did {
lint.span_note(self.tcx.def_span(method_did), "mutable reference created due to call to this method");
}

Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_mir/src/util/find_self_call.rs
@@ -1,30 +1,31 @@
use rustc_middle::mir::*;
use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::DefId;

/// Checks if the specified `local` is used as the `self` prameter of a method call
/// in the provided `BasicBlock`. If it is, then the `DefId` of the called method is
/// returned.
pub fn find_self_call(
tcx: TyCtxt<'_>,
body: &Body<'_>,
pub fn find_self_call<'tcx>(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
local: Local,
block: BasicBlock,
) -> Option<DefId> {
) -> Option<(DefId, SubstsRef<'tcx>)> {
debug!("find_self_call(local={:?}): terminator={:?}", local, &body[block].terminator);
if let Some(Terminator { kind: TerminatorKind::Call { func, args, .. }, .. }) =
&body[block].terminator
{
debug!("find_self_call: func={:?}", func);
if let Operand::Constant(box Constant { literal: ty::Const { ty, .. }, .. }) = func {
if let ty::FnDef(def_id, _) = *ty.kind() {
if let ty::FnDef(def_id, substs) = *ty.kind() {
if let Some(ty::AssocItem { fn_has_self_parameter: true, .. }) =
tcx.opt_associated_item(def_id)
{
debug!("find_self_call: args={:?}", args);
if let [Operand::Move(self_place) | Operand::Copy(self_place), ..] = **args {
if self_place.as_local() == Some(local) {
return Some(def_id);
return Some((def_id, substs));
}
}
}
Expand Down

0 comments on commit d18b4bb

Please sign in to comment.