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

Use normal mutable borrows in matches #57609

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@matthewjasper
Copy link
Contributor

matthewjasper commented Jan 14, 2019

ref mut borrows are currently two-phase with NLL enabled. This changes them to be proper mutable borrows. To accommodate this, first the position of fake borrows is changed:

[ 1. Pre-match ]
       |
[ (old create fake borrows) ]
[ 2. Discriminant testing -- check discriminants ] <-+
       |                                             |
       | (once a specific arm is chosen)             |
       |                                             |
[ (old read fake borrows) ]                          |
[ 3. Create "guard bindings" for arm ]               |
[ (create fake borrows) ]                            |
       |                                             |
[ 4. Execute guard code ]                            |
[ (read fake borrows) ] --(guard is false)-----------+
       |
       | (guard results in true)
       |
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]

The following additional changes are made to accommodate ref mut bindings:

  • We no longer create fake Shared borrows. These borrows are no longer needed for soundness, just to avoid some arguably strange cases.
  • Shallow borrows no longer conflict with existing borrows, avoiding conflicting access between the guard borrow access and the ref mut borrow.

There is some further clean up done in this PR:

  • Avoid the "later used here" note for Shallow borrows (since it's not relevant with the message provided)
  • Make any use of a two-phase borrow activate it.
  • Simplify the cleanup_post_borrowck passes into a single pass.

cc #56254

r? @nikomatsakis

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 14, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

⌛️ Trying commit 34f7a41 with merge 26f8d25...

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57609 - matthewjasper:more-restrictive-match, r=<try>
[WIP] Use normal mutable borrows in matches

`ref mut` borrows are currently two-phase with NLL enabled. This changes them to be proper mutable borrows. To accommodate this:

* Fake borrows are no longer created for places that are merely bound to a variable in a match. These borrows weren't needed for soundness, just to avoid some arguably strange cases.
* As such all fake borrows are `Shallow` (renamed to `Guard`) borrows.
* All the fake borrows are repeated at the start of every guard, avoiding a conflict between the access from `ref mut` bindings and the guard borrow.
* Guard borrows no longer conflict with existing borrows, avoiding conflicting access between the guard borrow access and the `ref mut` borrow.
* We use a `FakeRead` at the start of the match to ensure that there are no existing mutable borrows.

Posting now for a crater run to see if this breaks any real world code. If it does, then this approach can be changed to use 2-phase borrows.

cc #56254

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 15, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 15, 2019

@rust-lang/infra please can this have a crater run?

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 15, 2019

@craterbot run start=master#03acbd71c977cd63ce5f39ba9b6fe9ffd578785a end=try#26f8d250bb3c8a85e812cf663c730ef9e0df9315 mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 15, 2019

👌 Experiment pr-57609 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 15, 2019

🚧 Experiment pr-57609 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 16, 2019

🎉 Experiment pr-57609 is completed!
📊 0 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 16, 2019

A bit of a longer write up on the two options here then.

CFG construction changes

For reference, the generated CFG for a match with 3 arms where the first one has a guard now looks something like this:

match-cfg-new

The binding_blocks array is new (the blocks in it were mostly there already).

Fake borrows changes

Fake borrows are now added not only to the start block, but also to the end of any binding block that are proceeded by a guard. This means that the fake borrows are not live for the binding block and so do not conflict with any ref mut bindings.

Option A (commit 1)

At this point using minimal two-phase borrows is compatible with the match lowering. Some notes:

  • This is the more minimal change to the current state
  • This ties what's allowed in matches to what's allowed with 2 phase borrows
  • Requires 2 + #patterns in the same arm different versions of the variable.
  • Uses more fake borrows.

Option B (commit 3)

This uses real borrows. See OP for some details. Some note:

  • This doesn't allow anything that the naive lowering doesn't.
  • There are only 2 versions of each variable.
  • The bound variable has a stable address.
  • Requires changes to how the borrow checker handles fake borrows and fake reads
  • Can cause some strange errors if a borrow from the guard conflicts with a borrow in the arm body.
  • Potentially breaks code (although the crater run was clean).
Show resolved Hide resolved src/librustc/mir/mod.rs Outdated
Show resolved Hide resolved src/librustc/mir/mod.rs Outdated
Show resolved Hide resolved src/librustc/ich/impls_mir.rs Outdated
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 18, 2019

@matthewjasper ok, I read your comment, but I still feel like I don't fully understand what is being proposed. Probably I'm just missing a certain amount of context. Let me just pepper out a few questions I guess:

Fake borrows are now added not only to the start block, but also to the end of any binding block that are proceeded by a guard.

When you say "fake borrows", you mean that we put dummy borrows like TMP = &foo, right? (Where the only uses of this TMP are artificial.) If I recall, we issue "shallow" borrows for basically all paths that lead to a discriminant that may be examined, more or less?

This means that the fake borrows are not live for the binding block...

Why does it mean that? Because the "fake borrows" we inserted at the start block never wind up being used? Are they live only during the matching code that we generate, which inspects the discriminants and so forth, or are they ever live for "user code" (i.e., guards)? If they are only live for code we generate, then it doesn't seem like they play much of a role, do they? (Since we trust that code not to mutate things.) Maybe they are needed for initialization checks or something?

At this point using minimal two-phase borrows is compatible with the match lowering.

What is a "minimal" 2PB? And which borrows are becoming 2PB exactly? Do you mean for the ref mut borrow that is used in the guard?

I feel like what I might like to see is some kind "representative desugaring" of a match in both styles.

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 18, 2019

A simple example of the generated MIR for nightly and for this branch is here https://gist.github.com/matthewjasper/fb0f4bfb9d3a6d179ed0c2eb7a5b0da5 I've marked the 2-phase borrow that's created on nightly.

When you say "fake borrows", you mean that we put dummy borrows like TMP = &foo, right? (Where the only uses of this TMP are artificial.) If I recall, we issue "shallow" borrows for basically all paths that lead to a discriminant that may be examined, more or less?

Yes. In the example above, all of the shallow/guard borrows are "fake borrows". See bb7[4] and bb7[5] in the branch code for where they're being added.

Why does it mean that? Because the "fake borrows" we inserted at the start block never wind up being used? Are they live only during the matching code that we generate, which inspects the discriminants and so forth, or are they ever live for "user code" (i.e., guards)? If they are only live for code we generate, then it doesn't seem like they play much of a role, do they? (Since we trust that code not to mutate things.) Maybe they are needed for initialization checks or something?

They're live in all guards (and while we're branching). They're not live outside of the match expression, in the arm expressions or after we've selected a pattern and are creating bindings for it - either the bindings for the guard or the bindings for the arm.

What is a "minimal" 2PB? And which borrows are becoming 2PB exactly?

I mean the version of 2 phase borrows discussed in #56254. The two-phase borrows are the ones that are already two phase: the borrows associated to ref mut bindings for the guard. (e.g. the one at bb9[2] on nightly)

@matthewjasper matthewjasper force-pushed the matthewjasper:more-restrictive-match branch from 34f7a41 to be15224 Jan 19, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 24, 2019

Sorry @matthewjasper been very distracted. I've added a calendar event to review and ponder this for Mon Jan 28, so hopefully that will help.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 28, 2019

@matthewjasper

OK, I read the desugared example in your gist. Allow me to attempt to re-summarize what is going on.

First off, the match process looks vaguely like this:

[ 1. Pre-match ]
       |
[ 2. Discriminant testing -- check discriminants ] <-+
       | (once a specific arm is chosen)             |
[ 3. Create "guard bindings" for arm ]               |
       |                                             |
[ 4. Execute guard code ] --(guard is false)---------+
       | (guard results in true)
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]

Does this sound right?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 28, 2019

To redraw my flow graph with fake borrows inserted, it looks something like this:

[ 1. Pre-match ]
[ (create fake borrows) ]
       |
[ 2. Discriminant testing -- check discriminants ] <-+
       |                                             |
       | (once a specific arm is chosen)             |
       |                                             |
[ (read fake borrows) ]                              |
[ 3. Create "guard bindings" for arm ]               |
[ (recreate fake borrows) ]                          |
       |                                             |
[ 4. Execute guard code ] --(guard is false)---------+
       |
       | (guard results in true)
       |
[ (read fake borrows) ]
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 28, 2019

If that is the case, I am wondering what the role of the fake borrows being live during "discriminant testing" is. I don't think they're harmful per se, just trying to create an accurate model of what the "threats" are.

I think there are a few things that users can do that are problematic:

  • Use guard code to mutate something that is being matched
    • prevented by the fake borrows being live over the guard code
  • Move things out that are being matched during the guard code
    • but that is prevented by desugaring x to the deref of a shared borrow
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 28, 2019

So, first a couple of my own versions of the diagram:

[ 1. Pre-match ]
[ (create fake borrows) ] (*)
       |
[ 2. Discriminant testing -- check discriminants ] <-+
       |                                             |
       | (once a specific arm is chosen)             |
       |                                             |                            |
[ (read fake borrows) ]                              |
[ 3. Create "guard bindings" for arm (optional) ]    |
[ (recreate fake borrows ) ] (**)                    |
       |                                             |
[ 4. Execute guard code (opt) ] --(guard is false)---+
       |
       | (guard results in true)
       |
[ ( no fake read ) ] (+)
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]

In the case of no guard

[ 1. Pre-match ]
[ (create fake borrows) ]
       |
[ 2. Discriminant testing -- check discriminants ]
       |
       | (once a specific arm is chosen) 
       |     
[ (read fake borrows) ] (***)
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]

No, there's no read here at the moment (you link to a fake read at (***)). It would certainly be nicer to have them there instead of where they currently are if it's not too annoying to implement. Currently we rely on the false edge to ensure that they are live during the guard.

If that is the case, I am wondering what the role of the fake borrows being live during "discriminant testing" is.

As well as the above I think you're asking why (*) exists, because that borrow is never live across user code. The reason is to (somewhat hackily) prevent a fake borrow from being live along a (**) -> [ Exit match ] -> (loop) -> [Pre-match] -> (***) path.

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Jan 30, 2019

So after some discussion on Zulip, I think that me and @nikomatsakis are agreed that we want the match CFG to look like this

[ 1. Pre-match ]
       |
[ 2. Discriminant testing -- check discriminants ] <-+
       |                                             |
       | (once a specific arm is chosen)             |
       |                                             |
[ 3. Create "guard bindings" for arm ]               |
[ (create fake borrows) ]                            |
       |                                             |
[ 4. Execute guard code ]                            |
[ (read fake borrows) ] --(guard is false)-----------+
       |
       | (guard results in true)
       |
[ 5. Create real bindings and execute arm ]
       |
[ Exit match ]

If there's no guard then 3. 4. and the fake borrows are omitted. This then gets us to the point where we can make 2-phase borrows conflict with all existing borrows, and possibly make the changes in the OP for being able to use real mutable borrows.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 20, 2019

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

@matthewjasper matthewjasper force-pushed the matthewjasper:more-restrictive-match branch from d937499 to bc474ff Feb 20, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 20, 2019

NLL triage. Reassigning to me for (hopefully final) review.

@pnkfelix pnkfelix assigned pnkfelix and unassigned nikomatsakis Feb 20, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 21, 2019

This looks great!

I came very close to r+'ing immediately. But I think I want clarity on the question I asked regarding the change to the diagnostic example. After I understand that change, then r=me once remaining nits are addressed.

matthewjasper added some commits Feb 2, 2019

Clean up MIR match lowering
* Adjust fake borrows to only be live over guards.
* Remove unused `slice_len_checked` field.
* Split the methods on builder into those for matches and those for all
  kinds of pattern bindings.
Activate two phase borrows on all uses
Two phase borrows are only used for adjustments now, so there's no need
to not activate them for shared borrows.

@matthewjasper matthewjasper force-pushed the matthewjasper:more-restrictive-match branch from bc474ff to 5ffc919 Feb 21, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 22, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 22, 2019

📌 Commit 5ffc919 has been approved by pnkfelix

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 23, 2019

Too big to rollup, @bors p=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment