Stream: t-compiler/wg-nll

Topic: match-soundness


nikomatsakis (May 11 2018 at 10:54, on Zulip):

so @pnkfelix ... the question is

pnkfelix (May 11 2018 at 10:54, on Zulip):

(wait lets give some context for others)

nikomatsakis (May 11 2018 at 10:54, on Zulip):

I guess I agree it's probably nontrivial, but I feel like it might not be that hard to handle the ref mut case...

nikomatsakis (May 11 2018 at 10:54, on Zulip):

ok :)

pnkfelix (May 11 2018 at 10:54, on Zulip):

niko and I are talking about https://github.com/rust-lang/rust/issues/27282

nikomatsakis (May 11 2018 at 10:55, on Zulip):

(I imagine @Reed Koser would be interested in particular)

pnkfelix (May 11 2018 at 10:55, on Zulip):

in particular, there are two avenues for handling the ref mut problem at the end of this comment: https://github.com/rust-lang/rust/issues/27282#issuecomment-384393383

pnkfelix (May 11 2018 at 10:57, on Zulip):

Niko has long described handling this via a "hack" to ensure that the initial mut-borrow caused by the ref mut does not get treated as conflicting with the FakeUse of the shared-borrow of the match input.

pnkfelix (May 11 2018 at 10:58, on Zulip):

but then ariel pointed out that what we want essentially amounts to another instance of two-phase borrows:

pnkfelix (May 11 2018 at 10:58, on Zulip):

We want the ref mut to initially be a reservation of the matched location

pnkfelix (May 11 2018 at 10:59, on Zulip):

and occurrences of the binding within the guard should not activate. This is sound, since they will be implicitly & &mut (a shared-borrow of a mut-borrow, which you cannot mutate through).

pnkfelix (May 11 2018 at 10:59, on Zulip):

Niko and I both like this two-phase borrow approach to the issue

pnkfelix (May 11 2018 at 11:00, on Zulip):

And then we were talking about just the question of how best to implement this. Right now two-phase borrows are only allowed for autorefs and operators

pnkfelix (May 11 2018 at 11:01, on Zulip):

so we would need to increase the scope of that collection of forms that allow two-phase borrows. My question was, how much effort to put into an attempt to restrict it to this specific case.

nikomatsakis (May 11 2018 at 11:01, on Zulip):

so this is the function that creates the temporaries for ref mut bindings, right? And, more specifically, this line?

nikomatsakis (May 11 2018 at 11:01, on Zulip):

(Hmm, I see we create two temporaries, one for the guard and one for "realz", are we going to recreate the one "for realz" later if the guard is true, or just use it?)

nikomatsakis (May 11 2018 at 11:02, on Zulip):

If the latter, we would maybe have to tweak the two-phase system, since that would have more than one use

nikomatsakis (May 11 2018 at 11:02, on Zulip):

but .. hopefully not too hard to do

pnkfelix (May 11 2018 at 11:02, on Zulip):

I believe we use the already created temporary

pnkfelix (May 11 2018 at 11:02, on Zulip):

(and yes, I believe that you have correctly identified the relevant code.)

nikomatsakis (May 11 2018 at 11:03, on Zulip):

ok

pnkfelix (May 11 2018 at 11:03, on Zulip):

But maybe the simplest thing would be to not reuse the already created temporary, but rather recreate it...

nikomatsakis (May 11 2018 at 11:04, on Zulip):

that would indeed be simple

nikomatsakis (May 11 2018 at 11:04, on Zulip):

at least w/r/t to the 2-phase borrow setup

nikomatsakis (May 11 2018 at 11:04, on Zulip):

I don't really recall how "baked in" the "single use" concept is, but, I am a bit nervous otherwise because

nikomatsakis (May 11 2018 at 11:04, on Zulip):

we don't want users to "see" the second phase I don't think

nikomatsakis (May 11 2018 at 11:04, on Zulip):

in particular:

nikomatsakis (May 11 2018 at 11:05, on Zulip):
match foo {
   Some(ref mut x) => {
       let y = &foo; // should error, even if `x` is not later used
   }
}
pnkfelix (May 11 2018 at 11:05, on Zulip):

So I'll admit up front that I had been thinking I would have to mess around with the flag here: https://github.com/rust-lang/rust/blob/41707d8df9a441e19387a4a61415ee0af58a9e48/src/librustc_mir/hair/pattern/mod.rs#L484

nikomatsakis (May 11 2018 at 11:05, on Zulip):

so if we don't add a second borrow, then I think we would want to add an artificial use

nikomatsakis (May 11 2018 at 11:06, on Zulip):

I see, that's another option, but that would have a dramatic effect (unless we add those artificial uses :)

pnkfelix (May 11 2018 at 11:06, on Zulip):

But it sounds like I might not have to mess with the hair, necessarily, since you have pointed out that there is already much magic injected during MIR generation and maybe I can just focus on adding more magic there

nikomatsakis (May 11 2018 at 11:06, on Zulip):

I could see merits to either approach. It might be confusing that the flag is false but the borrow is nonetheless 2-phase :)

nikomatsakis (May 11 2018 at 11:06, on Zulip):

at least a comment seems appropriate

pnkfelix (May 11 2018 at 11:07, on Zulip):

wait

pnkfelix (May 11 2018 at 11:07, on Zulip):

You really do want an error in that case above?

nikomatsakis (May 11 2018 at 11:07, on Zulip):

oh well

nikomatsakis (May 11 2018 at 11:07, on Zulip):

actually proably not because NLL

pnkfelix (May 11 2018 at 11:07, on Zulip):

exactly

nikomatsakis (May 11 2018 at 11:07, on Zulip):
match foo {
   Some(ref mut x) => {
       let y = &foo; // should error, even though `x` is not yet used
       drop(x);
   }
}
nikomatsakis (May 11 2018 at 11:08, on Zulip):

now that I've discovered that mem::drop is in the prelude it's my new general purpose "read" primitive ;)

pnkfelix (May 11 2018 at 11:08, on Zulip):

Okay, now we are in an interesting position

pnkfelix (May 11 2018 at 11:08, on Zulip):

/me thinks about the new example

pnkfelix (May 11 2018 at 11:09, on Zulip):

I guess going down that path eventually leads to the same counter-examples that led us to restrict two-phase borrows in the first place

nikomatsakis (May 11 2018 at 11:09, on Zulip):

you mean if we permitted that example above?

pnkfelix (May 11 2018 at 11:09, on Zulip):

Yes

nikomatsakis (May 11 2018 at 11:10, on Zulip):

I would expect so, but there is an add'l reason, which is that I don't want to have to "teach" 2-phase borrows

pnkfelix (May 11 2018 at 11:10, on Zulip):

Right right

nikomatsakis (May 11 2018 at 11:10, on Zulip):

at least not until people are asking pretty advanced questions :)

nikomatsakis (May 11 2018 at 11:10, on Zulip):

(but I think that these sorts of examples would be confusing early on, without much benefit...)

nikomatsakis (May 11 2018 at 11:11, on Zulip):

if we were going to go that way, I'd want to develop a more 'well-rounded' 2-phase system

nikomatsakis (May 11 2018 at 11:11, on Zulip):

and I guess we have our hands full right now :)

nikomatsakis (May 11 2018 at 11:12, on Zulip):

brb gonna head out from my hotel, I think, and to a local cafe or some kind of "hipper" surroundings. In particular ones where there is decent coffee.

pnkfelix (May 11 2018 at 11:26, on Zulip):

But maybe the simplest thing would be to not reuse the already created temporary, but rather recreate it...

(on the flip side of this: I'm pretty sure there was a reason I did not take this approach from the outset. I.e. the way that the code is written, I had to do more work on the implementation side to avoid reuse. Maybe there was some move error resulting from the attempt to recreate? Or just attempting to reassign the temp was disallowed? I will find out...)

Jake Goulding (May 11 2018 at 13:49, on Zulip):

this is why I'm trying to read all this — to learn and reteach

pnkfelix (May 11 2018 at 20:40, on Zulip):

guess who spent several rustc compile cycles adding debug! statements trying to figure out why his extensions to the two-phase borrow to include this match stuff didn't seem to be having any effect ... only to eventually realize/remember that -Z borrowck=mir does not currently imply -Z two-phase-borrows...

pnkfelix (May 11 2018 at 20:51, on Zulip):

on the plus side: I now have several more debug! statements to look at as I now debug my code for real. :)

pnkfelix (May 11 2018 at 21:28, on Zulip):

Oh, super interesting. I think my branch now works, including this new slight expansion of two phase borrows... The only problem (other than the diagnostics being non-ideal) is that I seem to have stopped triggering the unused mut lint in one case, namely this:

    match (30, 2) {
      (mut x, 1) | //[lexical]~ ERROR: variable does not need to be mutable
                   //[nll]~^ ERROR: variable does not need to be mutable
      (mut x, 2) |
      (mut x, 3) => {
      }
      _ => {}
    }
pnkfelix (May 11 2018 at 21:29, on Zulip):

Which is super weird; I wonder if its because I slightly altered the MIR codegen so that the variables get constructed on the arm body itself...

pnkfelix (May 11 2018 at 21:50, on Zulip):

Hmm... fixed that particular example, but now its made me start exploring the test space around multiple patterns on one arm, and I'm starting to doubt whether extending two-phase borrows for this case is as trivial as we had thought...

pnkfelix (May 11 2018 at 21:53, on Zulip):

e.g. a case like

   match (30, 2) {
      (ref mut x, 1) |  (30, ref mut x)  if { true } => { }
      _ => {}
    }

is now going to have multiple definition sites for x. And for the guard, they'll want to each be borrows of distinct locations on the matched input...

pnkfelix (May 11 2018 at 21:53, on Zulip):

But now its almost midnight so I think its time for me to put this away

nikomatsakis (May 11 2018 at 22:41, on Zulip):

@pnkfelix the unused mut lint does have some .. oddities as well

pnkfelix (May 15 2018 at 21:36, on Zulip):

woot: https://github.com/rust-lang/rust/pull/50783

pnkfelix (May 15 2018 at 21:37, on Zulip):

(and now time for me to get ready for bed)

nikomatsakis (May 15 2018 at 21:48, on Zulip):

Nice

pnkfelix (May 29 2018 at 00:39, on Zulip):

@nikomatsakis btw I think I’m done revising 50783 so you can just double check my note about whether you had already reviewed the extension to two-phase borrows

nikomatsakis (May 29 2018 at 08:12, on Zulip):

ok

nikomatsakis (May 29 2018 at 08:30, on Zulip):

@pnkfelix by "the extension to two-phase borrows", do you mean this commit? That is, considering a use solely in a borrow to not activate?

pnkfelix (May 29 2018 at 08:32, on Zulip):

Is that ab17cd4 ? It looks like it (and that is the correct one, at the current rebase state )

nikomatsakis (May 29 2018 at 08:34, on Zulip):

it is

Last update: Nov 21 2019 at 14:00UTC