[NLL] Better move errors #51729
[NLL] Better move errors #51729
Conversation
rust-highfive
added
the
S-waiting-on-review
matthewjasper
force-pushed
the
move-errors
branch
from
f6095d0 to
6d277ac
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
move-errors
branch
from
6d277ac to
c81653f
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
move-errors
branch
2 times, most recently
from
e6b8f1a to
9da2fa6
Compare
| // in the case of box expr there is no such check. | ||
| if let Place::Projection(..) = destination { | ||
| this.local_decls.push(LocalDecl::new_temp(expr.ty, expr.span)); | ||
| } |
There was a problem hiding this comment.
With #51139, I don't think this should be needed, right?
| @@ -44,6 +44,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| arms: Vec<Arm<'tcx>>) | |||
| -> BlockAnd<()> { | |||
| let tcx = self.hir.tcx(); | |||
| let discriminant_span = match discriminant { | |||
There was a problem hiding this comment.
Nit: seems like we should move this to a helper method on ExprRef
| @@ -288,6 +298,20 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| candidate.match_pairs); | |||
| } | |||
|
|
|||
| if let Some(span) = initializer_span { | |||
There was a problem hiding this comment.
it feels to me like a comment would be nice here
| @@ -0,0 +1,364 @@ | |||
| // Copyright 2018 The Rust Project Developers. See the COPYRIGHT | |||
| // file at the top-level directory of this distribution and at | |||
| // http://rust-lang.org/COPYRIGHT. | |||
There was a problem hiding this comment.
Thank you for making a module <3
| } | ||
|
|
||
| #[derive(Debug)] | ||
| enum GroupedMoveError<'tcx> { |
There was a problem hiding this comment.
I feel like a meta comment on the enum explaining (in general) what we group by and why would be super useful. e.g.
often when desugaring a pattern match we may have many individual moves in MIR
that are all part of one operation from the user's point-of-view. For example:
let (x, y) = foo()
would move x from the 0 field of some temporary, and y from the 1 field. We group such errors together for cleaner error reporting.
| #[derive(Debug)] | ||
| enum GroupedMoveError<'tcx> { | ||
| // Match place can't be moved from | ||
| // e.g. Matching on x[0]. |
There was a problem hiding this comment.
can you make this rust code? I guess this means match x[0] { ... } where x has type &[u32] or something?
There was a problem hiding this comment.
(Of course the same is true for &Vec...)
| binds_to: Vec<Local>, | ||
| }, | ||
| // Part of a pattern can't be moved from, | ||
| // e.g. moving from a &x pattern. |
There was a problem hiding this comment.
similarly, I would say e.g. let &x = ... -- here x references borrowed content, so a move would be illegal
| opt_ty_info: _, | ||
| }))) = local_decl.is_user_variable | ||
| { | ||
| let (match_place, match_span) = opt_match_place |
There was a problem hiding this comment.
I'd like to see some sort of comment explaining what these represent in terms of Rust source (e.g., give a Rust example, and explain what match_place and match_span would be)
| .map(|&(ref x, y)| (x, y)) | ||
| .unwrap_or((move_from, stmt_source_info.span)); | ||
|
|
||
| if self |
There was a problem hiding this comment.
I don't quite get why this sub_scope test makes sense... example of where it applies (and maybe where it doesn't?)
matthewjasper
force-pushed
the
move-errors
branch
from
9da2fa6 to
d88fde9
Compare
matthewjasper
force-pushed
the
move-errors
branch
from
d88fde9 to
2cb0a06
Compare
|
Nits addressed |
|
@bors r+ |
|
📌 Commit 2cb0a06 has been approved by |
bors
added
S-waiting-on-bors
[NLL] Better move errors Make a number of changes to improve the quality of NLL cannot move errors. * Group errors that occur in the same `match` with the same cause. * Suggest `ref`, `&` or removing `*` to avoid the move. * Show the place being matched on. Differences from AST borrowck: * `&` is suggested over `ref` when matching on a place that can't be moved from. * Removing `*` is suggested instead of adding `&` when applicable. * Sub-pattern spans aren't used, this would probably need Spans on Places. Closes #45699 Closes #46627 Closes #51187 Closes #51189 r? @pnkfelix
|
☀️ Test successful - status-appveyor, status-travis |
| @@ -36,7 +36,7 @@ fn foo<T: Copy>(_t: T, q: i32) -> i32 { | |||
| // _5 = (move _6, move _7); | |||
| // _8 = move (_5.0: i32); | |||
| // _9 = move (_5.1: i32); | |||
| // _0 = move _8; | |||
| // _0 = _8; | |||
There was a problem hiding this comment.
I am very confused as to why this happened. Why was it a move before and not after?
Make a number of changes to improve the quality of NLL cannot move errors.
matchwith the same cause.ref,&or removing*to avoid the move.Differences from AST borrowck:
&is suggested overrefwhen matching on a place that can't be moved from.*is suggested instead of adding&when applicable.Closes #45699
Closes #46627
Closes #51187
Closes #51189
r? @pnkfelix