-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix erroneous error note when using field after move #51688
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.
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
Fix erroneous error note when using field after move #51688
Conversation
o: Origin) | ||
-> DiagnosticBuilder<'cx> | ||
{ | ||
let moved_path = moved_path.map(|mp| format!(": `{}`", mp)).unwrap_or("".to_owned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have a test that shows the effect of this change? I don't see one... or is it that other tests regressed as a result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the stderr files changed like in https://github.com/rust-lang/rust/pull/51688/files#diff-4d3cc9be8c38f963314cf51e57d3fcbcL23
@bors r+ |
📌 Commit fe264bb6f0aa7efb7be3432360edd1f8923c1f80 has been approved by |
@bors r- |
@bors r+ -- I was confused, seems fine |
📌 Commit fe264bb6f0aa7efb7be3432360edd1f8923c1f80 has been approved by |
fe264bb
to
b9b0b93
Compare
☔ The latest upstream changes (presumably #51660) made this pull request unmergeable. Please resolve the merge conflicts. |
Some(name) => format!("`{}`", name), | ||
None => "value".to_owned(), | ||
}; | ||
self.tcx | ||
.cannot_act_on_uninitialized_variable( | ||
span, | ||
desired_action.as_noun(), | ||
&self.describe_place(place).unwrap_or("_".to_owned()), | ||
&self.describe_place(place, true).unwrap_or("_".to_owned()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to have a flag -- which probably makes sense at this juncture -- I would prefer two distinct fns that wrap describe_place
. I find "naked booleans" quite hard to read later on — maybe something like
describe_place_including_downcast(place)
describe_place_excluding_downcast(place)
descirbe_place(place, including_downcast: bool)
Or, better yet, since we aim to eventually never include downcasts, maybe we can make "excluding downcasts" be the default. Then we can make a newtype'd flag. Something like this:
struct IncludingPlace(bool);
impl {
fn describe_place(&self, place: &Place) {
self.describe_place_with_options(place, IncludingDowncast(false))
}
fn describe_place_with_options(&self, place: &Place, including_downcast: IncludingDowncast) {
match place {
..
Downcast(..) if including_downcast.0 => { ... }
Downcast(..) => Err(()),
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
4203003
to
532e03a
Compare
532e03a
to
1dae309
Compare
@@ -654,12 +668,26 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
} | |||
} | |||
|
|||
pub(super) struct IncludingDowncast(bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually a bit more idiomatic to use an enum with the possible states, but this is still an improvement in readibility over a naked Boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I prefer an enum, was just following @nikomatsakis suggestion. But I'm fine with either.
--> $DIR/issue-41962.rs:17:21 | ||
| | ||
LL | if let Some(thing) = maybe { | ||
| ^^^^^ value moved here in previous iteration of loop | ||
| | ||
= note: move occurs because `maybe.0` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait | ||
= note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder wether a small wording tweak would be warranted here:
Move occurs because the value has type
Move occurs because it has type
@bors r=nikomatsakis |
📌 Commit 1dae309 has been approved by |
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 |
1 similar comment
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 |
@estebank @nikomatsakis failures are related to Travis issues ... |
@bors retry |
⌛ Testing commit 1dae309 with merge 6ffc004cb6aae0aa1723dae93b756a669e43555e... |
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
@bors retry |
…matsakis Fix erroneous error note when using field after move Closes #51512 r? @nikomatsakis
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
1 similar comment
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
@bors retry |
…matsakis Fix erroneous error note when using field after move Closes #51512 r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Closes #51512
r? @nikomatsakis