Stream: t-compiler/wg-nll

Topic: #54943 user type annot in unreachable code


davidtwco (Nov 08 2018 at 22:50, on Zulip):

@pnkfelix I'd be interested in digging into this issue a little, particularly the first proposed solution by @nikomatsakis if you don't mind me taking it off your plate :slight_smile:

pnkfelix (Nov 08 2018 at 22:50, on Zulip):

please do!

davidtwco (Nov 12 2018 at 15:20, on Zulip):

Just an update on where I'm at with this: yesterday made the refactoring that moved user type ascriptions into an IndexVec on the Mir - got that all compiling but hitting an ICE that I've not had bunch time to look into. Going to keep digging on this tonight.

nikomatsakis (Nov 12 2018 at 17:54, on Zulip):

@davidtwco I think that approach is still the one I expect to work

nikomatsakis (Nov 12 2018 at 17:54, on Zulip):

:)

davidtwco (Nov 12 2018 at 17:54, on Zulip):

I expect it will. Fairly sure the ICE is a mistake I’ve made.

davidtwco (Nov 13 2018 at 21:28, on Zulip):

@nikomatsakis Would you be able to clarify the second half of your proposed fix? I'm not super clear on how I'd go about doing that.

So far I've moved the information that was previously in StatementKind::AscribeUserTy into the IndexVec in the Mir (I also added a Span as suggested) and then changed that statement kind to just keep an index.

That compiles but was ICE'ing somewhere in the serialization code from derives - so I tried a full rebuild and now it struggles to compile rustc in stage 1 (it can do everything before that) with a error: internal compiler error: librustc_mir/borrow_check/nll/universal_regions.rs:754: cannot convert `ReScope(Remainder { block: ItemLocalId(2120), first_statement_index: 0})` to a region vid.

Figured I might just need to continue on with the implementation and it might be a symptom of that. I assumed doing the refactoring into an IndexVec might be doable in isolation.

davidtwco (Nov 13 2018 at 21:34, on Zulip):

(as far as I can tell, the only change that seems like it'd have an impact in that refactoring is that the visit_place call that normally came from visiting StatementKind::AscribeUserTy doesn't happen because the Place<'tcx> is now stored in the IndexVec and so when the visitor gets to that StatementKind it doesn't have that to call visit_place with. Similarly, when visiting the items in the IndexVec (much like how the visiting locals happens), I don't have the Location - so I can't visit_place then either)

nikomatsakis (Nov 13 2018 at 21:55, on Zulip):

@davidtwco it feels like that refactoring should be do-able in isolation

nikomatsakis (Nov 13 2018 at 21:55, on Zulip):

maybe worth opening a [WIP] PR..

davidtwco (Nov 13 2018 at 21:56, on Zulip):

Sure thing.

nikomatsakis (Nov 13 2018 at 21:56, on Zulip):

I don't remember what the second half of my instructions were

nikomatsakis (Nov 13 2018 at 21:56, on Zulip):

so i'll have to check

davidtwco (Nov 13 2018 at 21:56, on Zulip):

link to comment

nikomatsakis (Nov 13 2018 at 21:56, on Zulip):

oh, right

davidtwco (Nov 13 2018 at 22:00, on Zulip):

#55937

davidtwco (Nov 13 2018 at 22:29, on Zulip):

Debugging an issue that happens compiling compiler crates is incredibly painful.

davidtwco (Nov 13 2018 at 22:35, on Zulip):

Only 17G of logs..

davidtwco (Nov 13 2018 at 22:43, on Zulip):

There's so much here I don't even know where I'd start. Seeing mentions of bb738 and higher..

nikomatsakis (Nov 14 2018 at 14:18, on Zulip):

@davidtwco I have some advice :)

nikomatsakis (Nov 14 2018 at 14:18, on Zulip):

which crate is failing, first of all?

davidtwco (Nov 14 2018 at 14:18, on Zulip):

rustc

nikomatsakis (Nov 14 2018 at 14:18, on Zulip):

ok, that's good

nikomatsakis (Nov 14 2018 at 14:18, on Zulip):

that means libstd is available

nikomatsakis (Nov 14 2018 at 14:18, on Zulip):

so, obviosuly, trying to narrow down to a smaller example is the best thing

nikomatsakis (Nov 14 2018 at 14:19, on Zulip):

in a pinch, I sometimes resort to this

RUST_LOG=rustc::foo rustc 2>&1 | tail -100000 > killme

i.e., keep only the last 100,000 lines of log

nikomatsakis (Nov 14 2018 at 14:19, on Zulip):

that's usually enough to capture the problem, without having an 17GB file

davidtwco (Nov 14 2018 at 14:19, on Zulip):

I'll definitely tail it like that if I need to do this again.

nikomatsakis (Nov 14 2018 at 14:19, on Zulip):

you might also consider checking the run-pass tests

davidtwco (Nov 14 2018 at 14:19, on Zulip):

But I've got the 17G file now.

nikomatsakis (Nov 14 2018 at 14:19, on Zulip):

well, I personally find tools die on that :)

nikomatsakis (Nov 14 2018 at 14:20, on Zulip):

but yes you could just tail the file :)

davidtwco (Nov 14 2018 at 14:20, on Zulip):

Fair, less seemed to be able to handle it.

nikomatsakis (Nov 14 2018 at 14:20, on Zulip):

I also tend to do a lot of "grep"

nikomatsakis (Nov 14 2018 at 14:20, on Zulip):

so having a smaller file helps there

nikomatsakis (Nov 14 2018 at 14:20, on Zulip):

anyway, yes, less is one of those tools that scales pretty well

nikomatsakis (Nov 14 2018 at 14:20, on Zulip):

is it ICEing?

davidtwco (Nov 14 2018 at 14:21, on Zulip):

It is.

nikomatsakis (Nov 14 2018 at 14:25, on Zulip):

OK

davidtwco (Nov 14 2018 at 14:25, on Zulip):

Here's the last 10k lines. ICE is at the very bottom.

davidtwco (Nov 14 2018 at 14:26, on Zulip):

It doesn't have the MIR build where the add_user_ty_ascription call (new function, adds to the new IndexVec) happens in that, but I've triple checked that those add the same things into the IndexVec that were previously added into the statements (just by looking at the code).

davidtwco (Nov 14 2018 at 14:28, on Zulip):

Here's the gist with the add_user_ty_ascription - the one being asserted is added at the very bottom.

davidtwco (Nov 14 2018 at 14:32, on Zulip):

Didn't make much progress digging into this last night and haven't had a chance to look into it much today.

davidtwco (Nov 14 2018 at 14:45, on Zulip):

run-pass tests are a good tip, very helpful for making the issue more manageable. Sadly, every run-pass failure I'm seeing is completely unrelated.

Jake Goulding (Nov 14 2018 at 15:20, on Zulip):

tend to do a lot of "grep"

Clearly you mean rg ;-)

davidtwco (Nov 14 2018 at 15:31, on Zulip):

Attempted to add in a visit_place that was lost in the visitor with the refactor - ended up causing an ICE earlier in stage1 at a seemingly more unrelated place.

davidtwco (Nov 14 2018 at 15:31, on Zulip):

(as far as I can tell, the only change that seems like it'd have an impact in that refactoring is that the visit_place call that normally came from visiting StatementKind::AscribeUserTy doesn't happen because the Place<'tcx> is now stored in the IndexVec and so when the visitor gets to that StatementKind it doesn't have that to call visit_place with. Similarly, when visiting the items in the IndexVec (much like how the visiting locals happens), I don't have the Location - so I can't visit_place then either)

(context for the missing visit_place)

nikomatsakis (Nov 14 2018 at 15:38, on Zulip):

[Quoting…]
sadly this is often the case :(

does mean we should try to make a reduced test case :)

davidtwco (Nov 14 2018 at 15:53, on Zulip):

This line seems to be the one causing the type annotation error.

davidtwco (Nov 14 2018 at 15:53, on Zulip):

I'm not sure what case that falls under.

davidtwco (Nov 14 2018 at 15:54, on Zulip):

According to the spans that are saved at the MIR build; types look like the ones being mentioned too.

nikomatsakis (Nov 14 2018 at 17:00, on Zulip):

@davidtwco took a quick look at the PR, first of all, and left some comments -- more questions than answers, I'm afraid

nikomatsakis (Nov 14 2018 at 17:00, on Zulip):

Well, actually

nikomatsakis (Nov 14 2018 at 17:01, on Zulip):

I suspect your ICE is indeed caused by the change to the visitor

nikomatsakis (Nov 14 2018 at 17:01, on Zulip):

In particular, not visiting the Place means we will no longer "renumber" the regions within

davidtwco (Nov 14 2018 at 17:17, on Zulip):

@nikomatsakis thanks for taking a look. The main issue I found with calling visit_place is that it requires a Location which I don’t have at that point in the visitor any more.

davidtwco (Nov 14 2018 at 17:18, on Zulip):

I tried to put a location into UserTypeAscriptionData but that didn’t quite work. Got it compiling but it would ICE earlier for a stranger reason that I didn’t look too much into.

nikomatsakis (Nov 14 2018 at 17:19, on Zulip):

@davidtwco yeah, I figured this was part of it; well, this is partly why I suggested not pulling out so many fields

nikomatsakis (Nov 14 2018 at 17:19, on Zulip):

but rather just the "core type"

nikomatsakis (Nov 14 2018 at 17:19, on Zulip):

however, we could also make a "PlaceContext" that doesn't have a location I think

davidtwco (Nov 14 2018 at 17:21, on Zulip):

PlaceContext doesn’t take a location, it’s just a parameter to the visit_place function.

davidtwco (Nov 14 2018 at 17:22, on Zulip):

I’m unclear on how this refactoring (and what little I understand of the remaining steps for the larger change) would help with the unreachable code issue though. Since surely the statements that keep indices into our IndexVec will be removed as unreachable in the same way they are now?

davidtwco (Nov 15 2018 at 20:56, on Zulip):

@nikomatsakis Apologies for another ping regarding this issue, would liked to have made more progress by now. Took another attempt at this and still not getting very far. So far, I've tried:

I've not yet tried moving the place back into the statement so that it can be visited when the statement is visited - that'd solve the renumbering issue, but I'm unsure how it would solve the issue of user type annotations not being checked in dead code since those statements with less in them could still be removed.

nikomatsakis (Nov 15 2018 at 22:11, on Zulip):

You don't need the place to check for WF

nikomatsakis (Nov 15 2018 at 22:12, on Zulip):

I think you would ideally move more than just the place back into the statement

nikomatsakis (Nov 15 2018 at 22:12, on Zulip):

you would also move the "projections" part

nikomatsakis (Nov 15 2018 at 22:12, on Zulip):

that said, I don't know why you need a location to visit a place, let me double check the visitor

nikomatsakis (Nov 15 2018 at 22:13, on Zulip):

oh, I see, because visit_place wants you to give one :)

davidtwco (Nov 15 2018 at 22:13, on Zulip):

Yeah.

nikomatsakis (Nov 15 2018 at 22:13, on Zulip):

we could alter the visitor

nikomatsakis (Nov 15 2018 at 22:13, on Zulip):

but I feel like we should avoid that for now

nikomatsakis (Nov 15 2018 at 22:13, on Zulip):

let me try and explain what I meant by "move the projections part" better

davidtwco (Nov 15 2018 at 22:14, on Zulip):

Don't we need to get a Place when we do the wf check here?

davidtwco (Nov 15 2018 at 22:14, on Zulip):

Or is that the wrong check?

nikomatsakis (Nov 15 2018 at 22:14, on Zulip):

so we have this

pub struct UserTypeProjection<'tcx> {
    pub base: UserTypeAnnotation<'tcx>,
    pub projs: Vec<ProjectionElem<'tcx, (), ()>>,
}
nikomatsakis (Nov 15 2018 at 22:14, on Zulip):

I was intending only to move the base field here into the array

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

and leave the rest as is

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

i.e., that structure would become

pub struct UserTypeProjection<'tcx> {
    pub base: UserTypeAnnotationIndex,
    pub projs: Vec<ProjectionElem<'tcx, (), ()>>,
}
nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

Don't we need to get a Place when we do the wf check here?

that is not the wf check

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

that is relating the type of the place to the user-given type

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

that stuff would stay the same

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

or, well, not the same

nikomatsakis (Nov 15 2018 at 22:15, on Zulip):

but it would still occur at that time

nikomatsakis (Nov 15 2018 at 22:16, on Zulip):

this part we would probably do earlier, ahead of time, just by iterating over the IndexVec and creating a new instantiated_types: IndexVec<UserTypeAnnotationIndex, Ty<'tcx>>

davidtwco (Nov 15 2018 at 22:16, on Zulip):

Ah, I thought the issue was that that code wasn't being called because the AscribeUserType statements were being removed, so the intent was to stop the data required for that being kept in a statement. :face_palm:

nikomatsakis (Nov 15 2018 at 22:17, on Zulip):

at the end, we would also go over all the things in that vec, and try to prove them well-formed

nikomatsakis (Nov 15 2018 at 22:17, on Zulip):

using Locations::All I guess

nikomatsakis (Nov 15 2018 at 22:17, on Zulip):

but the relate_type_and_user_type code would stay mostly the same,

nikomatsakis (Nov 15 2018 at 22:17, on Zulip):

except that it reads the instantiated type from the vector

nikomatsakis (Nov 15 2018 at 22:17, on Zulip):

instead of doing that work itself

nikomatsakis (Nov 15 2018 at 22:18, on Zulip):

I guess we would also want to normalize the instantiated type

davidtwco (Nov 15 2018 at 22:18, on Zulip):

So where is the wf check now?

nikomatsakis (Nov 15 2018 at 22:19, on Zulip):

hmm

nikomatsakis (Nov 15 2018 at 22:19, on Zulip):

I think it depends

nikomatsakis (Nov 15 2018 at 22:19, on Zulip):

:)

nikomatsakis (Nov 15 2018 at 22:20, on Zulip):

there are the two branches

nikomatsakis (Nov 15 2018 at 22:20, on Zulip):

on the *other* branch, we wind up here

nikomatsakis (Nov 15 2018 at 22:20, on Zulip):

I'm trying to decide if that has to change :P

nikomatsakis (Nov 15 2018 at 22:20, on Zulip):

I think that path comes up when you do stuff like

nikomatsakis (Nov 15 2018 at 22:21, on Zulip):
fn foo<T: 'static>() { ... }

fn main<'a>() {
    return;

    let x = foo::<&'a u32>(); // no error
}
nikomatsakis (Nov 15 2018 at 22:21, on Zulip):

(I confess that some part of me is back to wanting to say "screw it, this is fine as is")

nikomatsakis (Nov 15 2018 at 22:22, on Zulip):

I think that path comes up when you do stuff like

i.e,. the user didn't write a full type, but rather supplied explicit arguments to something

nikomatsakis (Nov 15 2018 at 22:22, on Zulip):

if the user wrote a full type, I don't think we explicitly prove well-formedness today

nikomatsakis (Nov 15 2018 at 22:22, on Zulip):

instead, we relate it to the type of the place, which we expect to be WF

nikomatsakis (Nov 15 2018 at 22:22, on Zulip):

i.e., in this branch

nikomatsakis (Nov 15 2018 at 22:23, on Zulip):

vs this branch, which is covering the "explicit substitution" case

nikomatsakis (Nov 15 2018 at 22:23, on Zulip):

/me wishes there were more comments, blames self

davidtwco (Nov 15 2018 at 22:23, on Zulip):

I appreciate your patience with this issue, I've been going the completely wrong direction with it all week.

nikomatsakis (Nov 15 2018 at 22:24, on Zulip):

well I appreciate your patience

nikomatsakis (Nov 15 2018 at 22:24, on Zulip):

:)

nikomatsakis (Nov 15 2018 at 22:24, on Zulip):

sorry i've been scattered this week, took me a while to catch up, now trying to track down some regressions :/

davidtwco (Nov 15 2018 at 22:25, on Zulip):

No worries. Now I've got some idea what is actually happening here I should be able to work something together tomorrow.

blitzerr (Nov 16 2018 at 00:03, on Zulip):

@nikomatsakis

sorry i've been scattered this week, took me a while to catch up, now trying to track down some regressions :confused:

Is there a list of these regressions somewhere ?

davidtwco (Nov 16 2018 at 18:16, on Zulip):

@nikomatsakis Pushed a new commit that is way closer to what we want. I had to change more than I thought to be able to check for well-formed-ness (presuming that I did that correctly, of course) but it should be doing that. For some reason, on a bunch of tests (probably the majority) we're seeing errors from libserialize. There are tests that have type annotations that work, so I don't really know what's going on. The test case that we're looking to fix for this issue fails with that ICE so I don't know if this actually fixes the issue yet.

davidtwco (Nov 16 2018 at 18:16, on Zulip):

I can change the way I did some of the things if you're not happy with them, it was a bit more invasive that I'd have expected.

davidtwco (Nov 16 2018 at 18:16, on Zulip):

And I might have missed a simpler way.

davidtwco (Nov 16 2018 at 18:25, on Zulip):

It occurs to me that I might have misinterpreted what you meant yesterday and should just invoke the same query again with the already-instantiated type instead of this new one that I've made.

davidtwco (Nov 16 2018 at 18:25, on Zulip):

Either way, I suspect that will not fix the serialize errors I'm seeing.

davidtwco (Nov 16 2018 at 18:41, on Zulip):

Hadn't tried a stage0 build so hadn't noticed the error on Travis. Will need to think about how to tackle that - the way I'd worked around that to be able to continue was quite hacky.

nikomatsakis (Nov 16 2018 at 19:23, on Zulip):

@davidtwco these changes are reflected in the PR? I'm thinking we should remove this from the milestone for two reasons (you may have seen me comment on discord):

nikomatsakis (Nov 16 2018 at 19:23, on Zulip):

doesn't seem worth the risk

nikomatsakis (Nov 16 2018 at 19:23, on Zulip):

(not to say we shoudln't keep working on it, just lowers the priority level somewhat)

nikomatsakis (Nov 16 2018 at 19:24, on Zulip):

(done)

davidtwco (Nov 16 2018 at 19:24, on Zulip):

I’ve got something compiling for the issue on Travis. Don’t know if it works yet because I’m walking home.

davidtwco (Nov 16 2018 at 19:24, on Zulip):

That’s fine, makes sense.

davidtwco (Nov 16 2018 at 21:58, on Zulip):

Pushed another change. Stripped it back a bit, the way I was doing the wf check wasn't right at all. This just has the refactor, but even that isn't quite right - it causes two ICEs, but it's nearly there.

davidtwco (Nov 16 2018 at 22:02, on Zulip):

I had it sort-of working for the UserTypeAnnotation::TypeOf variant, but not the other variant.

davidtwco (Nov 16 2018 at 22:03, on Zulip):

And the other variant happened to be what was generated for the test case from the issue.

davidtwco (Nov 16 2018 at 22:41, on Zulip):

Pushed another change, still only the refactor, but no error. It's not ideal because there's two IndexVecs now in the TypeChecker.

nikomatsakis (Nov 20 2018 at 19:15, on Zulip):

@davidtwco reviewing now sorry for the delay

nikomatsakis (Nov 20 2018 at 19:15, on Zulip):

looks like the mir-opt tests need updating though

davidtwco (Nov 20 2018 at 19:16, on Zulip):

I've not made any changes since Friday, was hoping to get to it tonight.

davidtwco (Nov 20 2018 at 19:17, on Zulip):

I'm not all that happy with the way it is looking. I dislike that I'm using two maps for each variant of UserTypeAnnotation (since I need the substs from the UserTypeAnnotation::TypeOf variant - I found that if I still use the instantiated_types map for that branch then it ICEs, I had to construct and normalize the type where it currently happens).

davidtwco (Nov 20 2018 at 19:19, on Zulip):

w/r/t checking the well-formedness: Is that only the UserTypeAnnotation::TypeOf branch? If it is, that doesn't cover the case mentioned in the issue since it is the other variant. I had a working branch where the user_self_ty well-formedness check was moved and happening outside of the existing path as we want but it didn't cover the test case. I've not worked out what check I need to do for the other variant in UserTypeAnnotation.

davidtwco (Nov 20 2018 at 19:20, on Zulip):

My memory might be a bit hazy on the details of above since I haven't looked at it for a few days.

nikomatsakis (Nov 20 2018 at 19:24, on Zulip):

heh I thought it was nice :)

nikomatsakis (Nov 20 2018 at 19:24, on Zulip):

maybe I overlooked something

nikomatsakis (Nov 20 2018 at 19:24, on Zulip):

I actually realize now I didn't look to see if you enforce the WF rules

nikomatsakis (Nov 20 2018 at 19:24, on Zulip):

anyway I guess the WF rules apply to both branches; we just need to do keep the def-id and apply the substitution to the type_of(...)

davidtwco (Nov 20 2018 at 19:25, on Zulip):

I'm not doing that yet. I had it in for the UserTypeAnnotation::TypeOf case but it wasn't great IMO.

nikomatsakis (Nov 20 2018 at 19:25, on Zulip):

re: the UserTypeAnnotation

nikomatsakis (Nov 20 2018 at 19:25, on Zulip):

right now it has this form

nikomatsakis (Nov 20 2018 at 19:25, on Zulip):
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum UserTypeAnnotation<'tcx> {
    Ty(CanonicalTy<'tcx>),

    /// The canonical type is the result of `type_of(def_id)` with the
    /// given substitutions applied.
    TypeOf(DefId, CanonicalUserSubsts<'tcx>),
}
nikomatsakis (Nov 20 2018 at 19:25, on Zulip):

but maybe we should change it to

nikomatsakis (Nov 20 2018 at 19:26, on Zulip):
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum UserTypeAnnotation<'tcx> {
    Ty(Ty<'tcx>),

    /// The canonical type is the result of `type_of(def_id)` with the
    /// given substitutions applied.
    TypeOf(DefId, UserSubsts<'tcx>),
}

and move the Canonical out, so we store a Canonical<UserTypeAnnotation<'tcx>>

nikomatsakis (Nov 20 2018 at 19:26, on Zulip):

then you could have instantiated_user_types: IndexVec<UserTypeAnnotationIndex, UserTypeAnnotation<'tcx>>

nikomatsakis (Nov 20 2018 at 19:26, on Zulip):

(in the type checker)

nikomatsakis (Nov 20 2018 at 19:26, on Zulip):

thoughts?

davidtwco (Nov 20 2018 at 19:27, on Zulip):

Am I right that it is this part of the well-formedness check that we want to extract out to happen w/out the statement?

davidtwco (Nov 20 2018 at 19:27, on Zulip):

Yeah, I could try doing that.

davidtwco (Nov 20 2018 at 19:27, on Zulip):

(that link is only for the UserTypeAnnotation::TypeOf case)

nikomatsakis (Nov 20 2018 at 19:27, on Zulip):

yes, that looks like the right part

davidtwco (Nov 20 2018 at 19:28, on Zulip):

That's what you had linked before, wasn't sure if it was just that.

davidtwco (Nov 20 2018 at 19:28, on Zulip):

I'd been experimenting with just that part so that's fine.

davidtwco (Nov 21 2018 at 19:33, on Zulip):

@nikomatsakis I'm trying out that change you suggested yesterday to change UserTypeAnnotation<'tcx> with canonicalized types in it to Canonical<'tcx, UserTypeAnnotation<'tcx>> with the normal types in it. I've got that compiling and running, but I get a few ICEs that I'm not quite sure about. It definitely makes some of the code changes to type check related to this issue easier.

To do that, I'm needing to modify where the Ty<'tcx> and UserSubsts<'tcx> were initially being canonicalized in librustc_typeck. Instead of canonicalizing a UserSubsts<'tcx> and storing that so that it can later be put into a UserTypeAnnotation<'tcx>, I'm now making a UserTypeAnnotation<'tcx> at that point and canonicalizing that so it can later be used when building the Mir. I'm not thrilled that I'm needing to use that rustc::mir type in rustc_typeck because I figured we'd want to keep a separation there, but I don't know how else to get around that.

Anyway, this is mostly working - I've got it compiling with around 100 or so tests failing due to an ICE. I get this logging on a build without the change applied:

DEBUG 2018-11-21T00:36:33Z: rustc_typeck::check: write_user_substs_from_substs(HirId { owner: DefIndex(0:15), local_id: ItemLocalId(12) }, [_]) in fcx 0x7fff8ef5e150
DEBUG 2018-11-21T00:36:33Z: rustc_typeck::check: instantiate_value_path: user_substs = Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General) }], value: UserSubsts { substs: [^0], user_self_ty: None } }
DEBUG 2018-11-21T00:36:33Z: rustc_typeck::check: write_user_substs(HirId { owner: DefIndex(0:15), local_id: ItemLocalId(12) }, Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General) }], value: UserSubsts { substs: [^0], user_self_ty: None } }) in fcx 0x7fff8ef5e150
DEBUG 2018-11-21T00:36:33Z: rustc_typeck::check: write_user_substs: skipping identity substs

So that logging is with a UserSubsts<'tcx> being canonicalized. When I canonicalize the whole UserTypeAnnotation<'tcx>, I get this logging:

DEBUG 2018-11-21T00:54:55Z: rustc_typeck::check: write_user_type_annotation_from_substs: hir_id=HirId { owner: DefIndex(0:15), local_id: ItemLocalId(12) } def_id=DefId(2/1:13146 ~ core[420c]::option[0]::Option[0]::Some[0]) substs=[_] user_self_ty=None in fcx 0x7ffda113afe0
DEBUG 2018-11-21T00:54:55Z: rustc_typeck::check: write_user_type_annotation_from_substs: canonicalized=Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General) }], value: TypeOf(DefId(2/1:13146 ~ core[420c]::option[0]::Option[0]::Some[0]), UserSubsts { substs: [?0], user_self_ty: None }) }
DEBUG 2018-11-21T00:54:55Z: rustc_typeck::check: write_user_type_annotation: hir_id=HirId { owner: DefIndex(0:15), local_id: ItemLocalId(12) } canonical_user_type_annotation=Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General) }], value: TypeOf(DefId(2/1:13146 ~ core[420c]::option[0]::Option[0]::Some[0]), UserSubsts { substs: [?0], user_self_ty: None }) } tag=0x7ffda113afe0

I'm going on the perhaps naive assumption that I can just take the types that were being canoncalized, add them as fields to a type and canonicalize that and it works the same. Either way, the is_identity function is now returning true (due to the change from ^0 to ?0 - though I don't know what these represent) and that annotation isn't being skipped as it should be.

nikomatsakis (Nov 21 2018 at 19:56, on Zulip):

@davidtwco

I'm not thrilled that I'm needing to use that rustc::mir type in rustc_typeck because I figured we'd want to keep a separation there, but I don't know how else to get around that.

That type is not particularly tied to MIR. It would be OK, I think, to move it so it lives alongside TypeckTables

nikomatsakis (Nov 21 2018 at 19:56, on Zulip):

what's the ICE you are getting exactly?

davidtwco (Nov 21 2018 at 20:00, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/mir/tcx.rs#L73

davidtwco (Nov 21 2018 at 20:00, on Zulip):

It comes from that line.

davidtwco (Nov 21 2018 at 20:01, on Zulip):

Due to the type you’re seeing in the logs.

davidtwco (Nov 21 2018 at 20:01, on Zulip):

(on phone just now so have limited typing ability)

davidtwco (Nov 21 2018 at 20:18, on Zulip):

Oops. Not that line, this one: https://github.com/rust-lang/rust/blob/master/src/librustc/mir/tcx.rs#L149

davidtwco (Nov 21 2018 at 22:49, on Zulip):

That type is not particularly tied to MIR. It would be OK, I think, to move it so it lives alongside TypeckTables

I've changed this, much better now.

davidtwco (Nov 21 2018 at 22:59, on Zulip):

Also managed to fix that above ICE.

davidtwco (Nov 22 2018 at 19:38, on Zulip):

@nikomatsakis Alright, I've finally got something that's compiling and passing tests here. I'm not seeing a massive effect to the tests (including the test case from the linked issue), but I think this does what it is supposed to. PR is updated.

davidtwco (Nov 22 2018 at 19:44, on Zulip):

Still need to rebase, but doing that now.

davidtwco (Nov 23 2018 at 00:29, on Zulip):

Huh, didn't notice that failure locally. Will take a look tomorrow.

davidtwco (Nov 23 2018 at 10:51, on Zulip):

Should have fixed those - was seeing some odd ICE in run-pass locally but wasn't sure if it was just my machine so pushed what I have.

davidtwco (Nov 23 2018 at 12:18, on Zulip):

Looks like it wasn't just local :sad:

davidtwco (Nov 26 2018 at 13:47, on Zulip):

Made a decent amount of progress on this PR over the weekend. Added fixes for two other type annotation issues. Still dealing with one (and hopefully the last) ICE in it that affects three run-pass tests. I know which line is causing it, but removing that removes a error that it introduced on another test case that seemed correct - not looked into it that much yet.

davidtwco (Nov 26 2018 at 16:17, on Zulip):

Struggling to work out what is expected for this sort of case, got the log output and backtrace here.

nikomatsakis (Nov 26 2018 at 20:11, on Zulip):

@davidtwco hey so I'm looking at the PR now

nikomatsakis (Nov 26 2018 at 20:18, on Zulip):

much bigger PR than I anticipated :)

davidtwco (Nov 26 2018 at 20:41, on Zulip):

Yeah, it got big.

davidtwco (Nov 26 2018 at 20:42, on Zulip):

Mostly the refactor.

davidtwco (Nov 28 2018 at 21:46, on Zulip):

@nikomatsakis I’d appreciate if you could take a look at this if you have time, it’s not a priority though. I’ve been finding this ICE relatively impenetrable - not been able to narrow down the root cause of it.

davidtwco (Dec 09 2018 at 02:14, on Zulip):

@nikomatsakis I think I've now fixed that ICE - I experimented with a bunch of solutions but in the end I had to move some of the well-formedness checking back to only reachable code. That's not ideal but it's the best I could get. That said, the well-formedness check that had to be moved was one that I don't think we were doing at all before, so it's still an improvement.

(I spent like three hours on an alternate solution that worked for the tests that were failing, was very pleased when I finally got something that worked, until I made that test's code unreachable and my solution didn't work for it)

davidtwco (Dec 09 2018 at 02:14, on Zulip):

It passes tests locally but it's 2am and I need sleep, so there might be something Travis catches that I didn't see.

nikomatsakis (Dec 11 2018 at 20:23, on Zulip):

@davidtwco can you say a bit more about the ICE?

nikomatsakis (Dec 11 2018 at 20:23, on Zulip):

It seems like it came from unresolved type variables?

davidtwco (Dec 11 2018 at 20:27, on Zulip):

Yeah, it might be visible in the hidden Travis error GitHub comments @nikomatsakis

nikomatsakis (Dec 11 2018 at 20:27, on Zulip):

I feel like I thought that wouldn't happen but now I'm not sure why I thought that

nikomatsakis (Dec 11 2018 at 20:27, on Zulip):

I'm trying to find the test

nikomatsakis (Dec 11 2018 at 20:27, on Zulip):

comment says issue-54943-1.rs

nikomatsakis (Dec 11 2018 at 20:28, on Zulip):

oh

nikomatsakis (Dec 11 2018 at 20:28, on Zulip):

that is something added in your PR :)

nikomatsakis (Dec 11 2018 at 20:28, on Zulip):

no wonder I can't find it

davidtwco (Dec 11 2018 at 20:28, on Zulip):

Yeah, there were three dropck run-pass tests.

davidtwco (Dec 11 2018 at 20:28, on Zulip):

I made them smaller into issue-54943-1.rs and issue-54943-2.rs.

davidtwco (Dec 11 2018 at 20:29, on Zulip):

I had a fix that solved the issue unless the same code was unreachable.

davidtwco (Dec 11 2018 at 20:29, on Zulip):

Hence the two tests.

nikomatsakis (Dec 11 2018 at 20:29, on Zulip):

I can see why the ICE is occurring

nikomatsakis (Dec 11 2018 at 20:29, on Zulip):

I'm not sure why I thought this wouldn't happen

nikomatsakis (Dec 11 2018 at 20:29, on Zulip):

it seems obvious that it would

davidtwco (Dec 11 2018 at 20:31, on Zulip):
fn main() {
    struct A<'a, B: 'a>(&'a B);
    let (a1, a2): (String, A<_>) = (String::from("auto"), A(&"this"));
    foo((a1, a2));
}

would cause the issue, but

fn main() {
    struct A<'a, B: 'a>(&'a B);
    let (a1,): (A<_>,) = (A(&"this"),);
    foo((a1,));
}

wouldn't - comparing both logs I worked it out to be a certain map that didn't contain something - I'm slightly fuzzy on exactly what map.

nikomatsakis (Dec 11 2018 at 20:33, on Zulip):

I'm not convinced the other case is immune from this problem

nikomatsakis (Dec 11 2018 at 20:33, on Zulip):

it seems like we would maybe need to remember the type we inferred to solve it

davidtwco (Dec 11 2018 at 20:34, on Zulip):

The difference was this line.

davidtwco (Dec 11 2018 at 20:34, on Zulip):

In the successful case it took the Ok(..) branch.

davidtwco (Dec 11 2018 at 20:35, on Zulip):

I wasn't able to track it down further.

davidtwco (Dec 11 2018 at 20:37, on Zulip):

I eventually tried applying the projections into the type annotation and saving those into the TypeChecker map that I had (requiring a whole bunch of refactoring so that we stored the UserTypeProjection in the MIR, not the UserTypeAnnotation - that meant that projections didn't need to be applied in the code that gets run for the AscribeUserType statement and it fixed the issue unless the code was unreachable.

nikomatsakis (Dec 11 2018 at 20:37, on Zulip):

I guess what I have to do is to checkout and build your branch

nikomatsakis (Dec 11 2018 at 20:37, on Zulip):

it's gotten large enough I can't really peruse it effectively on GH :)

davidtwco (Dec 11 2018 at 20:38, on Zulip):

I find it isn't too awful if you go commit-by-commit, but all at once it's hard for me to work out.

davidtwco (Dec 11 2018 at 20:38, on Zulip):

I deliberately didn't include the fix to the ICE in the commit that introduced it like I normally would have, instead just adding it on so that it could be popped off and the ICE could be observed again.

davidtwco (Dec 12 2018 at 12:46, on Zulip):

I eventually tried applying the projections into the type annotation and saving those into the TypeChecker map that I had (requiring a whole bunch of refactoring so that we stored the UserTypeProjection in the MIR, not the UserTypeAnnotation - that meant that projections didn't need to be applied in the code that gets run for the AscribeUserType statement and it fixed the issue unless the code was unreachable.

Remembered this morning that I saved the diff of this change. Don't think there's anything useful in there, but it took a while so I kept it around just in case.

nikomatsakis (Dec 12 2018 at 17:52, on Zulip):

ok @davidtwco so I spent some time reading into the ICE etc

nikomatsakis (Dec 12 2018 at 17:52, on Zulip):

and in general playing with the branch

nikomatsakis (Dec 12 2018 at 17:52, on Zulip):

I was particularly interested in this example:

nikomatsakis (Dec 12 2018 at 17:52, on Zulip):
fn main() {
    struct A<'a, B: 'a>(&'a B);
    let (a1, a2): (String, A<_>) = (String::from("auto"), A(&"this"));
    foo((a1, a2));
}
nikomatsakis (Dec 12 2018 at 17:52, on Zulip):

which is issue-54943-1.rs

nikomatsakis (Dec 12 2018 at 17:52, on Zulip):

I think the problem in this specific case is that we are creating too many CanonicalUserTypeAnnotation entries

nikomatsakis (Dec 12 2018 at 17:53, on Zulip):

in particular, we wind up with kind of two distinct copies of the (String, A<_>) annotation, one used when projecting out String, and one used when projecting out the A<_>

nikomatsakis (Dec 12 2018 at 17:53, on Zulip):

the result is that -- when we instantiate -- we have distinct type variables for those two cases, and the A<_> type variable winds up unconstrained

nikomatsakis (Dec 12 2018 at 17:53, on Zulip):

I feel like there should be only be one index in this case, used with two distinct projections,

nikomatsakis (Dec 12 2018 at 17:54, on Zulip):

but it's not entirely obvious how to modify the source code to make that happen.

nikomatsakis (Dec 12 2018 at 17:54, on Zulip):

But also, I'm a bit worried about a separate problem.

nikomatsakis (Dec 12 2018 at 17:55, on Zulip):

which has to do with issue-54943-2.rs -- in that case, I imagine that the type variable would simply never be constrained

nikomatsakis (Dec 12 2018 at 17:55, on Zulip):

(that is, the _ in A<_>)

nikomatsakis (Dec 12 2018 at 17:55, on Zulip):

because those constraints arise from type-checking the let, and that code is dead... unless, perhaps we would still see it because we've not stripped dead code?

nikomatsakis (Dec 12 2018 at 17:55, on Zulip):

(I can't recall)

nikomatsakis (Dec 12 2018 at 17:57, on Zulip):

(nope, we definitely strip dead code)

nikomatsakis (Dec 12 2018 at 18:04, on Zulip):

anyway, so that seems like a bit of a difficult challenge that for some reason I had not anticipated. I'm not sure what I expected us to do here, maybe retain some information (which we could do) from typeck that tells us what type (modulo regions) we inferred those variables to?

nikomatsakis (Dec 12 2018 at 18:04, on Zulip):

i'll stop here for now :)

davidtwco (Dec 12 2018 at 18:17, on Zulip):

That explains what I observed happening when experimenting with those tests.

davidtwco (Dec 12 2018 at 18:21, on Zulip):

In particular with the attempt I mentioned yesterday (and linked to a gist of earlier) where by applying the projections to the (String, A<_>) before checking well-formedness fixed the issue - but only when the source of the type annotation wasn’t dead code.

davidtwco (Dec 12 2018 at 18:25, on Zulip):

I’m not sure how you’d work around the issue because, as I understand it, the (String, A<_>) annotation’s well-formedness check depends on the inference variable for the _ having been related to the &str type when the associated AscribeUserType or local decl processing happened previously? I haven’t been able to work out a way to do that when it is in dead code.

davidtwco (Dec 12 2018 at 18:26, on Zulip):

(That’s the half-baked understanding I was left with having dug into the compiler as far as that canonicalizer code and the ty_probe function I linked yesterday)

nikomatsakis (Dec 12 2018 at 20:13, on Zulip):

Yes. I'm sort of leaning towards "won't fix" here, tbqh, but I'm torn about it still. I think the best fix would have to involve preserving -- for each user type -- not only the user-given type (as now) but also the "fully inferred" variant of it, so that we can unify those. Right now, that fully inferred (modulo regions) type comes from the type-check, but that doesn't apply to dead code, hence the problem.

pnkfelix (Dec 12 2018 at 20:21, on Zulip):

the result is that -- when we instantiate -- we have distinct type variables for those two cases, and the A<_> type variable winds up unconstrained

Fixing this might be another path that could help with #55748

nikomatsakis (Dec 12 2018 at 20:21, on Zulip):

interesting

pnkfelix (Dec 12 2018 at 20:22, on Zulip):

or at least, it roughly matches up with one of my theories when I was reviewing the RUST_LOG output

pnkfelix (Dec 12 2018 at 20:22, on Zulip):

trying to make sense of what was happening

pnkfelix (Dec 12 2018 at 20:22, on Zulip):

in that it seemed like we might be creating two separate sets of fresh type inference variables

pnkfelix (Dec 12 2018 at 20:22, on Zulip):

for two separate projections out of the same type annotation

nikomatsakis (Dec 12 2018 at 20:22, on Zulip):

that makes sense

nikomatsakis (Dec 12 2018 at 20:23, on Zulip):

it does feel like there should be a "connection"

nikomatsakis (Dec 12 2018 at 20:23, on Zulip):

looking at the code, it felt like it wouldn't be entirely trivial to create that; mostly, we have to somehow carry the identify forward

pnkfelix (Dec 12 2018 at 20:23, on Zulip):

but ohI think we should focus on the meeting now. :)

nikomatsakis (Dec 12 2018 at 20:23, on Zulip):

in a few minutes I guess

pnkfelix (Dec 12 2018 at 20:23, on Zulip):

or at least in 7 minutes, yeah

pnkfelix (Dec 12 2018 at 20:24, on Zulip):

my phone alarm went off so I thought I was already late. :)

nikomatsakis (Dec 12 2018 at 20:37, on Zulip):

btw I think this exact question is the reason I used Invariant here

davidtwco (Dec 13 2018 at 21:51, on Zulip):

So how do we want to fix the issue here? We could consider it a follow up and land this PR with the well-formedness check of the UserTypeAnnotation::Ty(..) only happening in reachable code and the other case happening in both.

nikomatsakis (Dec 13 2018 at 22:05, on Zulip):

A good question

nikomatsakis (Dec 13 2018 at 22:05, on Zulip):

I'm not super sure

nikomatsakis (Dec 13 2018 at 22:05, on Zulip):

obviously we need to land the PR, it's got a lot of great stuff in it

nikomatsakis (Dec 13 2018 at 22:05, on Zulip):

we could move everything over to reachable code for now, to at least get consistency

davidtwco (Dec 13 2018 at 22:06, on Zulip):

I don't know that it being consistent is better than it covering more cases. IIRC it still takes the same code path to check well-formedness, the call site for each case is just different.

davidtwco (Dec 13 2018 at 22:07, on Zulip):

Previously we only checked the UserTypeAnnotation::TypeOf case and the whole issue in the first place was supporting unreachable code for that.

nikomatsakis (Dec 13 2018 at 22:20, on Zulip):

I don't really see how the two cases are different per se, but perhaps you are right. It seems better to report more errors. I guess I just remain sort of ambivalent about whether we should report "lifetime errors" in dead code.

pnkfelix (Dec 18 2018 at 13:37, on Zulip):

I would like to land this PR (#55937) as is. I agree it has lots of great stuff in it, and I would like to have it already landed before I land anything for #55748

davidtwco (Dec 18 2018 at 13:40, on Zulip):

I think it's in a state where it could land with a follow-up to fix the underlying cause of the ICE that the last commit fixed.

pnkfelix (Dec 18 2018 at 13:40, on Zulip):

I agree with that assessment. The only reason why I haven't r+'ed already is that I'll be meeting with @nikomatsakis in about 20 minutes anyway, so I figure I'll give them a chance to weigh in if he so chooses.

nikomatsakis (Dec 18 2018 at 17:38, on Zulip):

I told @pnkfelix that I'd like to take one more look. I'm doing so now.

davidtwco (Dec 18 2018 at 17:39, on Zulip):

Just pushed a rebase for the conflict that happened.

nikomatsakis (Dec 18 2018 at 17:54, on Zulip):

ok, so, I think I figured out why I was not seeing the errors I expected, but I also don't think that this PR is quite fixing the bug it claims to fix

nikomatsakis (Dec 18 2018 at 17:54, on Zulip):

though it does fix some other bugs

nikomatsakis (Dec 18 2018 at 17:55, on Zulip):

in particular, the test issue-54943.rs does not include #![feature(nll)]:

fn foo<T: 'static>() { }

fn main<'a>() {
    return;

    let x = foo::<&'a u32>();
    //~^ ERROR the type `&'a u32` does not fulfill the required lifetime [E0477]
}
nikomatsakis (Dec 18 2018 at 17:55, on Zulip):

if you run with -Zborrowck=mir, the test compiles

nikomatsakis (Dec 18 2018 at 17:55, on Zulip):

well, almost:

warning: unreachable statement
  --> issue-54943.rs:16:5
   |
16 |     let x = foo::<&'a u32>();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unreachable_code)] on by default

error[E0131]: `main` function is not allowed to have generic parameters
  --> issue-54943.rs:13:8
   |
13 | fn main<'a>() {
   |        ^^^^ `main` cannot have generic parameters
nikomatsakis (Dec 18 2018 at 17:56, on Zulip):

(if I rename main to bar, that error goes away)

nikomatsakis (Dec 18 2018 at 17:56, on Zulip):

anyway I think I know why that is happening

nikomatsakis (Dec 18 2018 at 17:57, on Zulip):

the code is creating the TyFnDef for foo::<&'a u32> and is proving the predicate WF(foo::<&'a u32>), but the code that computes the WF conditions for such a type is rather flawed

nikomatsakis (Dec 18 2018 at 17:57, on Zulip):

and in fact proving that predicate is a no-op

nikomatsakis (Dec 18 2018 at 17:57, on Zulip):

if you run with -Zborrowck=mir, the test compiles

(the error you are seeing comes from the older, lexical check)

nikomatsakis (Dec 18 2018 at 17:58, on Zulip):

this explains why e.g. this test (which I created as issue-54943-3.rs) compiles with no error:

use std::fmt::Debug;

fn foo<T: 'static + Debug>(_: T) { }

fn bar<'a>() {
    return;

    let _x = foo::<Vec<_>>(Vec::<&'a u32>::new());
    //~^ ERROR the type `&'a u32` does not fulfill the required lifetime [E0477]
}

...and also no ICE.

nikomatsakis (Dec 18 2018 at 18:01, on Zulip):

I expected that test to ICE because the _ in Vec<_> would be unconstrained, and we would never be able to prove that Vec<_>: Debug

nikomatsakis (Dec 18 2018 at 18:01, on Zulip):

(but it turns out we don't even try)

nikomatsakis (Dec 18 2018 at 18:02, on Zulip):

(I'm trying to decide what I think we ought to do, in any case)

nikomatsakis (Dec 18 2018 at 18:02, on Zulip):

I would definitely like to land this PR or most of it, it seems to be in a good direction

davidtwco (Dec 18 2018 at 18:02, on Zulip):

If we don't try it might be the UserTypeAnnotation::Ty(..) case.

nikomatsakis (Dec 18 2018 at 18:05, on Zulip):

no

nikomatsakis (Dec 18 2018 at 18:05, on Zulip):

the problem is this code here:

                ty::FnDef(..) | ty::FnPtr(_) => {
                    // let the loop iterate into the argument/return
                    // types appearing in the fn signature
                }
nikomatsakis (Dec 18 2018 at 18:05, on Zulip):

that is supposed to be returning the conditions for the type to be WF

nikomatsakis (Dec 18 2018 at 18:06, on Zulip):

for FnPtr, it is correct, but for FnDef, I think it is wrong

nikomatsakis (Dec 18 2018 at 18:06, on Zulip):

it should probably be treated the same as TyAdt

nikomatsakis (Dec 18 2018 at 18:06, on Zulip):

the result is that when we do this:

nikomatsakis (Dec 18 2018 at 18:07, on Zulip):
                self.prove_predicate(Predicate::WellFormed(ty));

in librustc_traits/type_op.rs (in the PR branch)

nikomatsakis (Dec 18 2018 at 18:07, on Zulip):

it winds up being basically a no-op

nikomatsakis (Dec 18 2018 at 18:07, on Zulip):

hmm although

nikomatsakis (Dec 18 2018 at 18:08, on Zulip):

let me double check a few things ;)

nikomatsakis (Dec 18 2018 at 18:08, on Zulip):

I still think that code is wrong but there is something I don't quite yet understand

nikomatsakis (Dec 18 2018 at 18:17, on Zulip):

Oh, hm, interesting. The type-op code never actually checks whether the type-ops result in a "certainty: Proven" result, which I believe is the expected thing. So actually we do create the conditions I thought would lead to an ICE, but I guess we're not checking the result that I thought we were.

nikomatsakis (Dec 18 2018 at 18:17, on Zulip):

to clarify: in my example above that I expected to ICE, what happens is that we go to prove WF(foo::<Vec<_>> or whatever -- this requires us to prove WF(Vec<_>), which we fail to do, and so we give back an ambiguous result

nikomatsakis (Dec 18 2018 at 18:18, on Zulip):

I .. think that should probably be an ICE, we're not supposed to have unresolved type variables like that in the region queries

nikomatsakis (Dec 18 2018 at 18:18, on Zulip):

I'm not entirely sure why the other examples do result in ICEs now, I have to go back and re-check that

nikomatsakis (Dec 18 2018 at 18:20, on Zulip):

(I'm a bit afraid to add in the asseritons I think we should have...)

davidtwco (Dec 18 2018 at 18:23, on Zulip):

Changing the ty::FnDef case that you mentioned previously makes issue-54943.rs fail.

davidtwco (Dec 18 2018 at 18:23, on Zulip):

(that is, fail to compile, as expected, not fail to do what we want)

davidtwco (Dec 18 2018 at 18:24, on Zulip):

It also makes the issue-54943-3.rs you mention above ICE.

davidtwco (Dec 18 2018 at 18:25, on Zulip):

unresolved inference variable in outlives: _#0t as before

nikomatsakis (Dec 18 2018 at 18:33, on Zulip):

ah ok yes that is what I expect

nikomatsakis (Dec 18 2018 at 18:33, on Zulip):

you altered wf.rs?

nikomatsakis (Dec 18 2018 at 18:33, on Zulip):

does it affect any other tests?

davidtwco (Dec 18 2018 at 18:34, on Zulip):

None of the issue-54943-* tests, let me run the rest.

nikomatsakis (Dec 18 2018 at 18:34, on Zulip):

I meant the whole test suite , yeah

davidtwco (Dec 18 2018 at 18:36, on Zulip):

Two new errors on these tests w/out compare mode NLL:

    [ui] ui/nll/ty-outlives/projection-where-clause-env-wrong-bound.rs
    [ui] ui/nll/ty-outlives/projection-where-clause-none.rs
davidtwco (Dec 18 2018 at 18:36, on Zulip):
---- [ui] ui/nll/ty-outlives/projection-where-clause-none.rs stdout ----
diff of stderr:

+       error[E0309]: the associated type `<T as MyTrait<'a>>::Output` may not live long enough
+         --> $DIR/projection-where-clause-none.rs:16:5
+          |
+       LL |     bar::<T::Output>() //~ ERROR the parameter type `T` may not live long enough
+          |     ^^^^^^^^^^^^^^^^
+          |
+          = help: consider adding an explicit lifetime bound `<T as MyTrait<'a>>::Output: 'a`...
+
1       error[E0309]: the parameter type `T` may not live long enough
2         --> $DIR/projection-where-clause-none.rs:16:5
3          |

6          |
7          = help: consider adding an explicit lifetime bound `T: 'a`...
8
-       error: aborting due to previous error
+       error: aborting due to 2 previous errors
10
11      For more information about this error, try `rustc --explain E0309`.
12
davidtwco (Dec 18 2018 at 18:36, on Zulip):

Same w/ compare-mode NLL.

nikomatsakis (Dec 18 2018 at 18:38, on Zulip):

ok so we get some double errors

nikomatsakis (Dec 18 2018 at 18:38, on Zulip):

but not new errors

nikomatsakis (Dec 18 2018 at 18:38, on Zulip):

that's not surprising, I think we have some code that is basically doing repeating same checks twice now

davidtwco (Dec 18 2018 at 18:42, on Zulip):

Didn't affect any run-pass tests.

nikomatsakis (Dec 18 2018 at 18:43, on Zulip):

specifically I think that these checks are kind of duplicated by the WF check now

nikomatsakis (Dec 18 2018 at 18:43, on Zulip):

but I'm not 100% sure

davidtwco (Dec 18 2018 at 18:44, on Zulip):

Any thoughts on the ICEs?

nikomatsakis (Dec 18 2018 at 19:26, on Zulip):

sorry, I was on a call

nikomatsakis (Dec 18 2018 at 19:26, on Zulip):

well, my thoughts are that we should (for now) try to move towards checking only reachable constraints

nikomatsakis (Dec 18 2018 at 19:26, on Zulip):

but I think that the "right fix" is probably to have typeck produce a (region-erased) "instantiated version" of those annotations

nikomatsakis (Dec 18 2018 at 19:27, on Zulip):

i.e., the user-type-annotations which are -- right now -- canonicalized, would also have to have the "inferred" form

davidtwco (Dec 18 2018 at 19:27, on Zulip):

So should I remove the query for well formed type annotations and just move what it does back into the original query?

nikomatsakis (Dec 18 2018 at 19:27, on Zulip):

probably for now yes

nikomatsakis (Dec 18 2018 at 19:27, on Zulip):

(hmm)

nikomatsakis (Dec 18 2018 at 19:27, on Zulip):

I am trying to decide what the ultimate architeture would be

nikomatsakis (Dec 18 2018 at 19:28, on Zulip):

why exactly did we make a query in the first place?

davidtwco (Dec 18 2018 at 19:28, on Zulip):

I needed access to a certain kind of context and I felt it'd be easier to get that the same way the existing query did.

davidtwco (Dec 18 2018 at 19:29, on Zulip):

Well, I could just copy the existing well-formedness check for one of the cases but I needed the helper functions it had, so it had to be a query to get those.

davidtwco (Dec 18 2018 at 19:29, on Zulip):

Made it a tad easier.

davidtwco (Dec 18 2018 at 20:23, on Zulip):

I've now got a build that has the refactoring, other two fixes but doesn't actually perform well-formedness checks on unreachable code.

davidtwco (Dec 18 2018 at 20:23, on Zulip):

Only boring old reachable code.

davidtwco (Dec 18 2018 at 20:24, on Zulip):

If that's the route we want to take then I can push it and stop the PR from closing #54943.

nikomatsakis (Dec 18 2018 at 20:27, on Zulip):

seems like a solid step forward

nikomatsakis (Dec 18 2018 at 20:27, on Zulip):

and we can then discuss the next step for unreachable code

davidtwco (Dec 18 2018 at 20:30, on Zulip):

Alright, PR updated.

davidtwco (Dec 18 2018 at 22:04, on Zulip):

Don’t worry if you don’t, but do you think you’ll have time to jot down some ideas for fixing the unreachable type annotations before the holidays @nikomatsakis? I’d like to be able to get a start on that during mine.

davidtwco (Dec 18 2018 at 22:11, on Zulip):

Responded to the review comments.

nikomatsakis (Dec 18 2018 at 22:12, on Zulip):

@davidtwco will try to do so asap

nikomatsakis (Dec 18 2018 at 22:12, on Zulip):

I expect I can do that before the holidays, yes

davidtwco (Dec 18 2018 at 22:12, on Zulip):

Thanks, I appreciate it.

nikomatsakis (Dec 18 2018 at 22:23, on Zulip):

@davidtwco re: the ignored tests, as a rule, I think ignored tests should never be committed

nikomatsakis (Dec 18 2018 at 22:23, on Zulip):

basically because it's very easy to forget that they are ignored

nikomatsakis (Dec 18 2018 at 22:24, on Zulip):

whereas if you edit the annotations to document the current behavior

nikomatsakis (Dec 18 2018 at 22:24, on Zulip):

then when you fix it, you see that (as test failures :)

davidtwco (Dec 18 2018 at 22:33, on Zulip):

That’s fair. In this case I think since we’ll be following up pretty quickly it’s probably fine? If you want I can quickly change them though.

nikomatsakis (Dec 18 2018 at 22:34, on Zulip):

I'd prefer not to ignore them personally

nikomatsakis (Dec 18 2018 at 22:34, on Zulip):

(but I agree not that big a deal)

nikomatsakis (Dec 18 2018 at 22:34, on Zulip):

so if it's annoying for you to remove that maybe i'll just r+ :)

davidtwco (Dec 18 2018 at 22:36, on Zulip):

Give me a second and I’ll change them.

davidtwco (Dec 18 2018 at 22:40, on Zulip):

@nikomatsakis Fixed them.

pnkfelix (Dec 19 2018 at 14:23, on Zulip):

hey @davidtwco are you still around to answer Q's about this PR (#55937)?

davidtwco (Dec 19 2018 at 14:24, on Zulip):

Yeah, I'm around.

pnkfelix (Dec 19 2018 at 14:24, on Zulip):

so I've been playing with a checkout of it locally

pnkfelix (Dec 19 2018 at 14:24, on Zulip):

to see whether it helps me make a solution to #55748

pnkfelix (Dec 19 2018 at 14:24, on Zulip):

and I noticed someting

pnkfelix (Dec 19 2018 at 14:25, on Zulip):

the way you modified the fn user_ty on e.g. PatternTypeProjections

pnkfelix (Dec 19 2018 at 14:25, on Zulip):

takes a annotations: &mut CanonicalUserTypeAnnotations<'tcx>

pnkfelix (Dec 19 2018 at 14:26, on Zulip):

and then the user_ty in PatternTypeProjection (singular) pushes annotations on to it

pnkfelix (Dec 19 2018 at 14:27, on Zulip):

while the user_ty on PatternTypeProjections (plural) does a map over the .contents and calls user_ty on each

pnkfelix (Dec 19 2018 at 14:27, on Zulip):

this ends up meaning that we get distinct entries in the annotations for each projection

pnkfelix (Dec 19 2018 at 14:28, on Zulip):

but ... wouldn't it be more appropriate to have a single entry in the annotations, that we add at the outset, and then the projections into that annotation each reference the single entry that they share?

pnkfelix (Dec 19 2018 at 14:28, on Zulip):

(this obviously is a more invasive change the code. but I think it is ... "more correct" ...?)

davidtwco (Dec 19 2018 at 14:28, on Zulip):

Yeah, it would. Not sure why I did it that way, there wasn't any particular rhyme or reason behind it, just what I ended up doing while fixing the bunch of errors that would have been around during that refactoring.

pnkfelix (Dec 19 2018 at 14:29, on Zulip):

I suppose I can just try to make this change myself

davidtwco (Dec 19 2018 at 14:29, on Zulip):

In fact, I think that was a problem that @nikomatsakis had pointed out in relation to the ICEs we were seeing.

pnkfelix (Dec 19 2018 at 14:29, on Zulip):

on my check out of this branch

pnkfelix (Dec 19 2018 at 14:29, on Zulip):

hmm okay yes

davidtwco (Dec 19 2018 at 14:29, on Zulip):

I think it wouldn't be too bad to fix actually.

davidtwco (Dec 19 2018 at 14:30, on Zulip):

this message

pnkfelix (Dec 19 2018 at 14:30, on Zulip):

I think he and I briefly spoke on it yesterday (where I asserting that your PR did not suffer from this problem, because I was looking in the wrong place for the instance of the problem)

davidtwco (Dec 19 2018 at 14:31, on Zulip):

I think if we made that change, then asserted well-formedness for the UserTypeAnnotation::Ty(..) and UserTypeAnnotation::TypeOf(..) case then where that would normally break issue-54943-1.rs and issue-54943-2.rs, it would only break issue-54943-2.rs?

pnkfelix (Dec 19 2018 at 14:32, on Zulip):

/me looks

pnkfelix (Dec 19 2018 at 14:34, on Zulip):

it certainly seems like it would help in at least issue-54943-1.rs, if done properly.

davidtwco (Dec 19 2018 at 14:35, on Zulip):

Since the issue there was that we had some annotation over (String, &'XXX str) (or something like that, I forget exactly) and because there were two copies of the annotation, when the type relation happened because the code was reachable, you ended up with one copy that was (String, &'XXX str) (when looking at the first field) and one that was (String, &'a str) (when relating the second field) - and when doing well-formedness checking of that whole annotation later you ended up trying to work with &'XXX str and failing. Since there would now be only one annotation, both fields would be related and you'd only have (String, &'a str).

davidtwco (Dec 19 2018 at 14:35, on Zulip):

But they don't get related at all when unreachable, which is what issue-54943-2.rs shows.

pnkfelix (Dec 19 2018 at 14:35, on Zulip):

how are you thinking you'd go about doing this? Would you scan the annotations first to see if the given CanonicalUserTypeAnnotation was previously pushed onto it? Or are you thinking of a bigger re-architecting?

davidtwco (Dec 19 2018 at 14:36, on Zulip):

I had worked around this previously by having the projections indexed into an vec instead of the annotations, and then in the instantiate phase where I normally instantiate the canonicalized annotations now, I do that and apply the projections - that way when checking the well-formedness later, we were only doing so on that one field that would have been related - still failed on the unreachable case though.

davidtwco (Dec 19 2018 at 14:37, on Zulip):

I'm not too sure, I'd need to refresh myself on the code.

pnkfelix (Dec 19 2018 at 14:37, on Zulip):

okay. just curious

davidtwco (Dec 19 2018 at 14:37, on Zulip):

I had worked around this previously by having the projections indexed into an vec instead of the annotations, and then in the instantiate phase where I normally instantiate the canonicalized annotations now, I do that and apply the projections - that way when checking the well-formedness later, we were only doing so on that one field that would have been related - still failed on the unreachable case though.

diff for this experiment

davidtwco (Dec 19 2018 at 14:42, on Zulip):

how are you thinking you'd go about doing this? Would you scan the annotations first to see if the given CanonicalUserTypeAnnotation was previously pushed onto it? Or are you thinking of a bigger re-architecting?

Do you remember if a PatternTypeProjections only ever has the projections into a single annotation? Is it only ever Field(0), Field(1) and Field(2) into the same (X, Y, Z) or can it be into (X, Y), (X, Z) and (Z, Y, X) respectively?

pnkfelix (Dec 19 2018 at 14:43, on Zulip):

I'm pretty sure that the intent is that the base will be able to differ, at least in the long term

pnkfelix (Dec 19 2018 at 14:43, on Zulip):

because we want to be able to ascribe types on patterns themselves

pnkfelix (Dec 19 2018 at 14:43, on Zulip):

which means we'll get into scenarios like

pnkfelix (Dec 19 2018 at 14:44, on Zulip):

let (x: X, y): PairOfXandY;

davidtwco (Dec 19 2018 at 14:45, on Zulip):

I guess I'd probably just have worked out some sort of check to see whether an annotation already exists and if so just return the same id.

pnkfelix (Dec 19 2018 at 14:45, on Zulip):

and thus the accumulated state as we descend will (eventually) have distinct patterns that we are potentially projecting out of.

pnkfelix (Dec 19 2018 at 14:45, on Zulip):

I guess I'd probably just have worked out some sort of check to see whether an annotation already exists and if so just return the same id.

Yeah I'm going to try that now on my local checkout

pnkfelix (Dec 19 2018 at 14:45, on Zulip):

I'm not going to bother with a hashtable or anything. I don't expect this vector to grow much.

pnkfelix (Dec 19 2018 at 14:45, on Zulip):

(which may be a terrible assumption longer term, but for now... )

davidtwco (Dec 19 2018 at 14:47, on Zulip):

Alternatively, I don't mind having some duplication of annotations as we propagate them through to NLL and then applying the projection before it is used at all in NLL type check.

davidtwco (Dec 19 2018 at 14:47, on Zulip):

Or just as early as we can.

pnkfelix (Dec 19 2018 at 14:48, on Zulip):

well the reason why I want to try avoiding duplicating the annotations is that, in cases like the "potentially wrong-headed demo" from #55748, I think I need to ensure that we only have one annotation in hand when it gets its variables instantiated.

pnkfelix (Dec 19 2018 at 14:49, on Zulip):

Right now, because there are duplicate annotations for each projection, when I look at the types for y and _z in let (mut y, mut _z): Pair<&u32> = (s, x);, I do not think I have much hope of ensuring that their respective types actually use the same regions.

davidtwco (Dec 19 2018 at 14:50, on Zulip):

Ah, I see.

pnkfelix (Dec 19 2018 at 15:40, on Zulip):

yay: my modifications to your PR fix #55748

davidtwco (Dec 19 2018 at 15:41, on Zulip):

The deduplication of annotations?

davidtwco (Dec 19 2018 at 15:41, on Zulip):

Is it on a branch? I'd be interested in taking a look.

pnkfelix (Dec 19 2018 at 15:41, on Zulip):

Its not uploaded yet, but I'll push in a moment.

davidtwco (Dec 19 2018 at 15:42, on Zulip):

If you want you could push straight onto my branch so we don't need to wait two bors cycles for it to land.

pnkfelix (Dec 19 2018 at 15:52, on Zulip):

Well I think the best thing may be for you to look at the two commits in question and cherry-pick them over if you like them.

pnkfelix (Dec 19 2018 at 15:53, on Zulip):

I don't want the other change I made to get mixed in with yours unless you want it to be there.

davidtwco (Dec 19 2018 at 15:53, on Zulip):

Alright, sounds good.

pnkfelix (Dec 19 2018 at 15:54, on Zulip):

this is branch: https://github.com/pnkfelix/rust/commits/issue-54943-with-fix-for-55748

pnkfelix (Dec 19 2018 at 15:54, on Zulip):

I think it will build and still fix my problem. (Its a quick result of me removing a bunch of code that was just noise now that I found these smaller deltas...)

davidtwco (Dec 19 2018 at 15:55, on Zulip):

Is it just the first two commits?

pnkfelix (Dec 19 2018 at 15:55, on Zulip):

the two commits I'm suggesting you consider cherry picking are 194e25d and eaf0621

pnkfelix (Dec 19 2018 at 15:55, on Zulip):

yeah, just the first two

davidtwco (Dec 19 2018 at 15:56, on Zulip):

Yeah, I'll grab those, they look like they'd just be a straight improvement on what is currently there.

pnkfelix (Dec 19 2018 at 15:56, on Zulip):

okay great. and now I've just confirmed that that branch does indeed fix my demo.

davidtwco (Dec 19 2018 at 15:59, on Zulip):

Doing a quick UI test run with it locally and then I'll push them to my PR.

pnkfelix (Dec 19 2018 at 16:02, on Zulip):

note that it might ui/nll/user-annotations/pattern.rs, since one test in there is broken by my branch.

pnkfelix (Dec 19 2018 at 16:02, on Zulip):

but honestly I think that is due to my third commit that you are not cherry-picking.

davidtwco (Dec 19 2018 at 16:02, on Zulip):

I guess I'll be able to confirm that in a moment.

davidtwco (Dec 19 2018 at 16:09, on Zulip):

Had a failure in ui/dropck/dropck_trait_cycle_checked.rs but it was just the attribution of a error changing from a "cast" to a "type annotation" so it seems OK.

pnkfelix (Dec 19 2018 at 16:10, on Zulip):

interesting that I didn't see that on my end...

pnkfelix (Dec 19 2018 at 16:10, on Zulip):

was it from compare-mode=nll or something?

davidtwco (Dec 19 2018 at 16:10, on Zulip):

It was.

pnkfelix (Dec 19 2018 at 16:10, on Zulip):

ok i see

davidtwco (Dec 19 2018 at 16:13, on Zulip):

Anyway, updated the PR with those.

nikomatsakis (Dec 19 2018 at 20:20, on Zulip):

I noted a concern here: link

davidtwco (Dec 19 2018 at 20:30, on Zulip):

I tried this test case which is similar to what you mentioned in the issue, and it has this MIR.

davidtwco (Dec 19 2018 at 20:30, on Zulip):

cc @nikomatsakis

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

I guess this is because the span is included

nikomatsakis (Dec 19 2018 at 20:32, on Zulip):

doesn't feel "right" to me

nikomatsakis (Dec 19 2018 at 20:32, on Zulip):

but it may prevent there from being an actual bug

nikomatsakis (Dec 19 2018 at 20:32, on Zulip):

I am wondering if some kind of trickery involving macros can distinct annotations with the same span

davidtwco (Dec 19 2018 at 20:35, on Zulip):

@nikomatsakis test and mir?

nikomatsakis (Dec 19 2018 at 20:36, on Zulip):

that's roughly what I had in mind, yes :)

pnkfelix (Dec 19 2018 at 20:52, on Zulip):

okay so then that approach isn't quite right I guess...

nikomatsakis (Dec 19 2018 at 20:53, on Zulip):

I guess if we included a HirId or something it could work

pnkfelix (Dec 19 2018 at 20:53, on Zulip):

yeah I was just musing that tying it back to either the HIR or the HAIR would be necessary to deal with these scenarios

nikomatsakis (Dec 19 2018 at 20:54, on Zulip):

right, I had thought first about refactoring things more broadly, but perhaps adding a HirId would be a "good enough" hack

pnkfelix (Dec 19 2018 at 20:58, on Zulip):

I should try to make a variant of @davidtwco 's test that shows the bug without having to look at MIR

davidtwco (Dec 19 2018 at 20:58, on Zulip):

I tried for a little bit to do that but struggled.

pnkfelix (Dec 19 2018 at 20:59, on Zulip):

I can probably do it. Not tonight though. Have to wait until I get a bit more sleep

pnkfelix (Dec 20 2018 at 10:43, on Zulip):

Okay I have a test but its coupled to my own work for #55748

pnkfelix (Dec 20 2018 at 10:44, on Zulip):

@davidtwco you may want to just take my commits out of your PR

pnkfelix (Dec 20 2018 at 10:44, on Zulip):

and I'll do them in a manner that doesn't get fooled by macros separately.

pnkfelix (Dec 20 2018 at 10:46, on Zulip):

(If you want to see it, this is the test.]

davidtwco (Dec 20 2018 at 12:25, on Zulip):

@pnkfelix Is it only deduplication within a PatternTypeProjections or is it over all annotations that you need?

pnkfelix (Dec 20 2018 at 12:25, on Zulip):

i think its only over PatternTypeProjections

pnkfelix (Dec 20 2018 at 12:26, on Zulip):

or rather, I think that the only place where a single source type can cause multiple entries to be added ...?

pnkfelix (Dec 20 2018 at 12:26, on Zulip):

by "source type" I mean ... I guess a node representing a type in the macro-expanded AST

davidtwco (Dec 20 2018 at 12:29, on Zulip):

I've got something that doesn't deduplicate as much - it'll deduplicate within PatternTypeProjections so if you end up having a Field(field[0]) and a Field(field[1]) out of the same annotation then it won't be duplicated and won't be affected by macros or rely on spans. But it doesn't deduplicate globally like your fix did where the same annotation isn't used for a StatementKind::AscribeUserType statement even if it is the same underlying Ty.

davidtwco (Dec 20 2018 at 12:29, on Zulip):

I'm not sure if that is useful.

pnkfelix (Dec 20 2018 at 12:29, on Zulip):

it may be enough for my purposes

pnkfelix (Dec 20 2018 at 12:29, on Zulip):

my fix de-duplicating globally was a bug

davidtwco (Dec 20 2018 at 12:31, on Zulip):

For example, with my test case from yesterday, I get this.

davidtwco (Dec 20 2018 at 12:31, on Zulip):

For the code from your issue, I get this.

davidtwco (Dec 20 2018 at 12:31, on Zulip):

I'm not sure if it is right or not.

davidtwco (Dec 20 2018 at 12:32, on Zulip):

I suspect it isn't, because there are a lot of annotations.

davidtwco (Dec 20 2018 at 12:32, on Zulip):

And they're still different.

pnkfelix (Dec 20 2018 at 12:33, on Zulip):

/me reads

davidtwco (Dec 20 2018 at 12:34, on Zulip):

In visit_bindings instead of adding the CanonicalUserTypeAnnotation to PatternTypeProjections and then cloning that each time you add a projection, I'm storing it to get an index at the start and cloning the index after adding a projection.

pnkfelix (Dec 20 2018 at 12:34, on Zulip):

its a good sign that lines 78 and 79 on your code (for my issue) are referencing the same UserTypeAnnotation

davidtwco (Dec 20 2018 at 12:34, on Zulip):

Which I thought would work.

pnkfelix (Dec 20 2018 at 12:34, on Zulip):

yes that sounds plausible.

pnkfelix (Dec 20 2018 at 12:34, on Zulip):

You maybe want to just show me the diff?

pnkfelix (Dec 20 2018 at 12:35, on Zulip):

(and I'll apply it on my branch and give it a whirl)

davidtwco (Dec 20 2018 at 12:35, on Zulip):

https://gist.github.com/davidtwco/a00b8af51ca45558085baf838cfdc79f

davidtwco (Dec 20 2018 at 12:36, on Zulip):

I also haven't run all the tests on it, just those two files - so it might be completely broken in ways I've not checked yet.

davidtwco (Dec 20 2018 at 12:36, on Zulip):

(going to grab lunch now, so might be delayed in responding for ~20mins)

pnkfelix (Dec 20 2018 at 13:09, on Zulip):

that diff does well against the test I posted earlier.

davidtwco (Dec 20 2018 at 13:10, on Zulip):

It passes the test suite (except for the same dropck test that your initial change affected, it will undo that).

pnkfelix (Dec 20 2018 at 13:10, on Zulip):

it even fixes a case that I didn't expect it to

davidtwco (Dec 20 2018 at 13:10, on Zulip):

(and the same mir-opt test, same thing)

davidtwco (Dec 20 2018 at 13:11, on Zulip):

If it seems to do what we want then I'll add it to the PR?

pnkfelix (Dec 20 2018 at 13:11, on Zulip):

namely I expanded my previous test with this new gist

davidtwco (Dec 20 2018 at 13:11, on Zulip):

I saw that earlier but didn't know what other stuff it depended on from your PR.

pnkfelix (Dec 20 2018 at 13:11, on Zulip):

and this case on line 79 is now accepted under your diff

pnkfelix (Dec 20 2018 at 13:12, on Zulip):

which is totally cool with me

pnkfelix (Dec 20 2018 at 13:12, on Zulip):

(I don't understand why my previous version was rejecting it)

pnkfelix (Dec 20 2018 at 13:12, on Zulip):

yeah no that test is not going to be useful without the other commit in my branch

pnkfelix (Dec 20 2018 at 13:13, on Zulip):

which, as I said before, I think we should keep as a separate PR

davidtwco (Dec 20 2018 at 13:13, on Zulip):

Would you prefer I just add it on top of the existing commits or replace the last three with the first attempt at deduplication?

pnkfelix (Dec 20 2018 at 13:13, on Zulip):

I think you should replace

pnkfelix (Dec 20 2018 at 13:13, on Zulip):

as in, don't keep the bogus deduplication. its going just introduce noise in the history.

davidtwco (Dec 20 2018 at 13:14, on Zulip):

Sure, will do that now.

pnkfelix (Dec 20 2018 at 13:14, on Zulip):

I am a little curious whether we can (should?) use linked structures rather than cloning as we descend

pnkfelix (Dec 20 2018 at 13:15, on Zulip):

but its not something I really want anyone to spend much mental energy on

davidtwco (Dec 20 2018 at 13:15, on Zulip):

Yeah, I didn't like how much cloning it involved - don't think it was any more than there was before though.

pnkfelix (Dec 20 2018 at 13:15, on Zulip):

oh actually you are right

pnkfelix (Dec 20 2018 at 13:16, on Zulip):

I just hid the cloning in the methods, didn't i

davidtwco (Dec 20 2018 at 13:16, on Zulip):

Yeah.

pnkfelix (Dec 20 2018 at 13:16, on Zulip):

okay then don't worry about it

pnkfelix (Dec 20 2018 at 13:16, on Zulip):

unless you feel like re-hiding it

davidtwco (Dec 20 2018 at 13:16, on Zulip):

I think I prefer it not hidden.

pnkfelix (Dec 20 2018 at 13:16, on Zulip):

fine with me

davidtwco (Dec 20 2018 at 13:24, on Zulip):

PR updated.

pnkfelix (Dec 20 2018 at 13:27, on Zulip):

first up, P-high issues

davidtwco (Dec 20 2018 at 13:27, on Zulip):

Wrong topic.

davidtwco (Dec 20 2018 at 13:27, on Zulip):

And stream.

pnkfelix (Dec 20 2018 at 13:27, on Zulip):

d'oh

davidtwco (Dec 20 2018 at 13:27, on Zulip):

Don't think you can edit that to the right stream.

pnkfelix (Dec 20 2018 at 13:27, on Zulip):

could just delete all of this. :smile:

nikomatsakis (Dec 20 2018 at 16:05, on Zulip):

what is current status of @davidtwco's branch here? (cc @pnkfelix)

pnkfelix (Dec 20 2018 at 16:07, on Zulip):

From my most recent comment on the PR:

(this looks good to me. I'm planning to r+ but I'll wait a few hours to see if @nikomatsakis wants to weigh in on the latest commit.)

pnkfelix (Dec 20 2018 at 16:08, on Zulip):

basically @davidtwco came up with a more targetted way to avoid duplicate entries just when descending into the projections of a pattern

pnkfelix (Dec 20 2018 at 16:08, on Zulip):

and it is enough to resolve the problem I was facing

davidtwco (Dec 20 2018 at 16:31, on Zulip):

:thumbs_up:

davidtwco (Dec 21 2018 at 11:17, on Zulip):

I'm happy to start building on this PR to handle the unreachable case once I've got a clearer idea how we want to handle it.

davidtwco (Jan 02 2019 at 16:37, on Zulip):

@nikomatsakis do you have any thoughts on the toolstate breakages as a result of #55937? I’ve been taking a look and think there might have been a bug with my fix for #55511 because as far as I understand, I think the code mentioned in the PR comments should work?

nikomatsakis (Jan 02 2019 at 16:38, on Zulip):

@davidtwco I haven't seen that yet, link?

davidtwco (Jan 02 2019 at 16:39, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/pull/55937#issuecomment-450899521

davidtwco (Jan 02 2019 at 16:43, on Zulip):

https://github.com/rust-lang/rust/commit/4be7214d30b121762c8675c3e732083da262b136 is the commit that would have affected that.

nikomatsakis (Jan 02 2019 at 18:56, on Zulip):

Hmm, @davidtwco that does sound strange

nikomatsakis (Jan 02 2019 at 18:57, on Zulip):

I wonder if we can get a minimized reproduction. I also wonder if @Igor Matuszewski monitors zulip =)

nikomatsakis (Jan 02 2019 at 19:00, on Zulip):

well maybe this playground link suffices:

#![feature(nll)]

trait Foo {
    const Blah: &'static str;
}

struct Placeholder;

impl Foo for Placeholder {
    const Blah: &'static str = "hi";
}

fn foo(x: &str) {
    match x {
        <Placeholder as Foo>::Blah => { }
        _ => { }
    }
}
davidtwco (Jan 02 2019 at 19:01, on Zulip):

That looks like the same issue.

nikomatsakis (Jan 02 2019 at 19:01, on Zulip):

it certainly seems like a bug

nikomatsakis (Jan 02 2019 at 19:01, on Zulip):

that same example does compile for me

nikomatsakis (Jan 02 2019 at 19:02, on Zulip):

(with an older nightly)

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

looking at the generated Mir, we do indeed have an AscribeUserTypeAnnotation on the value being matched

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

that doesn't seem right though

nikomatsakis (Jan 02 2019 at 19:06, on Zulip):

perhaps because https://github.com/rust-lang/rust/commit/4be7214d30b121762c8675c3e732083da262b136 is incorrect

nikomatsakis (Jan 02 2019 at 19:06, on Zulip):

in general the handling of user type annotations and patterns is more complex than we first imagined I think

nikomatsakis (Jan 02 2019 at 19:31, on Zulip):

I guess we should file an issue on this

nikomatsakis (Jan 02 2019 at 19:31, on Zulip):

I can look a bit more

nikomatsakis (Jan 02 2019 at 19:31, on Zulip):

@davidtwco there is no issue filed yet, right?

davidtwco (Jan 02 2019 at 19:31, on Zulip):

Not that I know of.

nikomatsakis (Jan 02 2019 at 19:39, on Zulip):

I'll file one and do a bit of investigation perhaps

nikomatsakis (Jan 02 2019 at 19:41, on Zulip):

Filed https://github.com/rust-lang/rust/issues/57280

nikomatsakis (Jan 02 2019 at 21:29, on Zulip):

So @davidtwco I that some of the original examples from https://github.com/rust-lang/rust/issues/55511 also give errors when they shouldn't (in particular, the first example)

nikomatsakis (Jan 02 2019 at 21:29, on Zulip):

I'm still a touch confused why though

nikomatsakis (Jan 02 2019 at 21:30, on Zulip):

I also thought of a potential complication with user-type-annotations around promoted stuff--

nikomatsakis (Jan 02 2019 at 21:30, on Zulip):

if we "promote" a type assertion.. maybe we never do that I guess

nikomatsakis (Jan 02 2019 at 21:33, on Zulip):

Ah, I see why the error occurs, duh. We are requiring that the type of the path being matched is a subtype of the type of the constant that ... doesn't seem right.

davidtwco (Jan 02 2019 at 21:34, on Zulip):

Oops, should have paid more attention when working on that fix.

davidtwco (Jan 02 2019 at 21:36, on Zulip):

Ah, I see why the error occurs, duh. We are requiring that the type of the path being matched is a subtype of the type of the constant that ... doesn't seem right.

This might just be my misunderstanding, but isn't that what is required in order to make the should-error case from #55511 produce an error?

nikomatsakis (Jan 02 2019 at 21:39, on Zulip):

Well, it will produce an error in that case, but it's not the only way :)

davidtwco (Jan 02 2019 at 21:39, on Zulip):

Well, it will produce an error in that case, but it's not the only way :slight_smile:

Of course, right.

nikomatsakis (Jan 02 2019 at 21:40, on Zulip):

to be honest, the semantics of matching against a constant have long been debated

nikomatsakis (Jan 02 2019 at 21:40, on Zulip):

but if you think of it as e.g. similar to a == b

nikomatsakis (Jan 02 2019 at 21:40, on Zulip):

that does not require that A <: B nor B <: A (where A and B are the types of a and b respectively)

nikomatsakis (Jan 02 2019 at 21:40, on Zulip):

it does require that A: PartialEq<B>

nikomatsakis (Jan 02 2019 at 21:41, on Zulip):

but e.g. &'a str: PartialEq<&'static str> and &'static str: PartialEq<&'a str>

nikomatsakis (Jan 02 2019 at 21:41, on Zulip):

Of course, right.

don't feel too bad :P I at first was thinking it was the semantics we wanted too

nikomatsakis (Jan 02 2019 at 21:41, on Zulip):

but that was clearly just wrong, even in the original set of examples

nikomatsakis (Jan 02 2019 at 21:42, on Zulip):

for those original examples, something like ExpectedType <: MatchedType does work

nikomatsakis (Jan 02 2019 at 21:42, on Zulip):

(which is the reverse of what we are doing now)

nikomatsakis (Jan 02 2019 at 21:42, on Zulip):

but I don't think it's really right either

nikomatsakis (Jan 02 2019 at 21:43, on Zulip):

and actually this PartialEq impl for Cell is unnecessarily strict

#[stable(feature = "rust1", since = "1.0.0")]
impl<T:PartialEq + Copy> PartialEq for Cell<T> {
    #[inline]
    fn eq(&self, other: &Cell<T>) -> bool {
        self.get() == other.get()
    }
}
nikomatsakis (Jan 02 2019 at 21:43, on Zulip):

but I guess that's why we are getting an error

nikomatsakis (Jan 02 2019 at 21:44, on Zulip):

that is, it could really be

impl<T: Copy, U: Copy> PartialEq<Cell<U>> for Cell<T>
where
  T: PartialEq<U>,
{ .. }

I suppose

nikomatsakis (Jan 02 2019 at 21:44, on Zulip):

anyway now I'm wondering how the lexical type-checker is handling this

nikomatsakis (Jan 02 2019 at 21:46, on Zulip):

OK, so, @davidtwco , the lexical type checker actually does kind of the "wrong" thing too:

                // somewhat surprising: in this case, the subtyping
                // relation goes the opposite way as the other
                // cases. Actually what we really want is not a subtyping
                // relation at all but rather that there exists a LUB (so
                // that they can be compared). However, in practice,
                // constants are always scalars or strings.  For scalars
                // subtyping is irrelevant, and for strings `ty` is
                // type is `&'static str`, so if we say that
                //
                //     &'static str <: expected
                //
                // that's equivalent to there existing a LUB.
                self.demand_suptype(pat.span, expected, pat_ty);

that's from _match.rs

nikomatsakis (Jan 02 2019 at 21:46, on Zulip):

that fix doesn't seem very right

nikomatsakis (Jan 02 2019 at 21:46, on Zulip):

but I suppose we could match that behavior by extending pattern ascriptions on constants to have the opposite variance

nikomatsakis (Jan 02 2019 at 21:47, on Zulip):

which would be a small fix, if not the ideal one

nikomatsakis (Jan 02 2019 at 21:47, on Zulip):

I wonder if I can craft a test case that behaves poorly, probably

davidtwco (Jan 02 2019 at 21:48, on Zulip):

but I suppose we could match that behavior by extending pattern ascriptions on constants to have the opposite variance

I think that would be a good first step so that nightly doesn't have this behaviour and instead mirrors what AST borrowck does.

davidtwco (Jan 02 2019 at 21:48, on Zulip):

We could follow-up with the more correct fix.

nikomatsakis (Jan 02 2019 at 21:49, on Zulip):

yes

nikomatsakis (Jan 02 2019 at 21:49, on Zulip):

I'm actually not sure if I can create a kind of "actionable" example where the rule goes wrong in stable Rust

nikomatsakis (Jan 02 2019 at 21:50, on Zulip):

because there..huh. There are supposed to be limitations on the types of constants you can match against

nikomatsakis (Jan 02 2019 at 21:50, on Zulip):

they have to "derive" Eq and PartialEq iirc

nikomatsakis (Jan 02 2019 at 21:50, on Zulip):

and not have manual impl's

nikomatsakis (Jan 02 2019 at 21:52, on Zulip):

but I don't see the requisite (unstable...) annotations on Cell...

nikomatsakis (Jan 02 2019 at 21:55, on Zulip):

ah, wow, interesting

nikomatsakis (Jan 02 2019 at 21:56, on Zulip):

my example only compiled because I was using Option<Cell<u32>> and the value was None

nikomatsakis (Jan 02 2019 at 21:56, on Zulip):

if the value is Some, you get the stability error

nikomatsakis (Jan 02 2019 at 21:56, on Zulip):

anyway, @davidtwco, are you going to do that fix, or should I?

nikomatsakis (Jan 02 2019 at 21:56, on Zulip):

I can leave a few notes if desired

davidtwco (Jan 02 2019 at 21:56, on Zulip):

I can fix it.

davidtwco (Jan 02 2019 at 21:57, on Zulip):

Assuming you are fine with adding a field here with the variance, I think it should be alright to thread that through.

centril (Jan 02 2019 at 21:57, on Zulip):

@nikomatsakis speaking of user type annotations... https://github.com/rust-lang/rfcs/pull/2522#issuecomment-415551732 :slight_smile:

nikomatsakis (Jan 02 2019 at 21:59, on Zulip):

Assuming you are fine with adding a field here with the variance, I think it should be alright to thread that through.

yes this is basically what I had in mind

davidtwco (Jan 03 2019 at 14:45, on Zulip):

Submitted a PR with that.

davidtwco (Jan 03 2019 at 14:45, on Zulip):

#57304

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

Thanks @davidtwco :)

davidtwco (Jan 03 2019 at 16:09, on Zulip):

Making a quick change to it to fix an issue I've noticed.

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

Ah, @davidtwco, left a comment here

davidtwco (Jan 03 2019 at 16:21, on Zulip):

I've made it locally so that it is an Option<Variance> and I only set it to Some(..) in the specific spot where we want to reverse it, and then I just use .unwrap_or(..) with whatever was there previously - I found that I couldn't replicate the exact variances used in each case if I shifted that decision back like I was doing.

davidtwco (Jan 03 2019 at 16:22, on Zulip):

It seemed off to introduce a variance field to the PatternKind::AscribeUserType and then only use it in one case.

davidtwco (Jan 03 2019 at 16:23, on Zulip):

That should resolve your review comment?

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

hmm maybe but I agree something seems fishy

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

also, the use I highlighted is not "like" the others

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

as I noted -- it is constraining the type of a different thing

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

and that is why the variance does not apply

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

so I don't really think that the unwrap_or solution is right there

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

(though I think that the variance will always be None in that case)

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

are there other cases that didn't match up, @davidtwco?

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

I imagined that we would only change the spot that was currently Covariant

davidtwco (Jan 03 2019 at 16:48, on Zulip):

When I was looking at the diff previously, there was a place where we used Invariant and and one where we used Covariant, but I wasn't sure that they wouldn't result from the same PatternKind::AscribeUserType construction - where I didn't have the information to pick between Invariant and Covariant.

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

I can take another look @davidtwco, but I feel a bit uneasy with Option<Variance>

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

I'd feel better if we can say with certainty when it matters and when it doesn't

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

(and I think we can)

davidtwco (Jan 03 2019 at 16:56, on Zulip):

I'm happy to change the approach, I'm just not too sure what that would look like.

nikomatsakis (Jan 03 2019 at 17:30, on Zulip):

@davidtwco so in your new PR, I see two calls to unwrap_or, if i'm not mistaken, right?

davidtwco (Jan 03 2019 at 17:30, on Zulip):

Yeah.

nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

basically I think the PR looks like your original PR except that the place which uses Invariant just ignores the variance and continues to specify Invariant

nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

and the reason it ignores it should have a comment,

nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

the point is that the variance is the variance to use when relating the type annotation to the value being matched

nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

but in this case, that same annotation is being applied to the binding

nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

as an example:

let x: T = <expr>
nikomatsakis (Jan 03 2019 at 17:31, on Zulip):

here, the type T_x of x just is T (T == T_x, or T <: T_x and T_x <: T)

nikomatsakis (Jan 03 2019 at 17:32, on Zulip):

but the type U of <expr> need only meet U <: T

nikomatsakis (Jan 03 2019 at 17:33, on Zulip):

this is also why it is using Invariant and not Covariant now

nikomatsakis (Jan 03 2019 at 17:33, on Zulip):

which I should have commented in the first place but anyway

nikomatsakis (Jan 03 2019 at 17:33, on Zulip):

does that make sense?

davidtwco (Jan 03 2019 at 17:34, on Zulip):

The example makes sense but I'm still not sure I understand exactly what changes you're requesting.

nikomatsakis (Jan 03 2019 at 17:34, on Zulip):

ok well to start can you revert to the older version?

davidtwco (Jan 03 2019 at 17:35, on Zulip):

Doing that now.

davidtwco (Jan 03 2019 at 17:43, on Zulip):

Alright, pushed what was there before.

davidtwco (Jan 03 2019 at 17:55, on Zulip):

It seems like in doing that, I've lost the equivalent of this line where Covariant is given.

davidtwco (Jan 03 2019 at 17:58, on Zulip):

But I've not been able to work out which of this spot or this spot should be changed, or if neither is really correct as it depends on some decision made later in the code (which is why I used an Option).

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

@davidtwco left a review

davidtwco (Jan 03 2019 at 18:13, on Zulip):

Great, thanks.

centril (Jan 03 2019 at 18:14, on Zulip):

@nikomatsakis btw, this conversation seems relevant to this thread: https://github.com/rust-lang/rfcs/pull/2522#discussion_r209286375

centril (Jan 03 2019 at 18:14, on Zulip):

(pinged you there)

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

I've got to review that RFC at some point

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

/me goes to print it out

davidtwco (Jan 03 2019 at 18:34, on Zulip):

Addressed review comments @nikomatsakis

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

@davidtwco did they make sense to you? :)

davidtwco (Jan 03 2019 at 18:42, on Zulip):

Yeah, it all makes sense, cleared up some misconceptions about how we were doing things.

Last update: Nov 21 2019 at 23:25UTC