-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Point to span of upvar making closure FnMut #81158
Conversation
r? @varkor (rust-highfive has picked a reviewer for you, use r? to override) |
r? @Aaron1011 |
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
Hmm, that's weird. I wonder why it says that it's trying to print a |
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
b082a67
to
dcb02bf
Compare
@1000teslas It looks like |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -241,6 +244,39 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { | |||
format!("mut {}", self.local_names[local].unwrap()), | |||
Applicability::MachineApplicable, | |||
); | |||
let tcx = self.infcx.tcx; | |||
// point to span of upvar making closure call require mutable borrow | |||
if let ty::Closure(id, _) = the_place_err.ty(self.body, tcx).ty.kind() { |
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.
This looks almost identical to the code you added later on this in this file. Could you factor out the common parts into a new closure/method?
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.
@Aaron1011 I've factored the code out.
format!("mutable borrow of `{}`", upvar) | ||
} | ||
ty::UpvarCapture::ByValue(_) => { | ||
format!("possible mutation of `{}`", upvar) |
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.
Why 'possible'? Can there be false positives here, or are you thinking of something like:
let cb = || {
if false {
capture = new_val;
}
}
If it's the latter, I would get rid of the word 'possible', since this can also happen for a mutable borrow (where we don't say 'possible').
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 was thinking of something like issue-80313-mutable-borrow-in-move-closure.rs
, where a mutable borrow is taken of the owned upvar, but no mutation occurs. If I omitted the mutable borrow, and used only the owned value, it would compile fine. In the case of a normal closure, even in your example, where the mutation is not reachable, I think the mutable borrow is always done.
The job Click to see the possible cause of the failure (guessed by this bot)
|
@1000teslas: Thanks! Can you squash the commits into one? |
Add expected error Add comment Tweak comment wording Fix after rebase to updated master Fix after rebase to updated master Distinguish mutation in normal and move closures Tweak error message Fix error message for nested closures Refactor code showing mutated upvar in closure Remove debug assert B
1dfd2d8
to
26b4baf
Compare
@Aaron1011 I've squashed the commits into one. |
@bors r+ |
📌 Commit 26b4baf has been approved by |
⌛ Testing commit 26b4baf with merge b749d67e10bd982df208b49a352fd2b510277f08... |
…1011 Point to span of upvar making closure FnMut For rust-lang#80313.
…1011 Point to span of upvar making closure FnMut For rust-lang#80313.
…1011 Point to span of upvar making closure FnMut For rust-lang#80313.
Rollup of 10 pull requests Successful merges: - rust-lang#79570 (rustc: Stabilize `-Zrun-dsymutil` as `-Csplit-debuginfo`) - rust-lang#79819 (Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` lint) - rust-lang#79991 (rustdoc: Render HRTB correctly for bare functions) - rust-lang#80215 (Use -target when linking binaries for Mac Catalyst) - rust-lang#81158 (Point to span of upvar making closure FnMut) - rust-lang#81176 (Improve safety of `LateContext::qpath_res`) - rust-lang#81287 (Split rustdoc JSON types into separately versioned crate) - rust-lang#81306 (Fuse inner iterator in FlattenCompat and improve related tests) - rust-lang#81333 (clean up some const error reporting around promoteds) - rust-lang#81459 (Fix rustdoc text selection for page titles) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
For #80313.