Stream: t-compiler/wg-nll

Topic: #54570 user type annot and complex patterns


nikomatsakis (Oct 09 2018 at 21:42, on Zulip):

@pnkfelix I think you once sent a link to your in-progress branch here...

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

yeah its around

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

hold on one sec

nikomatsakis (Oct 09 2018 at 21:43, on Zulip):

I am looking at refactoring how user type annotations are attached to constants in order to address #54574 (and, I think, the other issue assigned to me)

nikomatsakis (Oct 09 2018 at 21:43, on Zulip):

I think it will no longer suffice to just attach a CanonicalTy

nikomatsakis (Oct 09 2018 at 21:43, on Zulip):

but actually this is probably somewhat disjoint from what you are doing

nikomatsakis (Oct 09 2018 at 21:43, on Zulip):

since it applies specifically to constants

nikomatsakis (Oct 09 2018 at 21:43, on Zulip):

still, I was curious to see

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

this is probably the easiest thing for me to share: https://github.com/pnkfelix/rust/tree/issue-54570-proj-path-into-pats-with-type

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

I still need to rebase this atop your fix for the ICE

nikomatsakis (Oct 09 2018 at 21:45, on Zulip):

ok

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

or ... hmm, well, that actually might currently be icing for an entirely different reason

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

I cannot remember, and my most recent build log shows an ice that I myself injected here

pnkfelix (Oct 09 2018 at 21:47, on Zulip):

still, I think it captures the strategy I was planning to deploy

pnkfelix (Oct 09 2018 at 21:47, on Zulip):

Oh now I remember

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

Or ... I thought I had a big comment somewhere in the commit that said "if this works, why am I doing it this way"

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

but now I cannot find that comment. I must have decided it didn't work. :)

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

Oh I bet its on my laptop!

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

(sorry, so much of my workflow is centered on my remote desktop.)

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

yes yes yes!

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

okay I need to push the work from my laptop to that branch

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

but the ICE I was seeing on my laptop looked like this:

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

"librustc_mir/borrow_check/nll/universal_regions.rs:754: cannot convert ReScope(Remainder { block: ItemLocalId(110), first_statement_index: 1}) to a region vid

nikomatsakis (Oct 09 2018 at 21:55, on Zulip):

that is almost certainly the ICE I fixed, yes

nikomatsakis (Oct 09 2018 at 21:55, on Zulip):

and I can see why the changes you are making might trigger it

nikomatsakis (Oct 09 2018 at 21:56, on Zulip):

it would arise in particular if you were asserting types on more complex places that involve field projections

pnkfelix (Oct 09 2018 at 21:56, on Zulip):

okay github repo is updated

pnkfelix (Oct 09 2018 at 21:57, on Zulip):

and this is the comment slash big question to self that I was referring to up above: https://github.com/pnkfelix/rust/blob/30687327f112018837d227917e0331a052648b5f/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs#L93

pnkfelix (Oct 09 2018 at 21:57, on Zulip):

basically, when I started on this task, I expected at some point to have to apply the series of PlaceProjections to a base other than the one I started with

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

but that did not seem to happen

pnkfelix (Oct 09 2018 at 22:00, on Zulip):

but I suspect I'm probably overlooking some way in which the state of the Place (or the base of the Place) gets updated between the two points in the control flow of rustc

pnkfelix (Oct 25 2018 at 15:15, on Zulip):

@nikomatsakis thanks for the review. Got me off my duff and made an example that shows why we need to normalize during projection.

pnkfelix (Oct 25 2018 at 15:17, on Zulip):

if we weren't relatively close to cutting the beta, I'd wonder if I should be refactoring further (to fold my new code in with the code you had linked in sanitize type). But my current inclination is to try to land it as is, even with the minor amount of duplication of functionality?

pnkfelix (Oct 25 2018 at 15:17, on Zulip):

(oops and clearly a last minute rebase broke my PR... will fix)

pnkfelix (Oct 25 2018 at 15:19, on Zulip):

ooh and oh boy I need to rebase atop PR #55323 anyway! Okay so I guess don't worry about a re-review until i take care of that...

nikomatsakis (Oct 25 2018 at 15:30, on Zulip):

@pnkfelix ping me when ready

pnkfelix (Oct 25 2018 at 20:13, on Zulip):

@nikomatsakis ping: I have a question about how to properly rebase this work

pnkfelix (Oct 25 2018 at 20:49, on Zulip):

the main question is this: You shifted the code for handling TypeOf to a ascribe_user_type query. It looks like the "right" way (and perhaps only way?) forward for me is to add the list of projections to the AscribeUserType.

pnkfelix (Oct 25 2018 at 20:50, on Zulip):

But the struct AscribeUserType must be Copy. And therefore I'm probably going to have to intern the instances of [ProjectionElem<'tcx, (), ()>]. I can do that, it looks like a somewhat straight-forward addition. But I want you to stop me if this sounds like entirely the wrong path.

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

@pnkfelix why must AscribeUserType be copy?

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

let me think here...

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

because I tried making it non-Copy and everything went haywire?

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

heh :)

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

you could also intern them :)

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

I guess that's what you said

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

I get the impression that all the query types are Copy?

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

that may be true

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

it doesn't seem like it has to be true but maybe we added that requirement at some point to simplify things

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

(I didn't formally verify that. I just saw a macro that referenced AscribeUserType that itself requires it to be Copy. That was the impression I got from the compiler error)

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

anyway I.. think I agree that we have to pipe those projections in with the query

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

I may be about to the stage in my second rebase attempt where I could retry the experiment of making AscribeUserType non-copy

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

that is, add them to the query key

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

as an aside, @scalexm's PR that converts canonical to use bound-tys would be mildly helpful — my initial attempt at that query had the query key including a CanonicalUserTypeAnnotation

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

I had to back that out because we did not yet support recursive canonicalization

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

I think that should "just work" once https://github.com/rust-lang/rust/pull/55330 lands

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

I guess that's sort of neither here nor there

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

okay

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

just might mean a bit less duplication

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

(the "okay" was largely in response to "we have to pipe those projections in with the query")

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

I should have tried harder to the win the race. :)

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

haha sorry

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

though fwiw it seems like fixing ICE before adding your stuff is the right way forward

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

yeah no I'm just kidding

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

I'm sure its better that this ends up on my shoulders

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

for many reasons

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

and I think I'll be able to do it tomorrow. But I don't think I'll be able to do it quickly tonight, which means I will probably just go to bed now.

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

This is why AscribeUserType has to be Copy:

error[E0204]: the trait `Copy` may not be implemented for this type
   --> librustc/ty/query/plumbing.rs:729:18
    |
729 |           #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
    |                    ^^^^
730 |           pub enum Query<$tcx> {
731 |               $($(#[$attr])* $name($K)),*
    |                                    -- this field does not implement `Copy`
    |
   ::: librustc/ty/query/mod.rs:106:1
    |
106 | / define_queries! { <'tcx>
107 | |     Other {
108 | |         /// Records the type of every item.
109 | |         [] fn type_of: TypeOfItem(DefId) -> Ty<'tcx>,
...   |
688 | |     },
689 | | }
    | |_- in this macro invocation
pnkfelix (Oct 25 2018 at 21:19, on Zulip):

are you suggesting that we might be able to just make Query non-copy as well?

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

welp

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

I don't know how much work that would entail

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

seems plausible

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

otoh maybe it's better to just intern

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

it's kind of a pain

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

I think I'm better off interning the projection

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

the macros around this area are annoying I mean

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

but overall it seems probably good

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

I can at least predict the amount of effort attatched to that.

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

:)

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

I was musing to myself

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

whether we have any notion

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

of how much fragmentation is caused by these intern tables

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

i.e. how much a GC would reduce our rss

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

but its one of those things ... where you need to have a GC to even answer the question in the first place.

pnkfelix (Oct 26 2018 at 12:05, on Zulip):

@pnkfelix ping me when ready

Okay PR #55274 is now ready for review again. (I ended up merging much of the immediate review feedback into a commit during a rebase, but most of the new stuff to deal with the AscribeUserType type op is is follow-on commits, not squashed.

pnkfelix (Oct 26 2018 at 12:05, on Zulip):

@nikomatsakis ^

pnkfelix (Oct 26 2018 at 12:25, on Zulip):

hup, no, apparently that was premature. :(

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

or maybe its fine ...? I might have just hit some --keep-stage corruption on that last run, not sure yet

pnkfelix (Oct 26 2018 at 15:17, on Zulip):

Okay I think this is indeed ready @nikomatsakis : would be awesome if we could manage to get this in before beta is cut over weekend. But not end of world if not.

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

@pnkfelix r=me but needs rebase

pnkfelix (Oct 26 2018 at 18:39, on Zulip):

Yeah I hope to rebase tonight

Last update: Nov 21 2019 at 13:25UTC