Skip to content

Commit

Permalink
Rollup merge of rust-lang#72389 - Aaron1011:feature/move-fn-self-msg,…
Browse files Browse the repository at this point in the history
… r=nikomatsakis

Explain move errors that occur due to method calls involving `self`

When calling a method that takes `self` (e.g. `vec.into_iter()`), the method receiver is moved out of. If the method receiver is used again, a move error will be emitted::

```rust
fn main() {
    let a = vec![true];
    a.into_iter();
    a;
}
```

emits

```
error[E0382]: use of moved value: `a`
 --> src/main.rs:4:5
  |
2 |     let a = vec![true];
  |         - move occurs because `a` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
3 |     a.into_iter();
  |     - value moved here
4 |     a;
  |     ^ value used here after move
```

However, the error message doesn't make it clear that the move is caused by the call to `into_iter`.

This PR adds additional messages to move errors when the move is caused by using a value as the receiver of a `self` method::

```
error[E0382]: use of moved value: `a`
   --> vec.rs:4:5
    |
2   |     let a = vec![true];
    |         - move occurs because `a` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
3   |     a.into_iter();
    |     ------------- value moved due to this method call
4   |     a;
    |     ^ value used here after move
    |
note: this function takes `self`, which moves the receiver
   --> /home/aaron/repos/rust/src/libcore/iter/traits/collect.rs:239:5
    |
239 |     fn into_iter(self) -> Self::IntoIter;
```

TODO:

- [x] Add special handling for `FnOnce/FnMut/Fn` - we probably don't want to point at the unstable trait methods
- [x] Consider adding additional context for operations (e.g. `Shr::shr`) when the call was generated using the operator syntax (e.g. `a >> b`)
- [x] Consider pointing to the method parent (impl or trait block) in addition to the method itself.
  • Loading branch information
RalfJung authored Jun 15, 2020
2 parents 5c61a8d + 4646e2d commit 372cb9b
Show file tree
Hide file tree
Showing 45 changed files with 729 additions and 103 deletions.
24 changes: 19 additions & 5 deletions src/librustc_ast_lowering/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_data_structures::thin_vec::ThinVec;
use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_span::source_map::{respan, DesugaringKind, Span, Spanned};
use rustc_span::source_map::{respan, DesugaringKind, ForLoopLoc, Span, Spanned};
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_target::asm;
use std::collections::hash_map::Entry;
Expand All @@ -25,6 +25,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
}

pub(super) fn lower_expr_mut(&mut self, e: &Expr) -> hir::Expr<'hir> {
let mut span = e.span;
ensure_sufficient_stack(|| {
let kind = match e.kind {
ExprKind::Box(ref inner) => hir::ExprKind::Box(self.lower_expr(inner)),
Expand Down Expand Up @@ -53,6 +54,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
hir::ExprKind::MethodCall(hir_seg, seg.ident.span, args, span)
}
ExprKind::Binary(binop, ref lhs, ref rhs) => {
span = self.mark_span_with_reason(DesugaringKind::Operator, e.span, None);
let binop = self.lower_binop(binop);
let lhs = self.lower_expr(lhs);
let rhs = self.lower_expr(rhs);
Expand Down Expand Up @@ -222,7 +224,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
hir::Expr {
hir_id: self.lower_node_id(e.id),
kind,
span: e.span,
span,
attrs: e.attrs.iter().map(|a| self.lower_attr(a)).collect::<Vec<_>>().into(),
}
})
Expand All @@ -237,6 +239,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
}

fn lower_binop(&mut self, b: BinOp) -> hir::BinOp {
let span = self.mark_span_with_reason(DesugaringKind::Operator, b.span, None);
Spanned {
node: match b.node {
BinOpKind::Add => hir::BinOpKind::Add,
Expand All @@ -258,7 +261,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
BinOpKind::Ge => hir::BinOpKind::Ge,
BinOpKind::Gt => hir::BinOpKind::Gt,
},
span: b.span,
span,
}
}

Expand Down Expand Up @@ -1360,9 +1363,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
body: &Block,
opt_label: Option<Label>,
) -> hir::Expr<'hir> {
let orig_head_span = head.span;
// expand <head>
let mut head = self.lower_expr_mut(head);
let desugared_span = self.mark_span_with_reason(DesugaringKind::ForLoop, head.span, None);
let desugared_span = self.mark_span_with_reason(
DesugaringKind::ForLoop(ForLoopLoc::Head),
orig_head_span,
None,
);
head.span = desugared_span;

let iter = Ident::with_dummy_span(sym::iter);
Expand Down Expand Up @@ -1457,10 +1465,16 @@ impl<'hir> LoweringContext<'_, 'hir> {
// `mut iter => { ... }`
let iter_arm = self.arm(iter_pat, loop_expr);

let into_iter_span = self.mark_span_with_reason(
DesugaringKind::ForLoop(ForLoopLoc::IntoIter),
orig_head_span,
None,
);

// `match ::std::iter::IntoIterator::into_iter(<head>) { ... }`
let into_iter_expr = {
let into_iter_path = &[sym::iter, sym::IntoIterator, sym::into_iter];
self.expr_call_std_path(desugared_span, into_iter_path, arena_vec![self; head])
self.expr_call_std_path(into_iter_span, into_iter_path, arena_vec![self; head])
};

let match_expr = self.arena.alloc(self.expr_match(
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_infer/infer/error_reporting/need_type_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let msg = if let Some(simple_ident) = pattern.simple_ident() {
match pattern.span.desugaring_kind() {
None => format!("consider giving `{}` {}", simple_ident, suffix),
Some(DesugaringKind::ForLoop) => {
Some(DesugaringKind::ForLoop(_)) => {
"the element type for this iterator is not specified".to_string()
}
_ => format!("this needs {}", suffix),
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_metadata/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1317,13 +1317,13 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
}
}

fn get_fn_param_names(&self, tcx: TyCtxt<'tcx>, id: DefIndex) -> &'tcx [Symbol] {
fn get_fn_param_names(&self, tcx: TyCtxt<'tcx>, id: DefIndex) -> &'tcx [Ident] {
let param_names = match self.kind(id) {
EntryKind::Fn(data) | EntryKind::ForeignFn(data) => data.decode(self).param_names,
EntryKind::AssocFn(data) => data.decode(self).fn_data.param_names,
_ => Lazy::empty(),
};
tcx.arena.alloc_from_iter(param_names.decode(self))
tcx.arena.alloc_from_iter(param_names.decode((self, tcx)))
}

fn exported_symbols(
Expand Down
16 changes: 5 additions & 11 deletions src/librustc_metadata/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use rustc_middle::ty::{self, SymbolName, Ty, TyCtxt};
use rustc_serialize::{opaque, Encodable, Encoder, SpecializedEncoder};
use rustc_session::config::CrateType;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::{self, ExternalSource, FileName, SourceFile, Span};
use rustc_target::abi::VariantIdx;
use std::hash::Hash;
Expand Down Expand Up @@ -997,18 +997,12 @@ impl EncodeContext<'tcx> {
}
}

fn encode_fn_param_names_for_body(&mut self, body_id: hir::BodyId) -> Lazy<[Symbol]> {
self.tcx.dep_graph.with_ignore(|| {
let body = self.tcx.hir().body(body_id);
self.lazy(body.params.iter().map(|arg| match arg.pat.kind {
hir::PatKind::Binding(_, _, ident, _) => ident.name,
_ => kw::Invalid,
}))
})
fn encode_fn_param_names_for_body(&mut self, body_id: hir::BodyId) -> Lazy<[Ident]> {
self.tcx.dep_graph.with_ignore(|| self.lazy(self.tcx.hir().body_param_names(body_id)))
}

fn encode_fn_param_names(&mut self, param_names: &[Ident]) -> Lazy<[Symbol]> {
self.lazy(param_names.iter().map(|ident| ident.name))
fn encode_fn_param_names(&mut self, param_names: &[Ident]) -> Lazy<[Ident]> {
self.lazy(param_names.iter())
}

fn encode_optimized_mir(&mut self, def_id: LocalDefId) {
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_metadata/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use rustc_serialize::opaque::Encoder;
use rustc_session::config::SymbolManglingVersion;
use rustc_session::CrateDisambiguator;
use rustc_span::edition::Edition;
use rustc_span::symbol::Symbol;
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::{self, Span};
use rustc_target::spec::{PanicStrategy, TargetTriple};

Expand Down Expand Up @@ -327,7 +327,7 @@ struct ModData {
struct FnData {
asyncness: hir::IsAsync,
constness: hir::Constness,
param_names: Lazy<[Symbol]>,
param_names: Lazy<[Ident]>,
}

#[derive(RustcEncodable, RustcDecodable)]
Expand Down
9 changes: 8 additions & 1 deletion src/librustc_middle/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_hir::*;
use rustc_index::vec::IndexVec;
use rustc_span::hygiene::MacroKind;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, Symbol};
use rustc_span::symbol::{kw, Ident, Symbol};
use rustc_span::Span;
use rustc_target::spec::abi::Abi;

Expand Down Expand Up @@ -375,6 +375,13 @@ impl<'hir> Map<'hir> {
})
}

pub fn body_param_names(&self, id: BodyId) -> impl Iterator<Item = Ident> + 'hir {
self.body(id).params.iter().map(|arg| match arg.pat.kind {
PatKind::Binding(_, _, ident, _) => ident,
_ => Ident::new(kw::Invalid, rustc_span::DUMMY_SP),
})
}

/// Returns the `BodyOwnerKind` of this `LocalDefId`.
///
/// Panics if `LocalDefId` does not have an associated body.
Expand Down
20 changes: 16 additions & 4 deletions src/librustc_middle/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_hir::def_id::{LocalDefId, LOCAL_CRATE};
use rustc_hir::Body;
use rustc_hir::HirId;
use rustc_hir::ItemLocalId;
use rustc_hir::Node;
use rustc_hir::*;
use rustc_index::vec::IndexVec;

pub struct Owner<'tcx> {
Expand Down Expand Up @@ -79,5 +76,20 @@ pub fn provide(providers: &mut Providers<'_>) {
};
providers.hir_owner = |tcx, id| tcx.index_hir(LOCAL_CRATE).map[id].signature;
providers.hir_owner_nodes = |tcx, id| tcx.index_hir(LOCAL_CRATE).map[id].with_bodies.as_deref();
providers.fn_arg_names = |tcx, id| {
let hir = tcx.hir();
let hir_id = hir.as_local_hir_id(id.expect_local());
if let Some(body_id) = hir.maybe_body_owned_by(hir_id) {
tcx.arena.alloc_from_iter(hir.body_param_names(body_id))
} else if let Node::TraitItem(&TraitItem {
kind: TraitItemKind::Fn(_, TraitFn::Required(idents)),
..
}) = hir.get(hir_id)
{
tcx.arena.alloc_slice(idents)
} else {
span_bug!(hir.span(hir_id), "fn_arg_names: unexpected item {:?}", id);
}
};
map::provide(providers);
}
4 changes: 3 additions & 1 deletion src/librustc_middle/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,9 @@ pub fn struct_lint_level<'s, 'd>(
pub fn in_external_macro(sess: &Session, span: Span) -> bool {
let expn_data = span.ctxt().outer_expn_data();
match expn_data.kind {
ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop) => false,
ExpnKind::Root
| ExpnKind::Desugaring(DesugaringKind::ForLoop(_))
| ExpnKind::Desugaring(DesugaringKind::Operator) => false,
ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) => true, // well, it's "external"
ExpnKind::Macro(MacroKind::Bang, _) => {
// Dummy span for the `def_site` means it's an external macro.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_middle/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ rustc_queries! {
}

Other {
query fn_arg_names(def_id: DefId) -> &'tcx [Symbol] {
query fn_arg_names(def_id: DefId) -> &'tcx [rustc_span::symbol::Ident] {
desc { |tcx| "looking up function parameter names for `{}`", tcx.def_path_str(def_id) }
}
/// Gets the rendered value of the specified constant or associated constant.
Expand Down
72 changes: 65 additions & 7 deletions src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ use crate::borrow_check::{
};

use super::{
explain_borrow::BorrowExplanation, IncludingDowncast, RegionName, RegionNameSource, UseSpans,
explain_borrow::BorrowExplanation, FnSelfUseKind, IncludingDowncast, RegionName,
RegionNameSource, UseSpans,
};

#[derive(Debug)]
Expand Down Expand Up @@ -150,13 +151,70 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
format!("value moved{} here, in previous iteration of loop", move_msg),
);
} else {
err.span_label(move_span, format!("value moved{} here", move_msg));
move_spans.var_span_label(
&mut err,
format!("variable moved due to use{}", move_spans.describe()),
);
if let UseSpans::FnSelfUse { var_span, fn_call_span, fn_span, kind } =
move_spans
{
let place_name = self
.describe_place(moved_place.as_ref())
.map(|n| format!("`{}`", n))
.unwrap_or_else(|| "value".to_owned());
match kind {
FnSelfUseKind::FnOnceCall => {
err.span_label(
fn_call_span,
&format!("{} moved due to this call", place_name),
);
err.span_note(
var_span,
"this value implements `FnOnce`, which causes it to be moved when called",
);
}
FnSelfUseKind::Operator { self_arg } => {
err.span_label(
fn_call_span,
&format!("{} moved due to usage in operator", place_name),
);
if self.fn_self_span_reported.insert(fn_span) {
err.span_note(
self_arg.span,
"calling this operator moves the left-hand side",
);
}
}
FnSelfUseKind::Normal { self_arg, implicit_into_iter } => {
if implicit_into_iter {
err.span_label(
fn_call_span,
&format!(
"{} moved due to this implicit call to `.into_iter()`",
place_name
),
);
} else {
err.span_label(
fn_call_span,
&format!("{} moved due to this method call", place_name),
);
}
// Avoid pointing to the same function in multiple different
// error messages
if self.fn_self_span_reported.insert(self_arg.span) {
err.span_note(
self_arg.span,
&format!("this function consumes the receiver `self` by taking ownership of it, which moves {}", place_name)
);
}
}
}
} else {
err.span_label(move_span, format!("value moved{} here", move_msg));
move_spans.var_span_label(
&mut err,
format!("variable moved due to use{}", move_spans.describe()),
);
}
}
if Some(DesugaringKind::ForLoop) == move_span.desugaring_kind() {
if let Some(DesugaringKind::ForLoop(_)) = move_span.desugaring_kind() {
let sess = self.infcx.tcx.sess;
if let Ok(snippet) = sess.source_map().span_to_snippet(move_span) {
err.span_suggestion(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// Used in a closure.
(LaterUseKind::ClosureCapture, var_span)
}
UseSpans::OtherUse(span) => {
UseSpans::OtherUse(span) | UseSpans::FnSelfUse { var_span: span, .. } => {
let block = &self.body.basic_blocks()[location.block];

let kind = if let Some(&Statement {
Expand Down
Loading

0 comments on commit 372cb9b

Please sign in to comment.