Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explain move errors that occur due to method calls involving self #72389

Merged
merged 4 commits into from
Jun 15, 2020
Merged

Explain move errors that occur due to method calls involving self #72389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
754da88
Make `fn_arg_names` return `Ident` instead of symbol
Aaron1011 Jun 11, 2020
2c11c35
Explain move errors that occur due to method calls involving `self`
Aaron1011 Jun 11, 2020
5902b2f
Use `fn_span` to point to the actual method call
Aaron1011 Jun 11, 2020
4646e2d
Run fmt
Aaron1011 Jun 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/librustc_ast_lowering/expr.rs
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh this destroyed Clippy (23 UI tests suddenly failed) and send me on a multiple hour bisecting adventure. We need to enforce test-pass for Clippy ASAP 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh! Now that we are able to refactor clippy "in situ" that might be more reasonable, not sure. It'd be nice if we could at least get an "advisory" warning perhaps that clippy is affected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis Normally rustc gates on clippy tests iirc, right now due to some temporary toolstate changes they don't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this PR breaks the ability to understand if something comes from a macro desugaring or not, which is used in both clippy and rustc for diagnostics, I'm inclined to revert this until a solution that can preserve macro desugaring info is found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aaron1011 is also working on a fix that resurfaces that, i'm down to landing that as well, but as it currently stands I consider this a rustc bug

Copy link
Member Author

@Aaron1011 Aaron1011 Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this PR breaks the ability to understand if something comes from a macro desugaring or not,

Nit: This was already broken any time we created an ExpnKind::Desugaring span. However, DesugaringKind::Operator is created much more frequently that other DesugaringKinds (or it overlaps with macros/lint-able code in more cases), so it's more of an issue.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs
View file
Open in desktop
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