Stream: t-compiler

Topic: #58161 const patterns with ascribed patterns


Ariel Ben-Yehuda (Feb 06 2019 at 21:06, on Zulip):

hi @davidtwco

davidtwco (Feb 06 2019 at 21:06, on Zulip):

:wave:

Ariel Ben-Yehuda (Feb 06 2019 at 21:07, on Zulip):

so @Matthew Jasper had a comment on your PR

Ariel Ben-Yehuda (Feb 06 2019 at 21:07, on Zulip):

We still need them to check something like Type<'a>::X when we have impl Type<'static> { const X: i32 = 0; ... }

Ariel Ben-Yehuda (Feb 06 2019 at 21:08, on Zulip):

so you'll need to fix that

Ariel Ben-Yehuda (Feb 06 2019 at 21:08, on Zulip):

and also add a test for it

Ariel Ben-Yehuda (Feb 06 2019 at 21:09, on Zulip):

let me come up with one

davidtwco (Feb 06 2019 at 21:11, on Zulip):

I understood his comment as explaining why we would have a AscribeUserType pattern in this context, not that we'd need to check it here.

ie. I could see a scenario where your initial comment could be interpreted as a "should we even have "AscribeUserType patterns in this case?", and then he clarified it was used for checking well-formedness, and that was why we should have it - but not necessarily that we should be checking it here.

Perhaps I'm misunderstanding, but the checking he describes was #55511, fixed in #55937, which introduced this bug?

davidtwco (Feb 06 2019 at 21:12, on Zulip):

I may have misinterpreted it though.

Ariel Ben-Yehuda (Feb 06 2019 at 21:13, on Zulip):

so this function is not used in MIR typeck?

Ariel Ben-Yehuda (Feb 06 2019 at 21:13, on Zulip):

I think it is

davidtwco (Feb 06 2019 at 21:13, on Zulip):

This function is part of lowering to the MIR.

Ariel Ben-Yehuda (Feb 06 2019 at 21:14, on Zulip):

yea

Ariel Ben-Yehuda (Feb 06 2019 at 21:14, on Zulip):

so I think this can't actually be relevant

Ariel Ben-Yehuda (Feb 06 2019 at 21:14, on Zulip):

or is it?

Ariel Ben-Yehuda (Feb 06 2019 at 21:14, on Zulip):

the problem is that in theory a range can contain regions

Ariel Ben-Yehuda (Feb 06 2019 at 21:14, on Zulip):

if it is some sort of <Foo<'a> as Type>::Assoc

Ariel Ben-Yehuda (Feb 06 2019 at 21:15, on Zulip):

and in that case, you want the MIR to contain a usertypeascribe thingy

Ariel Ben-Yehuda (Feb 06 2019 at 21:15, on Zulip):

which will generate the region error

Ariel Ben-Yehuda (Feb 06 2019 at 21:15, on Zulip):

right?

davidtwco (Feb 06 2019 at 21:15, on Zulip):

I guess maybe we could lower it to a equivalent of AscribeUserType that contains a Constant rather than just bypassing it here.

davidtwco (Feb 06 2019 at 21:16, on Zulip):

I would expect so, yes, I'm just unsure if this pattern is also used elsewhere to that effect.

Ariel Ben-Yehuda (Feb 06 2019 at 21:16, on Zulip):

yea, to a AscribeUserType(PatternKind::Whatever)

Ariel Ben-Yehuda (Feb 06 2019 at 21:16, on Zulip):

so the main q. is whether you can have a test that witnesses this

Ariel Ben-Yehuda (Feb 06 2019 at 21:16, on Zulip):

maybe a MIR output test?

davidtwco (Feb 06 2019 at 21:16, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/issue-55511.nll.stderr

davidtwco (Feb 06 2019 at 21:16, on Zulip):

Is that what you are describing?

davidtwco (Feb 06 2019 at 21:17, on Zulip):

If so, the test already exists and I did in fact implement that check in #55937 - that just happened to also lead to this pattern appearing in this PR's function and that wasn't handled at the time. I think.

Ariel Ben-Yehuda (Feb 06 2019 at 21:19, on Zulip):

maybe it just never does anything with the AscribeUserType?

Ariel Ben-Yehuda (Feb 06 2019 at 21:19, on Zulip):

but that test is not for ranges

davidtwco (Feb 06 2019 at 21:19, on Zulip):

I see.

Ariel Ben-Yehuda (Feb 06 2019 at 21:20, on Zulip):

you'll need to craft a similar test for ranges

Ariel Ben-Yehuda (Feb 06 2019 at 21:21, on Zulip):

maybe just test that the relevant AscribeUserType is present?

davidtwco (Feb 06 2019 at 21:23, on Zulip):

maybe just test that the relevant AscribeUserType is present?

I'm not quite sure what you mean by this.

Ariel Ben-Yehuda (Feb 06 2019 at 21:25, on Zulip):

do a MIR test

davidtwco (Feb 06 2019 at 21:25, on Zulip):

Oh, I see.

Ariel Ben-Yehuda (Feb 06 2019 at 21:25, on Zulip):

because this feels hard to do in a UI test

Ariel Ben-Yehuda (Feb 06 2019 at 21:26, on Zulip):

because you sort of can't have lifetimes in patterns

Ariel Ben-Yehuda (Feb 06 2019 at 21:26, on Zulip):

*in range patterns

Ariel Ben-Yehuda (Feb 06 2019 at 21:30, on Zulip):

right?

davidtwco (Feb 06 2019 at 21:32, on Zulip):

I think so. I'm just trying to work out, if we should be preserving the AscribeUserType in the range pattern we create, what that would look like. Since the changes we're making here are the lowering to the HAIR - so both before and after this PR, there's not a way to really inspect the pattern we select in the MIR (unless we know what form it ends up as).

Ariel Ben-Yehuda (Feb 06 2019 at 21:33, on Zulip):

you can do a mir-opt test

Ariel Ben-Yehuda (Feb 06 2019 at 21:33, on Zulip):

I think, at least

Ariel Ben-Yehuda (Feb 06 2019 at 21:33, on Zulip):

you could hardcode the form it ends up being

davidtwco (Feb 06 2019 at 21:33, on Zulip):

Yeah, I know to use a mir-opt test. I just not too sure what I'm looking for yet.

Ariel Ben-Yehuda (Feb 06 2019 at 21:34, on Zulip):

or just check that there is an AscribeUserType in the MIR
along with the right
| 0: Canonical { max_universe: U0, variables: [], value: TypeOf(DefId(0/0:4 ~ ex[8787]::Foo[0]::ZERO[0]), UserSubsts { substs: [A], user_self_ty: None }) } at ex.rs:11:9: 11:16

Ariel Ben-Yehuda (Feb 06 2019 at 21:34, on Zulip):

or equivalent of that line

Ariel Ben-Yehuda (Feb 06 2019 at 21:34, on Zulip):

so use -Z mir-dump=all to see what it generates

Ariel Ben-Yehuda (Feb 06 2019 at 21:34, on Zulip):

eyeball that it is correct, and copy that

davidtwco (Feb 06 2019 at 21:34, on Zulip):

Right, check that it is still in the map so it is well-formedness checked.

Ariel Ben-Yehuda (Feb 06 2019 at 21:35, on Zulip):

I would have a test of the form

 | 0: Canonical { max_universe: U0, variables: [], value: TypeOf(DefId(0/0:4 ~ ex[8787]::Foo[0]::ZERO[0]), UserSubsts { substs: [A], user_self_ty: None }) } at ex.rs:11:9: 11:16
// ...
        AscribeUserType(_1, -, UserTypeProjection { base: UserTypeAnnotation(1), projs: [] }); // bb8[0]: scope 0 at ex.rs:11:9: 11:16
Ariel Ben-Yehuda (Feb 06 2019 at 21:35, on Zulip):

so I hope the span won't be a problem

Ariel Ben-Yehuda (Feb 06 2019 at 21:35, on Zulip):

maybe change it to be behind a // in the output

Ariel Ben-Yehuda (Feb 06 2019 at 21:35, on Zulip):

so that mir-opt will ignore it

Ariel Ben-Yehuda (Feb 06 2019 at 21:36, on Zulip):

| 0: Canonical { max_universe: U0, variables: [], value: TypeOf(DefId(0/0:4 ~ ex[8787]::Foo[0]::ZERO[0]), UserSubsts { substs: [A], user_self_ty: None }) } // at ex.rs:11:9: 11:16

Ariel Ben-Yehuda (Feb 06 2019 at 21:36, on Zulip):

by that, I mean, change the output of the "dumped MIR" to contain the comment thingy

davidtwco (Feb 06 2019 at 21:38, on Zulip):

So, there definitely isn't a AscribeUserType statement or an entry in Mir's user_type_annotations map.

Ariel Ben-Yehuda (Feb 06 2019 at 21:38, on Zulip):

there should be

Ariel Ben-Yehuda (Feb 06 2019 at 21:38, on Zulip):

that's because you are dropping it

davidtwco (Feb 06 2019 at 21:39, on Zulip):

Yeah.

Ariel Ben-Yehuda (Feb 06 2019 at 21:41, on Zulip):

can you fix that?

davidtwco (Feb 06 2019 at 21:41, on Zulip):

I think so.

Ariel Ben-Yehuda (Feb 06 2019 at 21:42, on Zulip):

cool

Matthew Jasper (Feb 07 2019 at 09:24, on Zulip):

Here's a test case that should error (on both functions): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=fe632010ad3dada68e2463919215a829

Matthew Jasper (Feb 07 2019 at 09:25, on Zulip):

inherent implementations appear to be unchecked, in an unrelated problem

davidtwco (Feb 07 2019 at 09:33, on Zulip):

I can get a fix in for this tonight.

davidtwco (Feb 07 2019 at 09:40, on Zulip):

If desired, can land the PR as is to fix miscompilation but not the well-formed mess check and I can follow up with that fix.

Ariel Ben-Yehuda (Feb 07 2019 at 10:21, on Zulip):

can you have a single PR?

davidtwco (Feb 07 2019 at 12:54, on Zulip):

Can do.

Ariel Ben-Yehuda (Feb 07 2019 at 22:50, on Zulip):

how are you doing?

davidtwco (Feb 08 2019 at 07:32, on Zulip):

Getting on fine with it, just busy with All Hands things so probably won't get a fix ready until tomorrow or when I'm back home.

Ariel Ben-Yehuda (Feb 08 2019 at 12:30, on Zulip):

this turns out to be more complicated than we thought, so we'll do the miscompilation fix for beta, and the other change in a separate PR

Last update: Nov 16 2019 at 02:30UTC