Stream: t-compiler/wg-nll

Topic: #53114, #53438 - `_` patterns should not count as borrows


Matthew Jasper (Aug 30 2018 at 19:40, on Zulip):

@nikomatsakis I realized after your comment that discriminants aren't places, so we can't borrow one. I guess I could try making it one or alternatively make a new StatementKind/Rvalue for them. I guess I could try both and see how they compare.

Matthew Jasper (Aug 30 2018 at 19:41, on Zulip):

I think that this is probably the approach that seems most natural.

nikomatsakis (Aug 31 2018 at 10:03, on Zulip):

@nikomatsakis I realized after your comment that discriminants aren't places, so we can't borrow one. I guess I could try making it one or alternatively make a new StatementKind/Rvalue for them. I guess I could try both and see how they compare.

actually, we have a hack here...

nikomatsakis (Aug 31 2018 at 10:04, on Zulip):

ah, right

nikomatsakis (Aug 31 2018 at 10:05, on Zulip):

I was thinking of this:

nikomatsakis (Aug 31 2018 at 10:05, on Zulip):

https://github.com/rust-lang/rust/blob/1114ab684fbad001c4e580326d8eb4d8c4e917d3/src/librustc_mir/borrow_check/mod.rs#L730-L734

nikomatsakis (Aug 31 2018 at 10:05, on Zulip):

however — presently — this is not part of a Place, you are correct, instead, that is used as part of specifying what you access

nikomatsakis (Aug 31 2018 at 10:06, on Zulip):

hmm, hmm, hmm

nikomatsakis (Aug 31 2018 at 10:07, on Zulip):

I mean borrowing the "thing that contains the discriminant" would be a suitable overapproximation :P

Matthew Jasper (Aug 31 2018 at 20:01, on Zulip):

Borrowing the thing that contains the discriminant is too much of an over-approximation. There are crates that match on an enum while one of its fields is borrowed, which conflicts with a borrow of the enum.

Jake Goulding (Aug 31 2018 at 20:03, on Zulip):

match on an enum while one of its fields is borrowed

but, if they've borrowed a field, the variant has to have already been decided; why would they match again?

Matthew Jasper (Aug 31 2018 at 20:05, on Zulip):

They could have returned, in which case the borrow will last until the end of the function until we have Polonius.

Matthew Jasper (Aug 31 2018 at 20:06, on Zulip):

They could also have only borrowed in one case in an if expression.

nikomatsakis (Aug 31 2018 at 20:07, on Zulip):

Borrowing the thing that contains the discriminant is too much of an over-approximation. There are crates that match on an enum while one of its fields is borrowed, which conflicts with a borrow of the enum.

are we seeing these regressions in crater?

nikomatsakis (Aug 31 2018 at 20:08, on Zulip):

all of this, incidentally, further argues for my desire to separate out the borrow checker's notion of "place" from MIR's notion of place

Matthew Jasper (Aug 31 2018 at 20:09, on Zulip):

I think we were, I held off on this PR until I was pretty sure that this was leading to actual regressions.

nikomatsakis (Aug 31 2018 at 20:09, on Zulip):

hmm I hadn't seen those?

Matthew Jasper (Aug 31 2018 at 20:13, on Zulip):

The only one I can find ATM is https://cargobomb-reports.s3.amazonaws.com/nll-3/00bcc44fb92c28465c727881355c3235a56a4045/reg/rgen3-save-0.1.0/log.txt. Does https://github.com/nikomatsakis/nll-crater-run-results contain everything, because I thought that there were more?

nikomatsakis (Aug 31 2018 at 20:15, on Zulip):

cc @lqd

nikomatsakis (Aug 31 2018 at 20:15, on Zulip):

that said, we might have erroneously considered that legit

nikomatsakis (Aug 31 2018 at 20:16, on Zulip):

it's not obvious from the quoted error that it's not

nikomatsakis (Aug 31 2018 at 20:16, on Zulip):

I guess we have to go to the original sources to tell

lqd (Aug 31 2018 at 20:17, on Zulip):

some are not obvious yeah. there might be some in the reports that are not in the repo, there's a parser combinator library I hadn't time to minimize yet as well

nikomatsakis (Aug 31 2018 at 20:19, on Zulip):

that looks pretty legit to me

lqd (Aug 31 2018 at 20:19, on Zulip):

I think I have minimized rgen3 (it's the pokémon one IIRC), it looked legit but I might have done it incorrectly ?

nikomatsakis (Aug 31 2018 at 20:19, on Zulip):

relevant function

    pub fn sections(&mut self) -> SaveSections {
        let block = &mut self.blocks[self.most_recent_index];
        let team_and_items = if let SectionData::TeamAndItems(ref mut data) =
                                    block.sections[block.team_and_items_index].data {
            data
        } else {
            panic!("Unexpected section data. Expected TeamAndItems");
        };
        let trainer_info = if let SectionData::TrainerInfo(ref mut data) =
                                  block.sections[block.trainer_info_index].data {
            data
        } else {
            panic!("Unexpected section data. Expected TrainerInfo");
        };
        SaveSections {
            team: &mut team_and_items.team,
            trainer: trainer_info,
        }
    }
nikomatsakis (Aug 31 2018 at 20:20, on Zulip):

if I'm not mistaken, unless team_and_items_index != trainer_info_index, those two mutable borrows could overlap?

nikomatsakis (Aug 31 2018 at 20:21, on Zulip):

that said, @Matthew Jasper, I could imagine cases where the borrow would be stronger than we would want

nikomatsakis (Aug 31 2018 at 20:21, on Zulip):

I'd just be surprised to see them in practice

nikomatsakis (Aug 31 2018 at 20:21, on Zulip):

or maybe not, who knows, mostly I mean "I just don't remember seeing them" :)

nikomatsakis (Aug 31 2018 at 20:22, on Zulip):

@Matthew Jasper are you currently doing some sort of hacking on this?

Matthew Jasper (Aug 31 2018 at 20:23, on Zulip):

I have what you see in the PR and 20 minutes work on making discriminants Places.

nikomatsakis (Aug 31 2018 at 20:28, on Zulip):

oh, ok

nikomatsakis (Aug 31 2018 at 20:28, on Zulip):

making discrim places seems plausible for now, anyway

nikomatsakis (Aug 31 2018 at 20:28, on Zulip):

presumably it'll result in some bug! calls...

nikomatsakis (Aug 31 2018 at 20:29, on Zulip):

the challenge is that there are discriminants that can't be treated like other places

nikomatsakis (Aug 31 2018 at 20:29, on Zulip):

e.g., the null value of Option<&T> =)

nikomatsakis (Aug 31 2018 at 20:29, on Zulip):

this is why we have ReadDiscriminant() and not discriminant places

Matthew Jasper (Aug 31 2018 at 20:31, on Zulip):

Indeed, I was planing to remove borrows of discriminants in one of the after borrowck passes

Matthew Jasper (Aug 31 2018 at 20:32, on Zulip):

It's still not great, having MIR that makes no sense to run.

nikomatsakis (Aug 31 2018 at 20:34, on Zulip):

well

nikomatsakis (Aug 31 2018 at 20:34, on Zulip):

yeah

nikomatsakis (Aug 31 2018 at 20:34, on Zulip):

I mean that part itself doesn't bother me

nikomatsakis (Aug 31 2018 at 20:34, on Zulip):

I always (in my head) imagine a "lowering" step between the MIR we initially create

nikomatsakis (Aug 31 2018 at 20:34, on Zulip):

and the LIR that we execute (which just happens to share the same data structures as MIR)

nikomatsakis (Aug 31 2018 at 20:35, on Zulip):

but one might imagine wanting something like BorrowDiscriminant(...) etc

nikomatsakis (Aug 31 2018 at 20:35, on Zulip):

so that you can at least more easily screen out the "impossible cases"

nikomatsakis (Aug 31 2018 at 20:35, on Zulip):

One could even imagine producing a whole distinct IR for LIR

nikomatsakis (Aug 31 2018 at 20:35, on Zulip):

this is not entirely crazy

nikomatsakis (Aug 31 2018 at 20:35, on Zulip):

in particular, it might eliminate some clones

nikomatsakis (Aug 31 2018 at 20:36, on Zulip):

that is, I think you could create the MIR, modify it in place to have full regions etc, then "clone/simplify" it into LIR, which might be somewhat different (SSA, maybe, etc)

nikomatsakis (Aug 31 2018 at 20:36, on Zulip):

anyway

nikomatsakis (Aug 31 2018 at 20:36, on Zulip):

that's all way far away for this bug :)

nikomatsakis (Aug 31 2018 at 20:36, on Zulip):

just thinking out loud

Matthew Jasper (Aug 31 2018 at 20:37, on Zulip):

I considered BorrowDiscriminant(...), it's probably worse for borrowck and better for everyone else who uses MIR.

Matthew Jasper (Aug 31 2018 at 20:38, on Zulip):

And is probably stricter in some edge cases.

nikomatsakis (Aug 31 2018 at 20:39, on Zulip):

it's annoying that we now have borrows coming from something other than Ref expressions I guess

nikomatsakis (Aug 31 2018 at 20:39, on Zulip):

that is a pretty big loss

nikomatsakis (Aug 31 2018 at 20:40, on Zulip):

ok well

nikomatsakis (Aug 31 2018 at 20:40, on Zulip):

I guess one other thing

nikomatsakis (Aug 31 2018 at 20:40, on Zulip):

would be BorrowKind::Discriminant

nikomatsakis (Aug 31 2018 at 20:40, on Zulip):

which is...wacky

nikomatsakis (Aug 31 2018 at 20:40, on Zulip):

but the idea would be sort of that it's a shared borrow of "the discriminant"

nikomatsakis (Aug 31 2018 at 20:41, on Zulip):

I forget just how places_conflict works...

nikomatsakis (Aug 31 2018 at 20:41, on Zulip):

in particular if it has access to the borrow kind

nikomatsakis (Aug 31 2018 at 20:41, on Zulip):

probably not

Matthew Jasper (Aug 31 2018 at 20:42, on Zulip):

It would probably work, but would almost certainly cause anyone to be confused when they first see it.

Matthew Jasper (Aug 31 2018 at 20:43, on Zulip):

It doesn't. I would have BorrowKind::Discriminant do a Shallow(Discriminant) access but borrow the whole place, if I could get away with it.

Matthew Jasper (Aug 31 2018 at 20:45, on Zulip):

Otherwise just make discriminant a Place

nikomatsakis (Aug 31 2018 at 20:47, on Zulip):

It doesn't. I would have BorrowKind::Discriminant do a Shallow(Discriminant) access but borrow the whole place, if I could get away with it.

what precisely are you looking to "accept" here?

nikomatsakis (Aug 31 2018 at 20:51, on Zulip):

I guess you are thinking of a case where something is borrowed that is not affecting the discriminant?

nikomatsakis (Aug 31 2018 at 20:51, on Zulip):

it'd be good to come up with some specific example

nikomatsakis (Aug 31 2018 at 20:51, on Zulip):

I suspect that most of them also won't build with AST borrow check

Matthew Jasper (Aug 31 2018 at 20:51, on Zulip):

The main things are

Using normal shared borrows more carefully should get us two of these, so I'll start there.

nikomatsakis (Aug 31 2018 at 20:51, on Zulip):

in which case, maybe we can leave it for future work :)

Matthew Jasper (Aug 31 2018 at 20:57, on Zulip):

Well AST borrowck has a soundness bug around matching on borrowed places, which makes this even more fun.

Matthew Jasper (Aug 31 2018 at 21:20, on Zulip):

Some examples of what might or might not compile: https://play.rust-lang.org/?gist=3393dae9f7a74179a77af649ba50689d&version=nightly&mode=debug&edition=2015

nikomatsakis (Aug 31 2018 at 21:45, on Zulip):

my guess is that the final example might not compile, even if we removed E0382 (which we plan to, once NLL is merged)

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

because we've been trying to be agnostic about the order in which match arms are tested

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

that is sort of a special case in that there is probably no other possible way to evaluate that pattern

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):
    match s {
        1 => (),
        0 if { s = 1; false } => (),
        _ => (),
    }
nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

but something like that would I expect still be illegal

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

regardless, all would be illegal under the compilation model I proposed

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

in that s needs to be borrowed

nikomatsakis (Aug 31 2018 at 21:46, on Zulip):

because it is compared for equality

nikomatsakis (Aug 31 2018 at 21:47, on Zulip):

and it would remain borrowed until the match arm executes

Matthew Jasper (Aug 31 2018 at 22:04, on Zulip):

Should this compile then?

fn is_this_ok(mut s: i32) {
    match s {
        _ if { s = 1; false } => (),
        _ => (),
    }
}
nikomatsakis (Aug 31 2018 at 22:58, on Zulip):

@Matthew Jasper an interesting question

Matthew Jasper (Sep 02 2018 at 19:48, on Zulip):

Ok so we need some kind of "shallow borrow" to allow this to compile

fn foo(x: &mut (i32, Option<i32>)) {
     let r = &mut x.0;
     match x.1 { // has to prevent assigning to x in guards, but borrowing it will conflict with the borrow above
        // ...
    }
    r;
}
Matthew Jasper (Sep 02 2018 at 19:54, on Zulip):

I guess I'll close the current PR and try implementing discriminant Places. Unless you think that now is the time to try using a different version of Place in borrowck.

Matthew Jasper (Sep 02 2018 at 20:18, on Zulip):

Maybe BorrowKind::Shallowand parsing the borrow kinds to places conflict is simpler.

Matthew Jasper (Sep 02 2018 at 21:39, on Zulip):

So, for reference, I'm proposing:

pnkfelix (Sep 03 2018 at 08:59, on Zulip):

Ok so we need some kind of "shallow borrow" to allow this to compile

fn foo(x: &mut (i32, Option<i32>)) {
     let r = &mut x.0;
     match x.1 { // has to prevent assigning to x in guards, but borrowing it will conflict with the borrow above
        // ...
    }
    r;
}

I have to admit, I'm confused: why does it not suffice here to just borrow x.1 alone?

pnkfelix (Sep 03 2018 at 09:00, on Zulip):

that is, your comment says "has to prevent assigning to x in guards", but borrowing x.1 should accomplish that, right? (and if the outer borrow &mut x.0 were not present, then presumably the guards then would be allowed to assign to x.0, in an ideal world at least?)

Matthew Jasper (Sep 03 2018 at 11:39, on Zulip):

Assignments only do shallow writes, so borrowing (*x).0 or *x doesn't prevent assigning to x.

pnkfelix (Sep 03 2018 at 13:10, on Zulip):

oh right, I overlooked that x is a reference to a tuple, not a tuple itself.

pnkfelix (Sep 03 2018 at 13:11, on Zulip):

(though then I'm not 100% sure why it should be illegal to assign to x ... its not like doing so would invalidate r, right...? I must not be thinking hard enough about this.)

pnkfelix (Sep 03 2018 at 13:12, on Zulip):

oh, the match itself... hmm.

pnkfelix (Sep 03 2018 at 13:12, on Zulip):

(but the match input is derefed...)

pnkfelix (Sep 03 2018 at 13:12, on Zulip):

I guess I'll just stop talking for a while

Matthew Jasper (Sep 03 2018 at 18:44, on Zulip):

See the tests in this commit for why we borrow x as well as *x https://github.com/rust-lang/rust/commit/cb5c989598178af505fb215dd97afca8cc2b659f#diff-a2126cd3263a1f5342e2ecd5e699fbc6

Matthew Jasper (Sep 03 2018 at 18:45, on Zulip):

Replace & with &mut in the second test to get something closer to the example here.

nikomatsakis (Sep 05 2018 at 13:27, on Zulip):

(though then I'm not 100% sure why it should be illegal to assign to x ... its not like doing so would invalidate r, right...? I must not be thinking hard enough about this.)

so @pnkfelix this is -- to some extent -- an artifact of how MIR desugars

nikomatsakis (Sep 05 2018 at 13:28, on Zulip):

that is, when matching on a place expression (like (*x).1) we will use that same (*x).1 place expression to load the option etc

nikomatsakis (Sep 05 2018 at 13:28, on Zulip):

so if it changes 'mid match', that is a problem

nikomatsakis (Sep 05 2018 at 13:29, on Zulip):

aside: I feel like we are overdue to update and complete the NLL RFC

nikomatsakis (Sep 05 2018 at 13:29, on Zulip):

So, for reference, I'm proposing:

let me try to compare this...

Matthew Jasper (Sep 05 2018 at 21:07, on Zulip):

I'm tempted to add "or bind to a variable" here as well. i.e. this should not compile

fn main()
    let mut x: i32 = 1;
    let mut w = 10;
    match x {
        z if { w = z; x = 0; false } => (),
        y => { assert_eq!(y, w) },
    }
}
nikomatsakis (Sep 05 2018 at 21:15, on Zulip):

sorry @Matthew Jasper, didn't get to this

nikomatsakis (Sep 05 2018 at 21:15, on Zulip):

argh

nikomatsakis (Sep 05 2018 at 21:15, on Zulip):

not sure where the days keep going!

pnkfelix (Sep 07 2018 at 10:48, on Zulip):

(though then I'm not 100% sure why it should be illegal to assign to x ... its not like doing so would invalidate r, right...? I must not be thinking hard enough about this.)

I cannot believe that I forgot that the match input is implicitly borrowed (and then we read from that borrow in the ReadForMatch MIR statements), especially since I'm the one who implemented that PR!

pnkfelix (Sep 07 2018 at 10:49, on Zulip):

just goes to show how badly sleep deprived I have been. :sleepy: :sad:

Matthew Jasper (Sep 07 2018 at 18:52, on Zulip):

@nikomatsakis ping

nikomatsakis (Sep 07 2018 at 19:27, on Zulip):

@Matthew Jasper pong

Matthew Jasper (Sep 07 2018 at 19:34, on Zulip):

Just a reminder that this exists.

nikomatsakis (Sep 10 2018 at 18:04, on Zulip):

@Matthew Jasper

Reading your big comment:

When we check for conflicts with them, if the access place is a strict prefix of the borrow place then we don't consider that a conflict.

I think you have this reversed, maybe? That is, I think you mean "if the borrow place is a strict prefix of the access place". That at least fits your example: "a Shallow borrow of x does not conflict with any access or borrow of x.0 or *x"

When building matches we take a Shallow borrow of any Place that we switch on, and any pointer that's dereferenced in that Place.

This sounds good, though I think I would say "we take a shallow borrow on all prefixes of any place that we switch on".

I'm not sure about "all pointer dereferences". For example, if you have a pattern like &_, do we count that? I sort of think not, but it's unclear. It interacts somewhat with unsafe code guidelines, actually, in that this might be used to imply that the pointer has a valid type (but in that scenario one would normally have a pattern more like &! or something).

we should have enough fake edges to only need to do this on the last arm

Hmm. Perhaps true but it makes me a bit nervous to only do the last arm. =) In part this is for the future: e.g., if we start to remove fake borrows, I don't want us to forget that we now have to add reads.

Change ReadForMatch to only check for initializedness

Hmm. So I think eventually we should adopt the ! pattern proposal that I blogged about the other day, which would address this. In particular, you would have match x { ! }, and the ! would be considered a "switch", so we would get a "fake borrow", which would in turn prevent x from being uninitialized.

This is probably "close enough" to what you are doing here, at least for now.

I'm trying to think if there are other examples to be concerned about ...

nikomatsakis (Sep 10 2018 at 18:04, on Zulip):

Sorry for taking so long

Matthew Jasper (Sep 10 2018 at 18:46, on Zulip):

@nikomatsakis

I think you have this reversed, maybe? That is, I think you mean "if the borrow place is a strict prefix of the access place". That at least fits your example: "a Shallow borrow of x does not conflict with any access or borrow of x.0 or *x"

Yes. :face palm:.

I'm not sure about "all pointer dereferences". For example, if you have a pattern like &_, do we count that?

No, we aren't switching or binding on any place behind the reference.

Hmm. So I think eventually we should adopt the ! pattern proposal that I blogged about the other day

Indeed, but we need something before then.

nikomatsakis (Sep 10 2018 at 18:47, on Zulip):

I just want to be sure we are doing something "sort of compatible" with that

nikomatsakis (Sep 10 2018 at 18:47, on Zulip):

I .. think we are :P

nikomatsakis (Sep 10 2018 at 18:47, on Zulip):

No, we aren't switching or binding on any place behind the reference.

nikomatsakis (Sep 10 2018 at 18:48, on Zulip):

it seems like we can define a set of things that must not change (e.g., those we are "switching" on) and then borrow all the prefixes

nikomatsakis (Sep 10 2018 at 18:48, on Zulip):

basically that Place must continue to evaluate to the same value throughout the match

nikomatsakis (Sep 10 2018 at 18:49, on Zulip):

I am happy, I think, modeling that as many small borrows — though you could imagine modeling it as a distinct kind of borrow

nikomatsakis (Sep 10 2018 at 18:49, on Zulip):

one for which places_conflict might act differently

nikomatsakis (Sep 10 2018 at 18:49, on Zulip):

(in fact, it's just "prefix of"?)

nikomatsakis (Sep 10 2018 at 18:50, on Zulip):

I have to look at that

nikomatsakis (Sep 10 2018 at 18:50, on Zulip):

refresh my memory on that code I mean

nikomatsakis (Sep 10 2018 at 18:50, on Zulip):

it seems like each_borrow_involving_path would be the one, probably, that checks the borrow kind

nikomatsakis (Sep 10 2018 at 18:51, on Zulip):

but basically traditional borrows ensure that the memory we reached by evaluating the place doesn't change, but here we want to ensure that we can continue to evaluate the place... an interesting distinction

Matthew Jasper (Sep 10 2018 at 18:57, on Zulip):

(in fact, it's just "prefix of"?)

It depends on how we want to handle unions.

nikomatsakis (Sep 10 2018 at 18:58, on Zulip):

yeah, right. I think we want to handle them as errors

nikomatsakis (Sep 10 2018 at 18:58, on Zulip):

(that is, the way that you would get from the many shallow borrows)

nikomatsakis (Sep 10 2018 at 18:58, on Zulip):

ps

nikomatsakis (Sep 10 2018 at 18:58, on Zulip):

I think the right way to think of it, still, is as a "discriminant borrow"

nikomatsakis (Sep 10 2018 at 18:59, on Zulip):

oh, well, hmm

nikomatsakis (Sep 10 2018 at 18:59, on Zulip):

I guess the point is that — for the prefixes — that doesn't work

nikomatsakis (Sep 10 2018 at 18:59, on Zulip):

okok

Matthew Jasper (Sep 10 2018 at 18:59, on Zulip):

Maybe, but it's a bit strange to call it the discriminant with integers.

nikomatsakis (Sep 10 2018 at 18:59, on Zulip):

"shallow" is a funny term

nikomatsakis (Sep 10 2018 at 18:59, on Zulip):

but I can't think of a better one

Matthew Jasper (Sep 10 2018 at 19:00, on Zulip):

Anyway I'll implement a hopefully conservative variant of this and I guess we can see where we stand.

nikomatsakis (Sep 10 2018 at 19:00, on Zulip):

in particular, the fact that (e.g.) a shallow borrow of a tuple means:

just surprises me a bit.

nikomatsakis (Sep 10 2018 at 19:00, on Zulip):

yes, I'm basically :+1: on the idea

Matthew Jasper (Sep 10 2018 at 19:03, on Zulip):

in particular, the fact that (e.g.) a shallow borrow of a tuple means:

just surprises me a bit.

True, but we won't ever create a shallow borrow of a tuple but none of its fields.

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

it's more that I affect "shallow" to mean "not through a pointer deref"

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

but that's not quite what this means

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

anyway, it's fine

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

maybe we find another name

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

regardless, it does feel like we need a new concept

nikomatsakis (Sep 10 2018 at 19:18, on Zulip):

that doesn't already exist

Matthew Jasper (Sep 12 2018 at 21:00, on Zulip):

Current state:
https://gist.github.com/matthewjasper/20706927286a2f81768dc67a175b15d2

nikomatsakis (Sep 12 2018 at 21:05, on Zulip):

reading

nikomatsakis (Sep 12 2018 at 21:05, on Zulip):

nice!

nikomatsakis (Sep 12 2018 at 21:05, on Zulip):

what is v here?

nikomatsakis (Sep 12 2018 at 21:06, on Zulip):

also, maybe add a variant of fn bad_indirect_mutation_in_guard that uses the new patterns

Matthew Jasper (Sep 12 2018 at 21:06, on Zulip):

s, but the neither me nor the compile read comments. :P

nikomatsakis (Sep 12 2018 at 21:06, on Zulip):

e.g.,

fn bad_indirect_mutation_in_guard(mut v: &bool) {
    match v {
        true => (),
        false if {
            v = &true; //~ ERROR
            false
        } => (),
        false => (),
    }
}
Matthew Jasper (Sep 15 2018 at 10:44, on Zulip):

Pushed

Matthew Jasper (Sep 17 2018 at 19:09, on Zulip):

Ok, so the problem is that the fake borrows are sometimes dereferencing pointers in inactive variants (or something like that)

Matthew Jasper (Sep 17 2018 at 19:09, on Zulip):

This will segfault:

fn foo(x: Option<&&i32>) -> i32 {
    match x {
        Some(0) if true => 0,
        _ => 1,
    }
}

fn main() {
    foo(None);
}
Matthew Jasper (Sep 17 2018 at 19:11, on Zulip):

So I'm planning to create a MIR transform to remove them. But I'm wondering why there isn't already a pass to remove ReadForMatches?

nikomatsakis (Sep 17 2018 at 20:15, on Zulip):

So I'm planning to create a MIR transform to remove them. But I'm wondering why there isn't already a pass to remove ReadForMatches?

I suspect there is -- but probably not to remove the borrows?

Matthew Jasper (Sep 17 2018 at 20:23, on Zulip):

They seem to survive up to codegen judging by emit mir output. Not removing the borrows is expected though.

nikomatsakis (Sep 17 2018 at 20:55, on Zulip):

They may just be made a no-op at codegen time I guess

pnkfelix (Sep 24 2018 at 11:49, on Zulip):

@Matthew Jasper what can a ShallowBorrowactually inspect on a place, since it will not conflict with any projection into that place? Is it solely for the discriminant? Or is it also for the array-length? Is it for all metadata (like discriminant and array-length) that is not covered by the set of things that can be borrowed via MIR-projections?

Matthew Jasper (Sep 24 2018 at 11:54, on Zulip):

The list is something like discriminants, array lengths the value of a numeric type (or char or bool), and the value of a reference, but not what it points to.

pnkfelix (Sep 24 2018 at 11:55, on Zulip):

oh interesting, I hadn't consider the pointer value of the reference

pnkfelix (Sep 24 2018 at 11:57, on Zulip):

If you get a chance, it might be nice to add that list of concrete examples; the current comment leads one to see the discriminant as one example, but seeing the others as you listed here, even if you don't intend for it to be an exhaustive enumeration of the possibilities, is useful.

pnkfelix (Sep 24 2018 at 11:57, on Zulip):

and my description of it being for "metadata" would be misleading, since you've pointed out there are things like numeric values that are not metadata, but as also not projections.

Last update: Nov 21 2019 at 14:15UTC