Stream: t-compiler/wg-nll

Topic: #56256 prohibit 2pb with existing borrows


nikomatsakis (Jan 03 2019 at 18:46, on Zulip):

So if I recall from the last time we discussed this, a first step that we could take was to remove the use of 2PB from within the match desugaring code, and replace it with some sort of special cast. (i.e., for a ref mut binding used in a guard, we create a value of &&Foo type and then cast that to &&mut Foo)

@Matthew Jasper, I recall you did some of the initial work pursuing these changes, do you have any interest in trying to do that refactoring?

If I understand correctly, it should be a "no-op" in terms of its effect on end-users -- i.e., the same set of code should be accepted, it would just be being accepted by relying on a "cast" instead of 2PB.

nikomatsakis (Jan 03 2019 at 18:47, on Zulip):

I guess an important question is whether we plan to actually prohibit 2PB-with-pre-existing-borrows or not. But this seems like a first step we can take in either case. OTOH, if we don't plan to prohibit pre-existing borrows...

nikomatsakis (Jan 03 2019 at 18:47, on Zulip):

ah I remember there were some comments I hadn't read yet

nikomatsakis (Jan 03 2019 at 18:59, on Zulip):

(I guess one point is that I think that the idea of using a cast is not a bad idea regardless; it feels like sort of a "later" move in terms of code cleanliness. This case always a weird 2PB case since it was never activated.)

Matthew Jasper (Jan 03 2019 at 19:03, on Zulip):

Yes, I was going to create a PR to change to a cast for matches, but then I realized that the "variable" in the match guard could have a subtype of the type of the variable in the arm and decided it would need more work or more thought.

nikomatsakis (Jan 03 2019 at 19:05, on Zulip):

hmm, say more?

nikomatsakis (Jan 03 2019 at 19:05, on Zulip):

do you have a specific example in mind?

Matthew Jasper (Jan 03 2019 at 19:08, on Zulip):
fn eq_types<T>(_: &&mut T, _: &mut T)

let mut x: &'static i32 = &0;
match x {
    ref mut z if eq_types(&z, &mut 0) => (), // z: &mut &'local i32 !?
    _ => (),
}

It's not unsafe, just (somewhat) unexpected.

nikomatsakis (Jan 03 2019 at 19:15, on Zulip):

so, let me see if I can say this explicitly:

nikomatsakis (Jan 03 2019 at 19:16, on Zulip):

seems true. we could force the issue but it'd be a bit awkward

nikomatsakis (Jan 03 2019 at 19:16, on Zulip):

e.g., that cast that converts the &&&i32 to a &&mut &i32 could conceivably take also the "place" that was originally borrowed, to equate the types

nikomatsakis (Jan 03 2019 at 19:24, on Zulip):

seems true, though I'm not sure if I am worried about it

Matthew Jasper (Jan 03 2019 at 19:26, on Zulip):

I would be surprised if it's noticed very often, but it is a difference compared to the current behaviour.

Matthew Jasper (Jan 03 2019 at 19:27, on Zulip):

Anyway, I think I could get this implemented soon.

nikomatsakis (Jan 03 2019 at 19:28, on Zulip):

that'd be great. Seems like we can decide whether/how to address the other problem once we have the outlines of the impl

Matthew Jasper (Jan 04 2019 at 19:17, on Zulip):

OK, but this one which currently compiles on my branch is too far...

fn main() {
    fn ref_to_static<T: 'static>(t: &T) {}
    match 0 {
        ref mut z if { // Apparently we try to promote per use
            ref_to_static(&z);
            false
        } => (),
        _ => (),
    }
}
Matthew Jasper (Jan 04 2019 at 21:15, on Zulip):

and this...

fn bar() {
    match &0 {
        &ref mut z if return => (),
        _ => (),
    }
}
nikomatsakis (Jan 04 2019 at 22:06, on Zulip):

so, &0 has static lifetime..

nikomatsakis (Jan 04 2019 at 22:06, on Zulip):

ah, ok, I see

nikomatsakis (Jan 04 2019 at 22:06, on Zulip):

ref mut z, right :)

nikomatsakis (Jan 04 2019 at 22:07, on Zulip):

hmm, so, one thing I was thinking about last night is whether we want this to be a distinct kind of borrow (ugh...)

nikomatsakis (Jan 04 2019 at 22:08, on Zulip):

(though so long as the resulting type is &T, I don't think it would solve the covariance issues)

nikomatsakis (Jan 04 2019 at 22:09, on Zulip):

(I guess the point with the latter example is that the actual mutable borrow winds up being dead code?)

Matthew Jasper (Jan 04 2019 at 23:15, on Zulip):

Indeed.

Matthew Jasper (Jan 04 2019 at 23:17, on Zulip):

hmm, so, one thing I was thinking about last night is whether we want this to be a distinct kind of borrow (ugh...)

This is the best thing I've thought of for now, but it's definitely not great.

nikomatsakis (Jan 07 2019 at 15:43, on Zulip):

@Matthew Jasper I'm trying to decide what I think about those examples being accepted. I feel like it's not horrible if they are -- I recall us talking about how much to expose the "desugaring" of bindings in guards before, and I continue to think that it's reasonable to expose to people (in extreme cases...) that bindings in a guard are actually shared borrows "at heart". But I'm wondering, do you remember when this came up before exactly and what we did then?

Matthew Jasper (Jan 07 2019 at 17:44, on Zulip):

This has come up previously in this thread, and the decision that the strangeness was ok.

Matthew Jasper (Jan 07 2019 at 17:46, on Zulip):

The issue was that the reference in the guard would alias the reference in being matched on and both could be used.

Matthew Jasper (Jan 07 2019 at 21:19, on Zulip):

Specifically, this

use std::ptr;

fn main() {
    let x = &mut 0;
    match x {
        &mut ref mut z if !ptr::eq(&z, &x) && ptr::eq(z, x) => println!("aliasing mutable references?"),
        _ => (),
    }
}
Matthew Jasper (Jan 07 2019 at 21:22, on Zulip):

The only really bad issue we would have with a naive lowering is the promotion issue, but this seems like it's a bug in the promotion code

fn main() {
    let y: *const i32;
    match 0 {
        ref mut z if { y = z; true } => assert_eq!(y, z as *const _, "???"),
        _ => (),
    }
}
Matthew Jasper (Jan 07 2019 at 21:25, on Zulip):

Notably the assert doesn't fire here (because the Context is a mutating projection, not a mutable borrow)

fn main() {
    let y: *const i32;
    match (0,) {
        (ref mut z,) if { y = z; true } => assert_eq!(y, z as *const _, "???"),
        _ => (),
    }
}
nikomatsakis (Jan 07 2019 at 21:35, on Zulip):

(I think, on a related note, that you can observe that &z changes in the guard, but that is also true today)

Matthew Jasper (Jan 14 2019 at 21:47, on Zulip):

So, I've tried a different approach that takes the naive match lowering and works around it to add the required fake borrows. I'm hoping that the rarity of ref mut x if ... means that no one has written code that it would break (neither the compiler, nor any of its tests, contain such code).

Matthew Jasper (Jan 14 2019 at 21:47, on Zulip):

If someone has then it can be changed to use (either kind of) two-phase borrows, using the same migration warnings as for other two-phase borrows.

nikomatsakis (Jan 15 2019 at 21:34, on Zulip):

@Matthew Jasper this is part of your recent PR?

Matthew Jasper (Jan 15 2019 at 21:36, on Zulip):

This is what the recent PR is for, it's waiting on crater to see if the "real mutable borrows" approach is possible.

nikomatsakis (Jan 15 2019 at 21:37, on Zulip):

I haven't had a chance to read it yet

nikomatsakis (Jan 15 2019 at 21:37, on Zulip):

so I'm not quite sure what "real mutable borrows" means exactly :)

nikomatsakis (Jan 15 2019 at 21:37, on Zulip):

I think the reason we didn't do that before was because it would have conflicted with the borrows of the thing we are matching against

nikomatsakis (Jan 15 2019 at 21:37, on Zulip):

but I guess that is what you changed, something in how we do those borrows?

Matthew Jasper (Jan 15 2019 at 21:38, on Zulip):

Yes, the idea (in either version) is that we can assign the fake borrows again at the start of any match guard to avoid conflicting with the borrow access from the ref mut.

Matthew Jasper (Jan 15 2019 at 21:39, on Zulip):

Then either some changes to fake borrows, or (minimal) two-phase borrows ensure that there's no conflict with the borrow from ref mut.

nikomatsakis (Jan 16 2019 at 14:30, on Zulip):

@Matthew Jasper are you around now by any chance? I'm skimming over your PR now

nikomatsakis (Jan 16 2019 at 14:31, on Zulip):

(and I had a question or two)

nikomatsakis (Jan 16 2019 at 14:47, on Zulip):

I think it would be helpful to show the desugared form for some simple example

Matthew Jasper (Jan 16 2019 at 17:52, on Zulip):

I'll try to writeup some notes on this soon

nikomatsakis (Jan 16 2019 at 17:58, on Zulip):

thanks -- I read the diff but I'm not sure I "got it" yet

nikomatsakis (Jan 16 2019 at 17:58, on Zulip):

I might have to re-read :)

nikomatsakis (Jan 16 2019 at 20:59, on Zulip):

Link to summary comment, for convenience =)

nikomatsakis (Jan 28 2019 at 20:30, on Zulip):

@Matthew Jasper ok does this description sound accurate?

Matthew Jasper (Feb 02 2019 at 10:59, on Zulip):

Branch with the new match lowering for anyone interested: https://github.com/matthewjasper/rust/tree/cleanup-match-lowering

Last update: Nov 21 2019 at 23:40UTC