NLL: Limit two-phase borrows to autoref-introduced borrows #47489

pnkfelix · 2018-01-16T10:40:26Z

This imposes a restriction on two-phase borrows so that it only applies to autoref-introduced borrows.

The goal is to ensure that our initial deployment of two-phase borrows is very conservative. We want it to still cover the v.push(v.len()); example, but we do not want it to cover cases like let imm = &v; let mu = &mut v; mu.push(imm.len());

(Why do we want it to be conservative? Because when you are not conservative, then the results you get, at least with the current analysis, are tightly coupled to details of the MIR construction that we would rather remain invisible to the end user.)

Fix #46747

I decided, for this PR, to add a debug-flag -Z two-phase-beyond-autoref, to re-enable the more general approach. But my intention here is not that we would eventually turn on that debugflag by default; the main reason I added it was that I thought it was useful for writing tests to be able to write source that looks like desugared MIR.

rust-highfive · 2018-01-16T10:40:36Z

r? @michaelwoerister

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

pnkfelix · 2018-01-16T10:40:50Z

the main reason I added it was that I thought it was useful for writing tests to be able to write source that looks like desugared MIR.

(Incidentally, I keep toying with the idea of adding an opt-in way to write MIR directly, something analogous to how asm.js lets one write low-level operations using Javascript syntax.)

pnkfelix · 2018-01-16T10:40:57Z

r? @nikomatsakis

bors · 2018-01-16T11:05:37Z

☔ The latest upstream changes (presumably #47209) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-01-16T18:24:02Z

src/librustc_mir/hair/cx/expr.rs

@@ -111,7 +112,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>,
                span,
                kind: ExprKind::Borrow {
                    region: deref.region,
-                    borrow_kind: to_borrow_kind(deref.mutbl),
+                    borrow_kind: to_borrow_kind(deref.mutbl, true),


I think I expected us to push these flags even further back, to type check. I'm a bit wary that we may introduce adjustments for reasons that are not "autoref-autoderef in conjunction with method calls".

For example, does this type-check?

fn foo(x: &mut u32, y: u32) { *x += y; } fn bar(x: &mut u32) { foo(x, *x) }

I think it might, because we get an implicit &mut *x on the call, and that might be treated as two-phase. Similarly let x: &mut u32 = x or something like that.

(Actually, I suppose we could conceivably thread a bit of state down during HAIR lowering to indicate whether the expression being coerced is in receiver position.)

I will figure out how to tighten the restriction. (As you have pointed out there may be different paths to getting there.)

(Also that example you have does leak through in the current state of this PR, so I will also add it as a compile-fail test)

(Though of course the example you give is sound; its just code that might surprise some if it passes, right? And anyway I agree that for our first deployment we should reject it)

Yes, just surprising

arielb1 · 2018-01-16T21:36:27Z

src/librustc/ich/impls_mir.rs

@@ -20,7 +20,8 @@ use std::mem;
 impl_stable_hash_for!(struct mir::GeneratorLayout<'tcx> { fields });
 impl_stable_hash_for!(struct mir::SourceInfo { span, scope });
 impl_stable_hash_for!(enum mir::Mutability { Mut, Not });
-impl_stable_hash_for!(enum mir::BorrowKind { Shared, Unique, Mut });
+impl_stable_hash_for!(struct mir::BorrowKind { mut_kind, introduced_via_autoref });
+impl_stable_hash_for!(enum mir::BorrowMutKind { Shared, Unique, Mut });


bikeshed: name looks confusing, I would prefer BorrowMutability.

(Hey look who it is!)

will do!

arielb1 · 2018-01-16T21:37:07Z

src/librustc/ich/impls_mir.rs

@@ -20,7 +20,8 @@ use std::mem;
 impl_stable_hash_for!(struct mir::GeneratorLayout<'tcx> { fields });
 impl_stable_hash_for!(struct mir::SourceInfo { span, scope });
 impl_stable_hash_for!(enum mir::Mutability { Mut, Not });
-impl_stable_hash_for!(enum mir::BorrowKind { Shared, Unique, Mut });
+impl_stable_hash_for!(struct mir::BorrowKind { mut_kind, introduced_via_autoref });


bikeshed: I would prefer the "operational" always_allow_2phi to the descriptive introduced_via_autoref.

please call it "two phase", but yes that's a good point. Operational is often better.

okay, this distinction doesn't matter to me, so I'll make this change.

pnkfelix · 2018-01-23T16:06:02Z

(sorry this is taking so long; I keep hitting #47381 and so that is slowing my progress

kennytm · 2018-01-31T10:02:05Z

Marking as blocked by #47381.

pnkfelix · 2018-01-31T21:31:15Z

(I'm hoping that now that I've rebased atop a master has ThinLTO turned off that I might stop hitting #47381? Though I'm not sure if that change has propagated yet into the betas that are used to build the stage1 rustc ...)

pnkfelix · 2018-02-06T11:47:16Z

@shepmaster I assume that you switched it from S-waiting-on-author to S-blocked because of #47381 ?

Just wanted to confirm, as after rebasing atop newer master, that bug is no longer blocking me. (And thus I am now switching it back to S-waiting-on-author)

shepmaster · 2018-02-06T14:15:04Z

you switched it from S-waiting-on-author to S-blocked because of #47381 ?

That's right!

nikomatsakis · 2018-02-06T18:49:24Z

@pnkfelix at first glance, changes look good but appveyor won't run because of the merge conflict

pnkfelix · 2018-02-07T12:43:23Z

src/librustc_mir/hair/cx/expr.rs

+        match *self {
+            AutoBorrowMutability::Mutable { allow_two_phase_borrows } =>
+                BorrowKind { mut_kind: BorrowMutability::Mut,
+                             // FIXME ugh typo/inconsisten pluralization


heh I guess I should address this FIXME now that I have this working again

pnkfelix · 2018-02-07T12:45:03Z

src/librustc_typeck/check/mod.rs

+                            // another kind of call"; perhaps it would
+                            // be more consistent to allow two-phase
+                            // borrows for .index() receivers here.
+                            allow_two_phase_borrows: false,


(I think the test illustrates the kind of code that is being filtered out by not allowing two-phase borrows for index operators... I'll leave another comment with my thoughts on that test itself.)

pnkfelix · 2018-02-07T12:47:52Z

src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs

+    double_access(&mut a, &a);
+    //[lxl]~^   ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]
+    //[nll]~^^  ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]
+    //[g2p]~^^^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502]


Important: even the generalized form tested by g2p/-Z two-phase-beyond-autoref (thus no longer connected to autoref) is still rejecting this example. Compare that against deref_coercion above...

pnkfelix · 2018-02-07T12:49:25Z

src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs

+
+fn coerce_index_op() {
+    let mut i = I(10);
+    i[i[3]] = 4;


so the generalized (non-autoref-connected) g2p form starts allowing things like i[i[3]] = ...;

I don't know about other people, but I think ... that seems okay to allow...? (As in, it seems like a reasonable extension to consider, if not in the immediate release of two-phase borrows, then at some point in the future?)

It works for normal slices, so it seems ok to me.

pnkfelix · 2018-02-08T11:04:31Z

(skimming over the commit series, I realize now that some of the last minute refactorings I did yesterday brought the code into a state where I could get the diff much smaller. I'll take care of that now.)

…o autoref. This is foundation for issue 46747 (limit two-phase borrows to method-call autorefs).

Added `-Z two-phase-beyond-autoref` to bring back old behavior (mainly to allow demonstration of desugared examples). Updated tests to use aforementioned flag when necessary. (But in each case where I added the flag, I made sure to also include a revision without the flag so that one can readily see what the actual behavior we expect is for the initial deployment of NLL.)

…se borrow info too. Namely, the mutable borrows also carries a flag indicating whether they should support two-phase borrows. This allows us to thread down, from the point of the borrow's introduction, whether the particular adjustment that created it is one that yields two-phase mutable borrows.

…phase borrows.

pnkfelix · 2018-02-08T11:19:46Z

@nikomatsakis okay I am done polishing; you should be safe to do a review without me push -f'ing in parallel. :)

nikomatsakis · 2018-02-08T18:17:35Z

@bors r+

bors · 2018-02-08T18:17:36Z

📌 Commit b55cd8c has been approved by nikomatsakis

bors · 2018-02-09T02:26:51Z

⌛ Testing commit b55cd8c with merge afa8acc...

NLL: Limit two-phase borrows to autoref-introduced borrows This imposes a restriction on two-phase borrows so that it only applies to autoref-introduced borrows. The goal is to ensure that our initial deployment of two-phase borrows is very conservative. We want it to still cover the `v.push(v.len());` example, but we do not want it to cover cases like `let imm = &v; let mu = &mut v; mu.push(imm.len());` (Why do we want it to be conservative? Because when you are not conservative, then the results you get, at least with the current analysis, are tightly coupled to details of the MIR construction that we would rather remain invisible to the end user.) Fix #46747 I decided, for this PR, to add a debug-flag `-Z two-phase-beyond-autoref`, to re-enable the more general approach. But my intention here is *not* that we would eventually turn on that debugflag by default; the main reason I added it was that I thought it was useful for writing tests to be able to write source that looks like desugared MIR.

bors · 2018-02-09T05:22:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing afa8acc to master...

rust-highfive assigned michaelwoerister Jan 16, 2018

rust-highfive assigned nikomatsakis and unassigned michaelwoerister Jan 16, 2018

pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2018

nikomatsakis reviewed Jan 16, 2018

View reviewed changes

arielb1 reviewed Jan 16, 2018

View reviewed changes

pnkfelix 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 Jan 18, 2018

kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2018

pnkfelix added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 31, 2018

pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 6, 2018

pnkfelix force-pushed the limit-2pb-issue-46747 branch from 5154cc0 to 0c108b1 Compare February 6, 2018 15:55

pnkfelix force-pushed the limit-2pb-issue-46747 branch from 0c108b1 to a9af49e Compare February 7, 2018 12:41

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

pnkfelix commented Feb 7, 2018

View reviewed changes

pnkfelix force-pushed the limit-2pb-issue-46747 branch 2 times, most recently from 028ca2b to 5c8ff83 Compare February 8, 2018 11:00

pnkfelix added 5 commits February 8, 2018 12:16

Encode (in MIR) whether borrows are explicit in source or arise due t…

c00266b

…o autoref. This is foundation for issue 46747 (limit two-phase borrows to method-call autorefs).

Test that autoref'ing beyond method receivers does not leak into two-…

81b93fa

…phase borrows.

Fleshed out the test a lot more.

b55cd8c

pnkfelix force-pushed the limit-2pb-issue-46747 branch from 5c8ff83 to b55cd8c Compare February 8, 2018 11:18

nikomatsakis approved these changes Feb 8, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2018

bors merged commit b55cd8c into rust-lang:master Feb 9, 2018

bors mentioned this pull request Feb 9, 2018

Fixes NLL: error from URL crate #47917

Merged

nikomatsakis mentioned this pull request Feb 13, 2018

two-phase borrows are causing #[feature(nll)] to reject a mutable borrow incorrectly #48070

Closed

pnkfelix mentioned this pull request Feb 22, 2018

NLL: Redo two-phase borrows with conservative, narrow focus #48431

Closed

pnkfelix mentioned this pull request Mar 28, 2019

Tracking issue for mutable_borrow_reservation_conflict compatibility lint #59159

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL: Limit two-phase borrows to autoref-introduced borrows #47489

NLL: Limit two-phase borrows to autoref-introduced borrows #47489

pnkfelix commented Jan 16, 2018

rust-highfive commented Jan 16, 2018

pnkfelix commented Jan 16, 2018

pnkfelix commented Jan 16, 2018

bors commented Jan 16, 2018

nikomatsakis Jan 16, 2018

nikomatsakis Jan 17, 2018

pnkfelix Jan 18, 2018

pnkfelix Jan 19, 2018

pnkfelix Jan 19, 2018

nikomatsakis Jan 19, 2018

arielb1 Jan 16, 2018

pnkfelix Jan 18, 2018

arielb1 Jan 16, 2018

nikomatsakis Jan 17, 2018

pnkfelix Jan 18, 2018

pnkfelix commented Jan 23, 2018

kennytm commented Jan 31, 2018

pnkfelix commented Jan 31, 2018

pnkfelix commented Feb 6, 2018

shepmaster commented Feb 6, 2018

nikomatsakis commented Feb 6, 2018

pnkfelix Feb 7, 2018

pnkfelix Feb 7, 2018

pnkfelix Feb 7, 2018

pnkfelix Feb 7, 2018 •

edited

Loading

nikomatsakis Feb 7, 2018

pnkfelix commented Feb 8, 2018

pnkfelix commented Feb 8, 2018

nikomatsakis commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 9, 2018

bors commented Feb 9, 2018

NLL: Limit two-phase borrows to autoref-introduced borrows #47489

NLL: Limit two-phase borrows to autoref-introduced borrows #47489

Conversation

pnkfelix commented Jan 16, 2018

rust-highfive commented Jan 16, 2018

pnkfelix commented Jan 16, 2018

pnkfelix commented Jan 16, 2018

bors commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Jan 23, 2018

kennytm commented Jan 31, 2018

pnkfelix commented Jan 31, 2018

pnkfelix commented Feb 6, 2018

shepmaster commented Feb 6, 2018

nikomatsakis commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Feb 8, 2018

pnkfelix commented Feb 8, 2018

nikomatsakis commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 9, 2018

bors commented Feb 9, 2018

pnkfelix Feb 7, 2018 •

edited

Loading