Stream: t-compiler/wg-nll

Topic: #57202 bounds from promoted constant


nikomatsakis (Feb 28 2019 at 15:15, on Zulip):

Hey @Matthew Jasper -- can we schedule a time to chat about https://github.com/rust-lang/rust/pull/57202, together with @pnkfelix perhaps? I'd be available tomorrow for the most part, or we can find a time next week

Matthew Jasper (Feb 28 2019 at 15:17, on Zulip):

I'll probably be available from 19:00 UTC tomorrow

nikomatsakis (Mar 01 2019 at 19:01, on Zulip):

So today turned out to be terrible :P

nikomatsakis (Mar 01 2019 at 19:01, on Zulip):

I mean it's not over yet

Matthew Jasper (Mar 01 2019 at 19:02, on Zulip):

I'm available now if you want to talk. Otherwise I'm available from around this time most weekdays.

nikomatsakis (Mar 01 2019 at 19:04, on Zulip):

I was going to suggest before the NLL meeting next week

nikomatsakis (Mar 01 2019 at 19:05, on Zulip):

Probably that doesn't work for @pnkfelix but that's .. probably ok

nikomatsakis (Mar 01 2019 at 19:05, on Zulip):

I'd kind of like to focus on finish up my to do list for this week at the moment, if that's ok, and this doesn't seem that urgent

nikomatsakis (Mar 01 2019 at 19:05, on Zulip):

If that works for you, I'll add a calendar event?

Matthew Jasper (Mar 01 2019 at 19:05, on Zulip):

I was going to suggest before the NLL meeting next week

Sounds good

nikomatsakis (Mar 01 2019 at 19:07, on Zulip):

sent you a calendar invite

nikomatsakis (Mar 01 2019 at 19:07, on Zulip):

I debated about adding to the rustc calendar but it seems...not quite to rise to that level :)

nikomatsakis (Mar 01 2019 at 19:07, on Zulip):

thanks!

Matthew Jasper (Mar 06 2019 at 19:25, on Zulip):

@nikomatsakis I might be a little late for the meeting tonight, hopefully it won't need the whole hour anyway.

nikomatsakis (Mar 06 2019 at 19:33, on Zulip):

OK, let me know when you're around @Matthew Jasper

Matthew Jasper (Mar 06 2019 at 19:42, on Zulip):

@nikomatsakis I'm here

nikomatsakis (Mar 06 2019 at 19:42, on Zulip):

Hey @Matthew Jasper =)

nikomatsakis (Mar 06 2019 at 19:43, on Zulip):

So I'm looking over the PR

nikomatsakis (Mar 06 2019 at 19:43, on Zulip):

I'm trying to make a good summary of what it does, to start

nikomatsakis (Mar 06 2019 at 19:43, on Zulip):

I think the behavior that I would hope to see is something analogous to what we do for closures:

nikomatsakis (Mar 06 2019 at 19:43, on Zulip):

We have some kind of query that we can execute which types the constant and produces a "constraint set" that we can incorporate into our result

nikomatsakis (Mar 06 2019 at 19:43, on Zulip):

I'm curious if you considered that and why it didn't work

nikomatsakis (Mar 06 2019 at 19:44, on Zulip):

Re-reading the PR, it looks .. kind of like what it does

nikomatsakis (Mar 06 2019 at 19:45, on Zulip):

I guess that on each reference to a promoted constant

nikomatsakis (Mar 06 2019 at 19:45, on Zulip):

we run the type-check on it again

nikomatsakis (Mar 06 2019 at 19:45, on Zulip):

I suppose most promoted constants are only referenced once

Matthew Jasper (Mar 06 2019 at 19:46, on Zulip):

I considered it. I didn't like it because it felt "too heavy".

Matthew Jasper (Mar 06 2019 at 19:46, on Zulip):

I suppose most promoted constants are only referenced once

I think so, we definitely don't deduplicate them as much as we could

nikomatsakis (Mar 06 2019 at 19:47, on Zulip):

Probably that de-duplication would come later

nikomatsakis (Mar 06 2019 at 19:47, on Zulip):

Although you could imagine doing it early

nikomatsakis (Mar 06 2019 at 19:47, on Zulip):

I considered it. I didn't like it because it felt "too heavy".

interesting. Heavy from a runtime overhead perspective or something like that?

nikomatsakis (Mar 06 2019 at 19:47, on Zulip):

It feels cleaner to me

nikomatsakis (Mar 06 2019 at 19:47, on Zulip):

I guess because I really wanted us to get to the point where

Matthew Jasper (Mar 06 2019 at 19:48, on Zulip):

Yes, they're deduplicated in the llvm output.

nikomatsakis (Mar 06 2019 at 19:48, on Zulip):

we weren't type-checking multiple MIR functions within one context

nikomatsakis (Mar 06 2019 at 19:48, on Zulip):

it feels "confusing" to do so, since regions are sets of points, but points from which MIR?

Matthew Jasper (Mar 06 2019 at 19:49, on Zulip):

Yes, I essential collapsed the promoted MIR to a point

nikomatsakis (Mar 06 2019 at 19:49, on Zulip):

I guess that is this line:

constraint.locations = locations;

nikomatsakis (Mar 06 2019 at 19:50, on Zulip):

how hard do you think it would be to restructure this into a recursive query?

nikomatsakis (Mar 06 2019 at 19:50, on Zulip):

I wonder if the existing query around closures can .. mostly just work?

nikomatsakis (Mar 06 2019 at 19:50, on Zulip):

in some sense a promoted constant is a lot like a closure

Matthew Jasper (Mar 06 2019 at 19:51, on Zulip):

I guess that is this line:

constraint.locations = locations;

yes

Matthew Jasper (Mar 06 2019 at 19:53, on Zulip):

in some sense a promoted constant is a lot like a closure

Except it promoteds share type annotations. I don't think I could find a case where we need to ensure the types are unified between the promoted and non-promoted MIR though.

nikomatsakis (Mar 06 2019 at 19:54, on Zulip):

what do you mean by that?

nikomatsakis (Mar 06 2019 at 19:54, on Zulip):

I noticed that comment

Matthew Jasper (Mar 06 2019 at 19:55, on Zulip):

I wonder if the existing query around closures can .. mostly just work?

As in changing the query to mir_borrowck(DefId, Option<Promoted>), making sure that the return type uses external regions. Well, it's possible.

nikomatsakis (Mar 06 2019 at 19:55, on Zulip):

ah, a type annotation is the canonical version of the things users wrote by hand, right?

nikomatsakis (Mar 06 2019 at 19:55, on Zulip):

I was looking at this line and trying to remember what it was

nikomatsakis (Mar 06 2019 at 19:56, on Zulip):

user_type_annotations: &'a CanonicalUserTypeAnnotations<'tcx>,

nikomatsakis (Mar 06 2019 at 19:56, on Zulip):

As in changing the query to mir_borrowck(DefId, Option<Promoted>), making sure that the return type uses external regions. Well, it's possible.

what is Option<Promoted> here?

Matthew Jasper (Mar 06 2019 at 19:57, on Zulip):

The id of the promoted we need to check, or None if we want to check the main MIR

Matthew Jasper (Mar 06 2019 at 19:58, on Zulip):

MIR contains a field pub user_type_annotations: CanonicalUserTypeAnnotations<'tcx>, which is indexed by UserTypeAnnotationIndex.

nikomatsakis (Mar 06 2019 at 19:58, on Zulip):

The id of the promoted we need to check, or None if we want to check the main MIR

oh, right, they don't have def-ids of their own

nikomatsakis (Mar 06 2019 at 19:59, on Zulip):

MIR contains a field pub user_type_annotations: CanonicalUserTypeAnnotations<'tcx>, which is indexed by UserTypeAnnotationIndex.

yeah I remember this now

Matthew Jasper (Mar 06 2019 at 19:59, on Zulip):

TypeAnnotations in promoted index into the list of their parent.

Matthew Jasper (Mar 06 2019 at 19:59, on Zulip):

i.e. we don't renumber them

nikomatsakis (Mar 06 2019 at 19:59, on Zulip):

seems like they should really copy that field over from their parent

nikomatsakis (Mar 06 2019 at 19:59, on Zulip):

but..

nikomatsakis (Mar 06 2019 at 19:59, on Zulip):

...well, so,

nikomatsakis (Mar 06 2019 at 20:00, on Zulip):

that seems to imply that type-checking the promoted constants may "inform" the user-type annot from parent, right?

nikomatsakis (Mar 06 2019 at 20:00, on Zulip):

i.e., they may share type variables

Matthew Jasper (Mar 06 2019 at 20:00, on Zulip):

Yes.

nikomatsakis (Mar 06 2019 at 20:00, on Zulip):

at least in the present setup, they definitely do

nikomatsakis (Mar 06 2019 at 20:00, on Zulip):

that certainly complicates matters

Matthew Jasper (Mar 06 2019 at 20:00, on Zulip):

Well, they also shared region variables right now.

nikomatsakis (Mar 06 2019 at 20:01, on Zulip):

in what sense?

nikomatsakis (Mar 06 2019 at 20:01, on Zulip):

they get renumbered, no?

nikomatsakis (Mar 06 2019 at 20:01, on Zulip):

they share free regions

Matthew Jasper (Mar 06 2019 at 20:01, on Zulip):

In the sense that they're renumbered together.

nikomatsakis (Mar 06 2019 at 20:01, on Zulip):

but do they share other regions too?

Matthew Jasper (Mar 06 2019 at 20:02, on Zulip):

Which I guess could be described in the opposite way.

nikomatsakis (Mar 06 2019 at 20:02, on Zulip):

:)

nikomatsakis (Mar 06 2019 at 20:03, on Zulip):

I guess what I'm saying is, apart from free regions, they should have disjoint region variables

nikomatsakis (Mar 06 2019 at 20:03, on Zulip):

(unless i'm misunderstanding something)

Matthew Jasper (Mar 06 2019 at 20:03, on Zulip):

Yes

nikomatsakis (Mar 06 2019 at 20:03, on Zulip):

the user type annotation thing is a bit odd

nikomatsakis (Mar 06 2019 at 20:04, on Zulip):

I guess I have to review the details of what we did there

nikomatsakis (Mar 06 2019 at 20:04, on Zulip):

it makes me a bit nervous that the promoted can -- in theory, anyway -- influence the outside type-check by unifying some type variable

nikomatsakis (Mar 06 2019 at 20:04, on Zulip):

otoh, I can't see why they would share type annotations with the surrounding context

nikomatsakis (Mar 06 2019 at 20:05, on Zulip):

I don't think I could find a case where we need to ensure the types are unified between the promoted and non-promoted MIR though.

I guess this is what you were talking about here

Matthew Jasper (Mar 06 2019 at 20:07, on Zulip):

Yes, so the potential issue is something like:

match constant_with_types::<Types> {
    ref x => (), // x refernces promoted MIR
    ref y => (), // y reference different promoted MIR
    ref mut z => (), // z references a temporary
}
Matthew Jasper (Mar 06 2019 at 20:08, on Zulip):

Not that sharing type annotations is obviously enough here.

nikomatsakis (Mar 06 2019 at 20:09, on Zulip):

why would x and y reference a different promoted MIR? will we promote same MIR twice?

Matthew Jasper (Mar 06 2019 at 20:09, on Zulip):

Yes, we promote for each use

nikomatsakis (Mar 06 2019 at 20:09, on Zulip):

(also, seems a bit strange that we promote at all there, but ...)

nikomatsakis (Mar 06 2019 at 20:10, on Zulip):

an aside: I keep wondering about the role of a HAIR

nikomatsakis (Mar 06 2019 at 20:10, on Zulip):

i.e., I'm not entirely convinced that promotion should operate on the MIR level

nikomatsakis (Mar 06 2019 at 20:10, on Zulip):

but a bit late for that I guess

nikomatsakis (Mar 06 2019 at 20:10, on Zulip):

I guess a side-effect of the above would be that x and y may have 'static lifetime but z cannot?

Matthew Jasper (Mar 06 2019 at 20:11, on Zulip):

indeed

nikomatsakis (Mar 06 2019 at 20:11, on Zulip):

/me grumbles

Matthew Jasper (Mar 06 2019 at 20:11, on Zulip):

The ref mut thing is arguably a bug.

nikomatsakis (Mar 06 2019 at 20:11, on Zulip):

what about it specifically?

nikomatsakis (Mar 06 2019 at 20:11, on Zulip):

like, what do you see as the correct behavior here?

Matthew Jasper (Mar 06 2019 at 20:12, on Zulip):

(ref mut z,) prevents any promotion

Matthew Jasper (Mar 06 2019 at 20:12, on Zulip):

The correct behavior is for them to be the same.

nikomatsakis (Mar 06 2019 at 20:12, on Zulip):

i.e., if we were doing &mut x.0 instead of &mut x, we would not promote x anywhere

nikomatsakis (Mar 06 2019 at 20:13, on Zulip):

I agree that feels like a bug

nikomatsakis (Mar 06 2019 at 20:13, on Zulip):

(cc @oli -- question about promotion here =)

nikomatsakis (Mar 06 2019 at 20:13, on Zulip):

Or at least it feels mildly surprising

nikomatsakis (Mar 06 2019 at 20:15, on Zulip):

@Matthew Jasper well even leaving that aside, one possibility might be that .. somehow .. type-checking x could influence type-checking y?

nikomatsakis (Mar 06 2019 at 20:15, on Zulip):

i.e., because they share a type variable through the user-type-annot

nikomatsakis (Mar 06 2019 at 20:16, on Zulip):

which is shared

nikomatsakis (Mar 06 2019 at 20:16, on Zulip):

kind of hard for me to see though

nikomatsakis (Mar 06 2019 at 20:16, on Zulip):

Also, @Matthew Jasper, are we giving some of error if type variables from the user-type annot wind up unconstrained?

nikomatsakis (Mar 06 2019 at 20:17, on Zulip):

Anyway, I think my conclusion from this conversation is that I would indeed prefer to try a closure-like approach, but I'm .. fairly convinced that the current one is largely equivalent in practice.

Matthew Jasper (Mar 06 2019 at 20:17, on Zulip):

Well, they don't share a type variable actually, since I changed type annotations to do place type <-> inferred renumbered type <-> type annotation

Matthew Jasper (Mar 06 2019 at 20:18, on Zulip):

Also, Matthew Jasper, are we giving some of error if type variables from the user-type annot wind up unconstrained?

I certainly haven't added one.

nikomatsakis (Mar 06 2019 at 20:18, on Zulip):

Well, they don't share a type variable actually, since I changed type annotations to do place type <-> inferred renumbered type <-> type annotation

what does <-> represent here?

Matthew Jasper (Mar 06 2019 at 20:18, on Zulip):

"Are used in the same call of eq or sub"

Matthew Jasper (Mar 06 2019 at 20:19, on Zulip):

Or "are related directly while type checking"

nikomatsakis (Mar 06 2019 at 20:20, on Zulip):

ok, I'm reading into the user-type annotation

nikomatsakis (Mar 06 2019 at 20:20, on Zulip):

I see that we have:

pub struct CanonicalUserTypeAnnotation<'tcx> {
    pub user_ty: CanonicalUserType<'tcx>,
    pub span: Span,
    pub inferred_ty: Ty<'tcx>,
}
nikomatsakis (Mar 06 2019 at 20:20, on Zulip):

inferred_ty, I take it, is the type that type-check resulted in?

Matthew Jasper (Mar 06 2019 at 20:21, on Zulip):

Yes, although it's obtained somewhat hackily from the expression/pattern that the type annotation is being applied to.

Matthew Jasper (Mar 06 2019 at 20:21, on Zulip):

I think at least.

Matthew Jasper (Mar 06 2019 at 20:22, on Zulip):

Although now I'm not sure why it should be done that way.

nikomatsakis (Mar 06 2019 at 20:23, on Zulip):

I was looking through the code a bit to see where it comes from

nikomatsakis (Mar 06 2019 at 20:24, on Zulip):

I remember thinking that would be necessary

nikomatsakis (Mar 06 2019 at 20:24, on Zulip):

but I can't entirely remember why anymore :)

nikomatsakis (Mar 06 2019 at 20:24, on Zulip):

so yeah looks like it comes from the expr.ty on the HAIR expression

nikomatsakis (Mar 06 2019 at 20:25, on Zulip):

..which is probably ok

nikomatsakis (Mar 06 2019 at 20:25, on Zulip):

anyway an orthogonal thing

Matthew Jasper (Mar 06 2019 at 20:25, on Zulip):

There was a point where I remember having problems with canocicalizing one type and resolving another.

nikomatsakis (Mar 06 2019 at 20:26, on Zulip):

right, so the scheme now is that we use the inferred_ty to relate to everything

nikomatsakis (Mar 06 2019 at 20:26, on Zulip):

and then we relate the (instantiated) user-given type to that

nikomatsakis (Mar 06 2019 at 20:26, on Zulip):

I don't know how much that changes things, though, in that the inferred_ty thus represents shared regions between the various promoted constants

nikomatsakis (Mar 06 2019 at 20:27, on Zulip):

but it doesn't sound like a big problem

nikomatsakis (Mar 06 2019 at 20:27, on Zulip):

I think there's no real concern here

nikomatsakis (Mar 06 2019 at 20:27, on Zulip):

I was more worried about having some type variable like ?X where one arm leaves it unconstrained but the other equates it to u32 and somehow that causes trouble when checking the constant from the first arm, since ?X would never wind up as u32

nikomatsakis (Mar 06 2019 at 20:29, on Zulip):

I'm trying to think now how this closure-like check might work and whether we could lose region constraints

Matthew Jasper (Mar 06 2019 at 20:30, on Zulip):

I think doing the obvious thing and seeing what goes wrong should be fine.

nikomatsakis (Mar 06 2019 at 20:32, on Zulip):

I was just reading over the tests

nikomatsakis (Mar 06 2019 at 20:32, on Zulip):

Are you interested in trying that change?

nikomatsakis (Mar 06 2019 at 20:32, on Zulip):

(I see meeting is now)

Matthew Jasper (Mar 06 2019 at 20:33, on Zulip):

Yes, but I may take a while to get around to it

nikomatsakis (Mar 06 2019 at 20:35, on Zulip):

Should we file an issue about it?

nikomatsakis (Mar 06 2019 at 20:35, on Zulip):

(Probably)

nikomatsakis (Mar 06 2019 at 20:36, on Zulip):

It doesn't seem like high priority

Matthew Jasper (Mar 06 2019 at 20:36, on Zulip):

Should we file an issue about it?

:+1:

nikomatsakis (Mar 06 2019 at 20:36, on Zulip):

I'd like to at least link to this thread :)

oli (Mar 07 2019 at 09:11, on Zulip):

I was slightly suprised promotion works across match arms

oli (Mar 07 2019 at 09:12, on Zulip):

I could not come up with anything problematic, but @eddyb worries that this could be a symptom that there might be problematic cases

blitzerr (Mar 07 2019 at 10:07, on Zulip):

I could not come up with anything problematic, but eddyb worries that this could be a symptom that there might be problematic cases

@oli Are you suggesting that the reason you coouldn't come up with something problematic is an indication enough that there might be something problematic ? :smiley:

oli (Mar 07 2019 at 10:09, on Zulip):

that sentence took me a bit to parse

oli (Mar 07 2019 at 10:09, on Zulip):

No, I believe there's no way to abuse this (right now)

oli (Mar 07 2019 at 10:10, on Zulip):

I screwed up the previous sentence. this is "promotion works across match arms" and not "I could not come up with anything problematic"

blitzerr (Mar 07 2019 at 10:13, on Zulip):

@oli No worries. I was just being annoying :smiley:

nikomatsakis (Mar 08 2019 at 10:47, on Zulip):

I could not come up with anything problematic, but eddyb worries that this could be a symptom that there might be problematic cases

It does worry me mildly, @oli

nikomatsakis (Mar 08 2019 at 10:47, on Zulip):

Did you see the point about mixed ref mut vs ref and how that could result in perhaps surprising lifetimes?

nikomatsakis (Mar 08 2019 at 10:48, on Zulip):

Yes, so the potential issue is something like:

match constant_with_types::<Types> {
    ref x => (), // x refernces promoted MIR
    ref y => (), // y reference different promoted MIR
    ref mut z => (), // z references a temporary
}

in particular here the type of x and y could wind up inferred to &'static Foo while z would be &'x mut Foo -- i.e., it cannot be 'static

nikomatsakis (Mar 08 2019 at 10:48, on Zulip):

@Matthew Jasper there isn't an issue filed about this discrepancy, is there?

Matthew Jasper (Mar 08 2019 at 12:00, on Zulip):

There's no issue for this.

oli (Mar 08 2019 at 12:31, on Zulip):

We basically have to treat such a match as

let tmp = constant_with_types::<Types>;
match constant_with_types::<Types> {
    _ => { let x = &constant_with_types::<Types>; },
    _ => { let y = &constant_with_types::<Types>; },
    _ => { let z = &mut tmp; },
}
oli (Mar 08 2019 at 12:36, on Zulip):

I agree with the surprising part, I was surprised by it after all. Not sure if there's a discrepancy here that needs addressing.

Last update: Nov 21 2019 at 14:30UTC