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

Aaron1011 · 2020-05-20T17:56:41Z

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::

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:

Add special handling for FnOnce/FnMut/Fn - we probably don't want to point at the unstable trait methods
Consider adding additional context for operations (e.g. Shr::shr) when the call was generated using the operator syntax (e.g. a >> b)
Consider pointing to the method parent (impl or trait block) in addition to the method itself.

rust-highfive · 2020-05-20T17:56:44Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2020-05-20T18:42:54Z

r? @nikomatsakis for review or reassignment

nikomatsakis

Skimming the code, it seems reasonable so far

Aaron1011 · 2020-05-25T19:38:58Z

I'm happy with the current output of this PR

nikomatsakis

This is really nice, I think it's going to be a big improvement. I left some comments where it seems like the output could be further improved -- thoughts?

src/librustc_mir/borrow_check/diagnostics/mod.rs

src/test/ui/binop/binop-consume-args.stderr

src/test/ui/issues/issue-34721.stderr

src/test/ui/once-cant-call-twice-on-heap.stderr

nikomatsakis · 2020-05-26T22:24:51Z

cc @estebank, I suspect you'd like to see this

src/test/ui/issues/issue-61108.stderr

src/librustc_mir/borrow_check/diagnostics/mod.rs

src/test/ui/moves/move-fn-self-receiver.stderr

Aaron1011 · 2020-06-09T21:17:29Z

This PR ended up accreting some non-diagnostic changes (serializing Idents instead of Symbols for a query, and added extra data to DesugaringKind::For). I can split them out into separate PRs if that would make this easier to review.

nikomatsakis · 2020-06-10T10:35:36Z

@Aaron1011 I think that would be good

Aaron1011 · 2020-06-10T19:49:55Z

@nikomatsakis: I've split out the metadata and desguaring changes locally, but I don't see a way of testing them without this PR. Do you still want me to split them out, or would it be better to merge them with PR where they actually get tested?

Fixes rust-lang#69977 When we parse a chain of method calls like `foo.a().b().c()`, each `MethodCallExpr` gets assigned a span that starts at the beginning of the call chain (`foo`). While this is useful for diagnostics, it means that `Location::caller` will return the same location for every call in a call chain. This PR makes us separately record the span of the function name and arguments for a method call (e.g. `b()` in `foo.a().b().c()`). This `Span` is passed through HIR lowering and MIR building to `TerminatorKind::Call`, where it is used in preference to `Terminator.source_info.span` when determining `Location::caller`. This new span is also useful for diagnostics where we want to emphasize a particular method call - for an example, see rust-lang#72389 (comment)

nikomatsakis · 2020-06-11T14:41:14Z

@Aaron1011 you mean that you split them out into distinct commits? I think if they can't be tested, they should probably stay in this PR.

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#71824 (Check for live drops in constants after drop elaboration) - rust-lang#72389 (Explain move errors that occur due to method calls involving `self`) - rust-lang#72556 (Fix trait alias inherent impl resolution) - rust-lang#72584 (Stabilize vec::Drain::as_slice) - rust-lang#72598 (Display information about captured variable in `FnMut` error) - rust-lang#73336 (Group `Pattern::strip_*` method together) - rust-lang#73341 (_match.rs: fix module doc comment) - rust-lang#73342 (Fix iterator copied() documentation example code) - rust-lang#73351 (Update E0446.md) - rust-lang#73353 (structural_match: non-structural-match ty closures) Failed merges: r? @ghost

flip1995 · 2020-06-16T18:00:58Z

src/librustc_ast_lowering/expr.rs

@@ -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);


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 😄

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.

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

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.

@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

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.

Following PR rust-lang#72389, we create many more spans with `ExpnKind::Desugaring`. This exposed a latent bug in Clippy - only the top-most `ExpnData` is considered when checking if code is the result of a macro expansion. If code emitted by a macro expansion gets annotated with an `ExpnKind::Desugaring` (e.g. an operator or a for loop), Clippy will incorrectly act as though this code is not the result of a macro expansion. This PR introduces the `ClosestAstOrMacro` enum, which allows linting code to quickly determine if a given `Span` is the result of a macro expansion. For any `ExpnId`, we keep track of closest `ExpnKind::Macro` or `ExpnKind::AstPass` in the `call_site` chain. This is determined when the `ExpnData` is set for an `ExpnId`, which allows us to avoid walking the entire chain in Clippy.

…elf-msg, r=nikomatsakis" This reverts commit 372cb9b, reversing changes made to 5c61a8d.

See rust-lang#73468 (comment) This removes the immediate need for PR rust-lang#73468, since Clippy will no longer be confused by the precense of a new `ExpnId` 'on top of' a macro expansion `ExpnId`. This makes some of the 'self move diagnostics' added by PR rust-lang#72389 slightly worse. I've left the operator-related diagnostics code in place, so it should be easy for a followup PR to plug in a new way of detecting desugared operators.

…elf-msg, r=nikomatsakis" This reverts commit 372cb9b, reversing changes made to 5c61a8d.

@petrochenkov

…r=petrochenkov Revert PR rust-lang#72389 - "Explain move errors that occur due to method calls involving `self" r? @petrochenkov

…elf-msg, r=nikomatsakis" This reverts commit 372cb9b, reversing changes made to 5c61a8d.

@petrochenkov

…Manishearth Revert PR rust-lang#72389 - "Explain move errors that occur due to method calls involving `self" r? @petrochenkov

This is a re-attempt of rust-lang#72389 (which was reverted in rust-lang#73594) Instead of using `ExpnKind::Desugaring` to represent operators, this PR checks the lang item directly.

…lf-msg, r=davidtwco Explain move errors that occur due to method calls involving `self` (take two) This is a re-attempt of rust-lang#72389 (which was reverted in rust-lang#73594) Instead of using `ExpnKind::Desugaring` to represent operators, this PR checks the lang item directly.

rust-highfive assigned petrochenkov May 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2020

rust-highfive assigned nikomatsakis and unassigned petrochenkov May 20, 2020

nikomatsakis reviewed May 22, 2020

View reviewed changes

Aaron1011 force-pushed the feature/move-fn-self-msg branch from 80c1359 to 334d559 Compare May 22, 2020 21:47

Aaron1011 changed the title ~~[WIP] Explain move errors that occur due to method calls involving self~~ Explain move errors that occur due to method calls involving self May 25, 2020

nikomatsakis reviewed May 26, 2020

View reviewed changes

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2020