Extend two-phase borrows to apply to method receiver autorefs #49348
Conversation
This is required to compile things like src/test/ui/borrowck/two-phase-method-receivers.rs
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -751,13 +760,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| pub fn try_coerce(&self, | |||
| expr: &hir::Expr, | |||
| expr_ty: Ty<'tcx>, | |||
| target: Ty<'tcx>) | |||
| target: Ty<'tcx>, | |||
| allow_two_phase: bool) | |||
There was a problem hiding this comment.
Nit: maybe we can make a wrapper? I don't like the way "random bool parameters" read in the surrounding code. Maybe something like:
fn try_coerce(...) {
self.try_coerce_full(..., false)
}
fn try_coerce_with_two_phase_borrow(...) {
self.try_coerce_full(..., true)
}
fn try_coerce_full(...) { // as today
}There was a problem hiding this comment.
Alternatively, since I see that the bool parameter propagates elsewhere, can we make it a newtype'd bool?
struct TwoPhase(bool);so that callers look like:
self.try_coerce(..., TwoPhase(false))This also gives us a central place to write docs that talk about 2-phase borrowing...
There was a problem hiding this comment.
I also wasn't super enthused about leaking that parameter everywhere. I think I'll take the newtype'd bool approach here and try to do a bit of a writeup. I think it makes sense to push that newtype through all the way until it's consumed in MIR borrwck which is going to touch a bit more code. I'll put that in a separate commit in case we want to back it out later.
Because more type safe is more better, and random boolean parameters everywhere were not the greatest thing.
For consistency, use AllowTwoPhase everywhere between the frontend and MIR.
|
Oops, failed tidy. I've added a pre-commit hook to my local working trees to prevent that in the future, and pushed a fix. |
| /// is more than one use of a mutable borrow, and we don't want to accept too much | ||
| /// new code via two-phase borrows, so we try to limit where we create two-phase | ||
| /// capable mutable borrows. | ||
| /// See #49434 for tracking. |
There was a problem hiding this comment.
lovin' the comments <3
| match *self { | ||
| AutoBorrowMutability::Mutable { allow_two_phase_borrow } => | ||
| BorrowKind::Mut { allow_two_phase_borrow }, | ||
| BorrowKind::Mut { allow_two_phase_borrow: match allow_two_phase_borrow { |
There was a problem hiding this comment.
maybe we should propagate the AllowTwoPhase type here, instead of converting to bool?
| /// but we do need two-phase borrows for function argument reborrows. | ||
| /// See #47489 and #48598 | ||
| /// See docs on the "AllowTwoPhase" type for a more detailed discussion | ||
| allow_two_phase: AllowTwoPhase, |
There was a problem hiding this comment.
Again, lovin' the comments
| @@ -123,10 +130,13 @@ fn success<'tcx>(adj: Vec<Adjustment<'tcx>>, | |||
| } | |||
|
|
|||
| impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { | |||
| fn new(fcx: &'f FnCtxt<'f, 'gcx, 'tcx>, cause: ObligationCause<'tcx>) -> Self { | |||
| fn new(fcx: &'f FnCtxt<'f, 'gcx, 'tcx>, | |||
There was a problem hiding this comment.
Nit: these days, I tend to move over to rustfmt style when changing a sig anyway
fn new(
fcx: ...
cause: ...
allow_two_phase: ...,
) -> Self {
| // Indexing can be desugared to a method call, | ||
| // so maybe we could use two-phase here. | ||
| // See the documentation of AllowTwoPhase for why that's | ||
| // not the case today. |
There was a problem hiding this comment.
are you just referring to our general desire for caution? (I didn't see any specific notes about indexing)
|
@bors r+ |
|
📌 Commit d8352af has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=9 |
Extend two-phase borrows to apply to method receiver autorefs Fixes #48598 by permitting two-phase borrows on the autorefs created when functions and methods.
|
☀️ Test successful - status-appveyor, status-travis |
…tsakis two-phase borrows: support multiple activations in one statement The need for this has arisen since the introduction of two-phase borrows on method autorefs in #49348. r'ing @pnkfelix to keep things off Niko's plate so he can make this redundant, and @pnkfelix is familiar with the code. Fixes #49635 Fixes #49662 r? @pnkfelix
Fixes #48598 by permitting two-phase borrows on the autorefs created when functions and methods.