Stream: t-compiler/wg-nll

Topic: #55288 cast fails to promote to 'static


davidtwco (Oct 23 2018 at 21:13, on Zulip):

FYI, this test case no longer ICEs on the latest master - it still produces an incorrect error though.

nikomatsakis (Oct 23 2018 at 21:17, on Zulip):

progess :P

davidtwco (Oct 23 2018 at 22:16, on Zulip):

Do you have any plans for how you'd like this tackled? It seems like the cause is the constraint to static created as a result of the AscribeUserTy statement.

nikomatsakis (Oct 24 2018 at 00:09, on Zulip):

@davidtwco i think we need to modify the promotion code to permit promotion even when there is a AscribeUserTy statement; @Oli or @RalfJ may have more insights, or I can take a closer look tomorrow

oli (Oct 24 2018 at 07:57, on Zulip):

Most likely, in qualify_const.rs one needs to prevent the visitor from recursing into AscribeUserTy (I'm not sure where though), because otherwise the const qualifier will see the place that is ascribed twice and assume that there are two accesses and thus the value cannot be promoted

davidtwco (Oct 24 2018 at 08:15, on Zulip):

Unless I'm being naive and this isn't the only place where that might happen, AscribeUserTy is only visited once?

oli (Oct 24 2018 at 08:59, on Zulip):

That's what I thought, too, but I haven't been able to figure out what else could have caused this. Did the MIR around the AscribeUserTy change? Or was the statement just added additionally without any other MIR changes?

davidtwco (Oct 24 2018 at 09:03, on Zulip):

As I understand it, the AscribeUserTy statement was added for this case to support user type annotations in cast statements - but in this particular instance, that new statement is resulting in a constraint that the lifetime must be 'static. I think what is happening is that the error caused by that new constraint is happening before the promotion to 'static.

davidtwco (Oct 24 2018 at 09:04, on Zulip):

As before, without this constraint, there wouldn't be an error generated by that constraint not being met and then it will get to be promoted and everything ends up fine.

davidtwco (Oct 24 2018 at 09:04, on Zulip):

That's my working theory, at least.

oli (Oct 24 2018 at 09:25, on Zulip):

I think you're on the right track

oli (Oct 24 2018 at 09:25, on Zulip):

the borrow checker runs after promotion

oli (Oct 24 2018 at 09:26, on Zulip):

the value that AscribeUserTy refers to was promoted successfully

oli (Oct 24 2018 at 09:26, on Zulip):

the problem is that AscribeUserTy still points to the unpromoted value

davidtwco (Oct 24 2018 at 09:27, on Zulip):

Ah, so I should try update that during promotion?

davidtwco (Oct 24 2018 at 09:27, on Zulip):

Or is that not a thing that can be done?

oli (Oct 24 2018 at 09:27, on Zulip):

jup, and it can be done

oli (Oct 24 2018 at 09:27, on Zulip):

but not in qualify_const.rs

oli (Oct 24 2018 at 09:27, on Zulip):

you'll need to do it in promote_consts.rs

oli (Oct 24 2018 at 09:28, on Zulip):

that's where the actual promotion is happening

davidtwco (Oct 24 2018 at 09:28, on Zulip):

Alright. I'll see what I can come up with. Thanks.

nikomatsakis (Oct 24 2018 at 14:58, on Zulip):

interesting

davidtwco (Oct 24 2018 at 21:49, on Zulip):

So, having just had a chance to look at this a little, in both cases (the working case where I remove the cast and the not-working case where I don't remove the cast), I only see rustc_mir::transform::promote_consts: promote_candidates([]) - as if there isn't any promotion happening, in either case?

This is the MIR without the cast.
This is the MIR with the cast.

davidtwco (Oct 24 2018 at 21:55, on Zulip):

Hoping I'll have some more time tomorrow night to properly dig in and see if I can't work anything out.

davidtwco (Oct 24 2018 at 21:58, on Zulip):

I think that the partial log statement I sent above comes from the empty main function - since const promotion is limited to either functions or const functions. It is not coming from the static map.

davidtwco (Oct 24 2018 at 21:59, on Zulip):

(I've double checked this, the mode variable that decides which branch we go into in qualify_consts.rs is Mode::Static which doesn't end up calling promote_consts::promote_candidates)

davidtwco (Oct 24 2018 at 22:00, on Zulip):

There's either somewhere else in qualify_consts that makes the working case work; or it's something else entirely. I'm not sure.

Matthew Jasper (Oct 24 2018 at 22:08, on Zulip):

This looks suspicious

_3 = &_4;                        // bb0[16]: scope 0 at src/test/ui/nll/issue-55288.rs:17:27: 19:2
// ...
StorageDead(_4); // bb0[21]: scope 0 at src/test/ui/nll/issue-55288.rs:19:2: 19:3
nikomatsakis (Oct 25 2018 at 13:48, on Zulip):

Hmm, yes, curious. I agree with @Matthew Jasper that the immediate cause of the error is that we have a StorageDead

davidtwco (Oct 25 2018 at 13:48, on Zulip):

Why is that StorageDead strange?

nikomatsakis (Oct 25 2018 at 13:48, on Zulip):

of course the next question is why

nikomatsakis (Oct 25 2018 at 13:48, on Zulip):

IIRC, in statics/constants, we emit StorageDead only for temporaries that doesn't escape into static scope — so why does it consider this a temporary etc in one case but not the other?

nikomatsakis (Oct 25 2018 at 13:49, on Zulip):

well, it's not whether it's strange or not, but it causes the error for sure— we are reporting the error because we see a StorageDead and we have an outstanding borrow

davidtwco (Oct 25 2018 at 13:49, on Zulip):

Ah, I didn't know that we used StorageDead differently in statics/consts.

nikomatsakis (Oct 25 2018 at 13:54, on Zulip):

@davidtwco are you familiar with the temporary lifetime rules?

nikomatsakis (Oct 25 2018 at 13:54, on Zulip):

e.g., let x = foo(&something()) -- in this, the return value of something() is saved into a temporary that is freed before the next statement

nikomatsakis (Oct 25 2018 at 13:55, on Zulip):

let x = &something() -- but here we extend the lifetime till the end of the block

nikomatsakis (Oct 25 2018 at 13:55, on Zulip):

the idea for statics/consts is to apply the same rules to the value of the static -- if it would have been "extended till end of block", it is never freed, but otherwise, it is

nikomatsakis (Oct 25 2018 at 13:55, on Zulip):

in this case, I would expect the StorageDead

nikomatsakis (Oct 25 2018 at 13:55, on Zulip):

(but I would also have expected promotion to kick in, maybe it a bit of a moot point)

nikomatsakis (Oct 25 2018 at 13:56, on Zulip):

so actually the question is more why doesn't the StorageDead get emitted?

nikomatsakis (Oct 25 2018 at 13:56, on Zulip):

(in the case that compiles successfully)

davidtwco (Oct 25 2018 at 13:56, on Zulip):

That makes sense. Hadn't considered how those rules would apply to statics.

davidtwco (Oct 25 2018 at 13:56, on Zulip):

I can dig into that more when I'm next looking at this.

davidtwco (Oct 25 2018 at 20:48, on Zulip):

@nikomatsakis So, I've worked out what the issue is here.

davidtwco (Oct 25 2018 at 20:54, on Zulip):

In the below snippet of MIR, the use of _6 in the AscribeUserType statement is causing the const promotion (which is actually happening despite my theory otherwise because of the branch it was falling into) to not happen, as was the suspicion all along.

        _6 = move _7;                    // bb0[12]: scope 0 at src/test/ui/nll/issue-55288.rs:18:5: 18:35
        AscribeUserType(_6, o, Ty(Canonical { variables: [], value: &'static [u8] })); // bb0[13]: scope 0 at src/test/ui/nll/issue-55288.rs:18:5: 18:35
       _5 = _6; // bb0[14]: scope 0 at src/test/ui/nll/issue-55288.rs:18:5: 18:35

In particular, what was happening was that the TempState for _6 is being marked as Unpromotable and then that meant it got flagged as Qualif::NOT_PROMOTABLE so didn't become a promotion candidate and as a result, didn't get the StorageDead statements removed by the qualify_consts pass.

_6 is being marked as Unpromotable because the PlaceContext::Validate returns false from is_nonmutating_use. That made a difference in this code. Without the PlaceContext::Validate introduced by the user type assertions, visiting the local never resulted in a TempState::Unpromotable and therefore was always promoted and the relevant StorageDead statements removed. I guess the solution if we want to make this code work is to either special-case PlaceContext::Validate as true just like PlaceContext::Borrow here, or change the return value of is_nonmutating_use to true for that PlaceContext variant.

davidtwco (Oct 25 2018 at 20:54, on Zulip):

Or something else that you think of.

pnkfelix (Oct 25 2018 at 21:00, on Zulip):

I would imagine Validate would be a non-mutating use. But I don't know the const promotion code...

pnkfelix (Oct 25 2018 at 21:01, on Zulip):

but you say that this same code is also special-casing PlaceContext::Borrow ?

pnkfelix (Oct 25 2018 at 21:01, on Zulip):

ah I see, you linked it. Hmm.

davidtwco (Oct 25 2018 at 21:02, on Zulip):

IIRC there was a topic in #t-compiler like this a bit ago.

davidtwco (Oct 25 2018 at 21:04, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/subject/PlaceContext.20for.20AscribeUserType/near/136343874

davidtwco (Oct 25 2018 at 21:06, on Zulip):

It looks like this might fix itself if whatever change @RalfJ was landing changes the variant used by AscribeUserType in PlaceContext and that variant returns true from is_nonmutating_use.

davidtwco (Oct 25 2018 at 21:07, on Zulip):

It seems wrong to make any change if PlaceContext::Validate is intended for use by miri and not just AscribeUserType?

pnkfelix (Oct 25 2018 at 21:07, on Zulip):

yeah I agree. It sounds like I misunderstood what Validate is used for

pnkfelix (Oct 25 2018 at 21:07, on Zulip):

maybe coordinate with @RalfJ here. Find out where they are in their PR

pnkfelix (Oct 25 2018 at 21:07, on Zulip):

worst case scenario: Add a new PlaceContext yourself

davidtwco (Oct 25 2018 at 21:08, on Zulip):

Yeah, I think that makes sense. If they have a big PR that might not land for a while then I'll make something small that adds the new variant so we can get this fixed, otherwise I'll leave it.

pnkfelix (Oct 25 2018 at 21:08, on Zulip):

right. and your adding a new PlaceContext will force @RalfJ to deal with it upon rebase. Or vice versa, depending on who wins the race.

davidtwco (Oct 25 2018 at 21:08, on Zulip):

:thumbs_up:

nikomatsakis (Oct 25 2018 at 21:16, on Zulip):

(I agree with all this; adding a new PlaceContext is indeed what I expected the fix would be)

RalfJ (Oct 26 2018 at 06:57, on Zulip):

@davidtwco @pnkfelix @nikomatsakis My PR at https://github.com/rust-lang/rust/pull/55316 does add a PlaceContext variant for AscribeUserType

RalfJ (Oct 26 2018 at 06:57, on Zulip):

but I made it neither mutating nor nonmutating use because that's what @nikomatsakis told me to do ;)

davidtwco (Oct 26 2018 at 07:02, on Zulip):

Awesome. I guess this fixes itself then. Presumably the test case from the issue here doesn’t fail to compile on your branch?

davidtwco (Oct 26 2018 at 08:30, on Zulip):

In fact, re-reading your message, this still won't be fixed as the is_nonmutating_use is false (which is correct). We'd still need to special-case the new variant here to make this issue pass.

davidtwco (Oct 26 2018 at 08:33, on Zulip):

(that and adding a test for the example in the issue)

pnkfelix (Oct 26 2018 at 09:40, on Zulip):

I'm wondering why else branch in that code is context.is_nonmutating_use() and not !context.is_mutating_use()

pnkfelix (Oct 26 2018 at 09:41, on Zulip):

Is it because it increments *uses ?

davidtwco (Oct 26 2018 at 09:42, on Zulip):

I'd have assumed that context.is_nonmutating_use() was just !context.is_mutating_use().

pnkfelix (Oct 26 2018 at 09:42, on Zulip):

except its not, since @RalfJ made it "neither mutating nor nonmutating use"

davidtwco (Oct 26 2018 at 09:43, on Zulip):

Yeah, I realise that now. That's where it threw my expections.

pnkfelix (Oct 26 2018 at 09:43, on Zulip):

note in particular that there's an is_use() method that just does the OR of is_mutating_use() and is_nonmutating_use()

pnkfelix (Oct 26 2018 at 09:43, on Zulip):

right, gotcha

davidtwco (Oct 26 2018 at 09:43, on Zulip):

But that is why, if someone had the understanding I did, they might call is_nonmutating_use when they really meant !is_mutating_use.

pnkfelix (Oct 26 2018 at 09:43, on Zulip):

true

RalfJ (Oct 26 2018 at 09:44, on Zulip):

I dont understand what these classifiers are used for at all

RalfJ (Oct 26 2018 at 09:44, on Zulip):

Validate was for miri's validation statements though, so that was definitely wrong

pnkfelix (Oct 26 2018 at 09:44, on Zulip):

oh dear, I was hoping @RalfJ was our local expert. :)

RalfJ (Oct 26 2018 at 09:44, on Zulip):

this is NLL land. or borrowck. or so. I know nothing about any of that^^

RalfJ (Oct 26 2018 at 09:45, on Zulip):

I know a lot about CTFE/miri and that's pretty much the end of my rustc knowledge :)

pnkfelix (Oct 26 2018 at 09:45, on Zulip):

well its actually const promotion code

RalfJ (Oct 26 2018 at 09:45, on Zulip):

/me runs away screaming

davidtwco (Oct 26 2018 at 09:45, on Zulip):

I can understand why AscribeUserTy isn't a mutating use or a nonmutating use - since it isn't using the value, just doing a check.

pnkfelix (Oct 26 2018 at 09:45, on Zulip):

const promotion doesn't overlap with CTFE ?

pnkfelix (Oct 26 2018 at 09:45, on Zulip):

@RalfJ ^ ?

RalfJ (Oct 26 2018 at 09:45, on Zulip):

now that Validate is gone, there is Retag. that has a side-effect in miri, it mutates the content of the place, so I made it a mutating use....^^

RalfJ (Oct 26 2018 at 09:45, on Zulip):

@pnkfelix const promotion is a static analysis on MIR

RalfJ (Oct 26 2018 at 09:46, on Zulip):

it has no code overlap with CTFE

RalfJ (Oct 26 2018 at 09:46, on Zulip):

but a lot of overlap of concerns

pnkfelix (Oct 26 2018 at 09:46, on Zulip):

okay

davidtwco (Oct 26 2018 at 09:46, on Zulip):

I think the correct approach here is actually to change that line to !is_mutating_use.

RalfJ (Oct 26 2018 at 09:46, on Zulip):

also, it exists twice (once on HIR and once on MIR)

RalfJ (Oct 26 2018 at 09:46, on Zulip):

and both passes are a mess

davidtwco (Oct 26 2018 at 09:46, on Zulip):

But I don't know if that would break other things.

RalfJ (Oct 26 2018 at 09:46, on Zulip):

so, generally, it's just bad^^

pnkfelix (Oct 26 2018 at 09:46, on Zulip):

maybe that's where my confusion lies: I had assumed that since there is an overlap of concerns, it might make sense for the code to overlap. Like reuse common elements

RalfJ (Oct 26 2018 at 09:46, on Zulip):

but when it comes to const promotion I'd like to summon @Oli

RalfJ (Oct 26 2018 at 09:46, on Zulip):

they know that mess way better than I do^^

RalfJ (Oct 26 2018 at 09:47, on Zulip):

@pnkfelix const promotion runs on runtime MIR though

RalfJ (Oct 26 2018 at 09:47, on Zulip):

on non-monomorphized runtime mir

RalfJ (Oct 26 2018 at 09:47, on Zulip):

it has to statically determine if we want to promote this

RalfJ (Oct 26 2018 at 09:47, on Zulip):

that has nothing to do with what CTFE does, which is to actually execute fully monomorphized MIR

davidtwco (Oct 26 2018 at 09:48, on Zulip):

It seems like StorageDead, StorageLive and Validate (or whatever the new variant is called) all return false for either.

RalfJ (Oct 26 2018 at 09:48, on Zulip):

IOW, CTFE is "dynamic" -- it runs at compile-time, but it is an interpreter so it is dynamic the same way running a python program is dynamic. const promotion is static.

RalfJ (Oct 26 2018 at 09:48, on Zulip):

@davidtwco the new Validate is called Retag and I made it return true for is_mutating_use

RalfJ (Oct 26 2018 at 09:48, on Zulip):

because it mutates. in my model.

pnkfelix (Oct 26 2018 at 09:48, on Zulip):

I guessed I had figured that much of the expressions that we can promote are also things that we can CTFE ?

davidtwco (Oct 26 2018 at 09:49, on Zulip):

I meant the new variant for AscribeUserTy.

pnkfelix (Oct 26 2018 at 09:49, on Zulip):

(apart from stuff that actually references statically allocated state ...?)

RalfJ (Oct 26 2018 at 09:49, on Zulip):

I guessed I had figured that much of the expressions that we can promote are also things that we can CTFE ?

it must be the case that everything we promote, we can run in CTFE.

RalfJ (Oct 26 2018 at 09:49, on Zulip):

but its like a static analysis: running on the source code to see if later doing CTFE will work.

oli (Oct 26 2018 at 09:49, on Zulip):

I am not certain we need this static analysis on the MIR

davidtwco (Oct 26 2018 at 09:49, on Zulip):

I think !is_mutating_use would work, since we exit early in that function for StorageDead and StorageLive before the check.

oli (Oct 26 2018 at 09:50, on Zulip):

I've been trying to formulate my thoughts (e.g. in https://github.com/rust-lang/rust/issues/53819), but I guess that's orthogonal

pnkfelix (Oct 26 2018 at 09:50, on Zulip):

@davidtwco if the only other case is the variant for AscribeUserType, maybe it would be clearer to just put that in as a special case...

pnkfelix (Oct 26 2018 at 09:50, on Zulip):

not obvious to me. Depends on what the supposed "specs" are for fn is_[non]mutating_use.

RalfJ (Oct 26 2018 at 09:50, on Zulip):

what is the trouble with making AscribeUserType a nonmutating use?

RalfJ (Oct 26 2018 at 09:50, on Zulip):

then const promotion should be fine with it

davidtwco (Oct 26 2018 at 09:51, on Zulip):

It depends what the intent of that code was - was it to check for a use that doesn't mutate, or a nonmutating use - since those seem to be different.

pnkfelix (Oct 26 2018 at 09:51, on Zulip):

I was assuming the problem is that AscribeUserTy doesn't correspond to an actual use ?

pnkfelix (Oct 26 2018 at 09:51, on Zulip):

just a constraint on the type of the local place

oli (Oct 26 2018 at 09:51, on Zulip):

yes that

RalfJ (Oct 26 2018 at 09:51, on Zulip):

@davidtwco I dont think they are different. just some of them arent uses at all

davidtwco (Oct 26 2018 at 09:51, on Zulip):

Yeah, it isn't a use at all.

RalfJ (Oct 26 2018 at 09:51, on Zulip):

i.e., StorageDead/StorageLive

RalfJ (Oct 26 2018 at 09:52, on Zulip):

but AscribeUserTy is a use, right?

RalfJ (Oct 26 2018 at 09:52, on Zulip):

it takes a value and returns the same value, or so

RalfJ (Oct 26 2018 at 09:52, on Zulip):

changing its type

RalfJ (Oct 26 2018 at 09:52, on Zulip):

no?

oli (Oct 26 2018 at 09:52, on Zulip):

No, it doesn't do anything

RalfJ (Oct 26 2018 at 09:52, on Zulip):

oh

oli (Oct 26 2018 at 09:52, on Zulip):

it's just a marker for nll

davidtwco (Oct 26 2018 at 09:52, on Zulip):

Does it make sense to back out early in this conditional then?

RalfJ (Oct 26 2018 at 09:52, on Zulip):

maybe that conditional should then be !is_use()

pnkfelix (Oct 26 2018 at 09:52, on Zulip):

backing out early might be the right call.

RalfJ (Oct 26 2018 at 09:52, on Zulip):

and AscribeUserTy should return false for both (mutating and nonmutating)

oli (Oct 26 2018 at 09:53, on Zulip):

what does is_storage_marker mean?

oli (Oct 26 2018 at 09:53, on Zulip):

sounds about right?

davidtwco (Oct 26 2018 at 09:53, on Zulip):

It's defined as context == PlaceContext::StorageDead || context == PlaceContext::StorageLive

pnkfelix (Oct 26 2018 at 09:53, on Zulip):

/me is going to let you three hash this out. :)

oli (Oct 26 2018 at 09:54, on Zulip):

the other use is https://github.com/rust-lang/rust/blob/441519536c8bd138e8c651743249acd6814747a1/src/librustc_mir/transform/copy_prop.rs#L257

oli (Oct 26 2018 at 09:54, on Zulip):

so I say yes, most definitely is AscribeUserTy a storage marker

RalfJ (Oct 26 2018 at 09:55, on Zulip):

not sure if that sounds right

oli (Oct 26 2018 at 09:55, on Zulip):

maybe redefine this function as an exhaustive match, so future changes to PlaceContext will hit it

davidtwco (Oct 26 2018 at 09:55, on Zulip):

Would making it a storage marker result in the pass you just linked removing the statements before they can be checked by NLL?

RalfJ (Oct 26 2018 at 09:55, on Zulip):

storage_marker sounds very specific to StorageDead/Live

RalfJ (Oct 26 2018 at 09:55, on Zulip):

but maybe rename it to is_marker?

oli (Oct 26 2018 at 09:56, on Zulip):

is_local_marker then

RalfJ (Oct 26 2018 at 09:56, on Zulip):

and then declare that every PlaceContext must be exactly one of mutating use, nonmutating use, marker

oli (Oct 26 2018 at 09:56, on Zulip):

I assumed strorage to mean Local

oli (Oct 26 2018 at 09:56, on Zulip):

@davidtwco no, the copy prop pass runs after nll

davidtwco (Oct 26 2018 at 09:56, on Zulip):

I'd add a new function that is is_use - returning false for AscribeUserTy, StorageDead and StorageLive. I'd change is_nonmutating_use and is_mutating_use to return false straight away as a match arm if that the new is_use returns true. That makes it much clearer why those variants return false for both.

RalfJ (Oct 26 2018 at 09:56, on Zulip):

@Oli AscribeUserTy works on any Place though

oli (Oct 26 2018 at 09:56, on Zulip):

oh

oli (Oct 26 2018 at 09:57, on Zulip):

well, then is_use sounds great

davidtwco (Oct 26 2018 at 09:57, on Zulip):

And then change the condition to be !is_use() || is_nonmutating_use() here.

oli (Oct 26 2018 at 09:57, on Zulip):

at which point we might change is_nonmutating_use to an Option<bool>?

RalfJ (Oct 26 2018 at 09:57, on Zulip):

@davidtwco the problem with that is that the non-use variants still have to be repeated in nonmutating use

RalfJ (Oct 26 2018 at 09:58, on Zulip):

I'd rather we classify PlaceContext in three disjoint classes, than in a hierarchy with some excluded

RalfJ (Oct 26 2018 at 09:58, on Zulip):

three disjoint classes covering everything seem clearer to me

davidtwco (Oct 26 2018 at 09:58, on Zulip):

That's true...

oli (Oct 26 2018 at 09:58, on Zulip):

So... split the enum into 4?

RalfJ (Oct 26 2018 at 09:58, on Zulip):

the classes being: mutating use, nonmutating use, marker

RalfJ (Oct 26 2018 at 09:58, on Zulip):

into 4?

pnkfelix (Oct 26 2018 at 09:58, on Zulip):

you can leave it as one enum

pnkfelix (Oct 26 2018 at 09:58, on Zulip):

just make a new 4 K variant enum

RalfJ (Oct 26 2018 at 09:58, on Zulip):

but why 4?

pnkfelix (Oct 26 2018 at 09:58, on Zulip):

and a method that maps from the first to the second

RalfJ (Oct 26 2018 at 09:58, on Zulip):

we have 3 classes^^

davidtwco (Oct 26 2018 at 09:59, on Zulip):

We could have three is_x functions with the invariant that any given variant can only return true for one of them.

oli (Oct 26 2018 at 09:59, on Zulip):

If we split the enum we don't need the methods anymore

RalfJ (Oct 26 2018 at 09:59, on Zulip):

@davidtwco that was my thinking but having this encoded in the types would be even nicer

davidtwco (Oct 26 2018 at 09:59, on Zulip):

Yeah, definitely.

oli (Oct 26 2018 at 09:59, on Zulip):

yea, base enum with 3 variants + one enum for each variant

RalfJ (Oct 26 2018 at 09:59, on Zulip):

enum PlaceContextKind { Mutating, Nonmutating, Marker }

RalfJ (Oct 26 2018 at 09:59, on Zulip):

fn classify(PlaceContext) -> PlaceContextKind

pnkfelix (Oct 26 2018 at 10:00, on Zulip):

s/Marker/StaticEffect/ ?

RalfJ (Oct 26 2018 at 10:00, on Zulip):

or what @Oli said. no strong preference, just it seems overkill to have 4 enums here

pnkfelix (Oct 26 2018 at 10:00, on Zulip):

(just a thought. I don't find "Marker" intuitive)

oli (Oct 26 2018 at 10:00, on Zulip):

enum PlaceContext { Mutating(MutPlaceUse), Nonmutating(NonMutPlaceUse), Marker(PlaceNonUse) }

RalfJ (Oct 26 2018 at 10:00, on Zulip):

@pnkfelix I'm happy to bikeshed the name^^

davidtwco (Oct 26 2018 at 10:01, on Zulip):

That would work. Then we'd change the condition here to be context_kind == PlaceContextKind::NonMutatingUse || context_kind == PlaceContextKind::NoUse and this test would pass.

davidtwco (Oct 26 2018 at 10:01, on Zulip):

Or do we handle the NoUse case by exiting early.

davidtwco (Oct 26 2018 at 10:01, on Zulip):

That'd probably be better.

oli (Oct 26 2018 at 10:02, on Zulip):

I still think it's more fragile if we don't do it in the type

oli (Oct 26 2018 at 10:02, on Zulip):

but I'm not blocking it on that

pnkfelix (Oct 26 2018 at 10:02, on Zulip):

there's a reasonable argument for doing it in the type

pnkfelix (Oct 26 2018 at 10:02, on Zulip):

it can be nice to narrow to the specific subset of variants relevant to a given branch

pnkfelix (Oct 26 2018 at 10:03, on Zulip):

but are there instances where you'd want to handle two of the three cases in one branch?

davidtwco (Oct 26 2018 at 10:03, on Zulip):

Is this change going to be made in @RalfJ's current PR that adds the AscribeUserTy variant or should I branch off that and make this change to fix this issue?

RalfJ (Oct 26 2018 at 10:06, on Zulip):

That would work. Then we'd change the condition here to be context_kind == PlaceContextKind::NonMutatingUse || context_kind == PlaceContextKind::NoUse and this test would pass.

we can still have the is_ methods

RalfJ (Oct 26 2018 at 10:06, on Zulip):

just they would internally use classify

RalfJ (Oct 26 2018 at 10:07, on Zulip):

@davidtwco I'd say do one of the two variants we discussed here (@Oli's or mine), ignore my PR

RalfJ (Oct 26 2018 at 10:07, on Zulip):

whoever lands last has to rebase^^

davidtwco (Oct 26 2018 at 10:07, on Zulip):

Easy to say when you've got the open PR that'll be in the queue earlier, but sure thing :stuck_out_tongue:

RalfJ (Oct 26 2018 at 10:08, on Zulip):

@davidtwco well my PR depends on another PR that didnt land yet though

RalfJ (Oct 26 2018 at 10:08, on Zulip):

so I have two PRs that need to land. both are earlier in the queue but the race isnt entirely settled^^

RalfJ (Oct 26 2018 at 10:08, on Zulip):

if you want you can do it on top of my PR

RalfJ (Oct 26 2018 at 10:08, on Zulip):

I just thought we'd not want to tie these things together

davidtwco (Oct 26 2018 at 10:09, on Zulip):

Sure thing, works for me.

davidtwco (Oct 26 2018 at 11:29, on Zulip):

Alright, submitted #55385, let the race begin.

davidtwco (Oct 26 2018 at 13:14, on Zulip):

Responded to review comments.

nikomatsakis (Oct 26 2018 at 17:52, on Zulip):

glad you all sorted this out :)

RalfJ (Oct 28 2018 at 10:13, on Zulip):

@davidtwco that's unfair, you got priority :P

davidtwco (Oct 28 2018 at 10:14, on Zulip):

Might have cheated a little bit, yeah.

Last update: Nov 22 2019 at 00:45UTC