Stream: t-compiler

Topic: const generics in `infer` #59008


nikomatsakis (Apr 17 2019 at 16:50, on Zulip):

Hey @varkor (not sure what yodaldevoid's Zulip name is),

I'm reviewing #59008 and I have a few questions. To start, what is the story with LazyConst?

nikomatsakis (Apr 17 2019 at 16:50, on Zulip):

i.e., initially it's there, but when I check out the branch locally, it's not -- so I gather you removed it along the way

varkor (Apr 17 2019 at 16:51, on Zulip):

I'm not sure yodal is on Zulip

nikomatsakis (Apr 17 2019 at 16:51, on Zulip):

Is that just a renaming thing, or is it a deeper change in strategy?

varkor (Apr 17 2019 at 16:51, on Zulip):

yeah, LazyConst has since been removed

nikomatsakis (Apr 17 2019 at 16:52, on Zulip):

Also, just a side observation:

If we had had the design meetings already, this would have been an awesome PR to go through such meetings. Reviewing this sort of thing would be so much easier if I had a clearer idea of what to expect when going into it.

It's probably a good 'case study' to think about what improvements we could make.

varkor (Apr 17 2019 at 16:52, on Zulip):

previously, Const was split into LazyConst::Unevaluated and LazyConst::Evaluated (with all the various Const variants inside Evaluated)

nikomatsakis (Apr 17 2019 at 16:52, on Zulip):

ok

varkor (Apr 17 2019 at 16:53, on Zulip):

this would have been an awesome PR to go through such meetings

ah, that's true

nikomatsakis (Apr 17 2019 at 16:54, on Zulip):

previously, Const was split into LazyConst::Unevaluated and LazyConst::Evaluated (with all the various Const variants inside Evaluated)

and now? :)

varkor (Apr 17 2019 at 16:54, on Zulip):

(so, to clarify, the LazyConst discussion/changes are essentially just irrelevant aesthetic changes — it was just making the PR harder to read before it got refactored)

nikomatsakis (Apr 17 2019 at 16:55, on Zulip):

ok

varkor (Apr 17 2019 at 16:55, on Zulip):

now all the variants are in a single ConstValue enum

varkor (Apr 17 2019 at 16:55, on Zulip):

which is how it was before LazyConst

varkor (Apr 17 2019 at 16:55, on Zulip):

LazyConst wasn't actually around for very long

nikomatsakis (Apr 17 2019 at 16:56, on Zulip):

ok, ok

nikomatsakis (Apr 17 2019 at 16:57, on Zulip):

One thing I am wondering about is what the "normalization strategy" is

nikomatsakis (Apr 17 2019 at 16:57, on Zulip):

i.e., is this meant to be "eager normalized" in the same way as associated types?

varkor (Apr 17 2019 at 16:59, on Zulip):

I didn't make any deliberate decisions when it came to normalisation :sweat_smile:
if I didn't know what should be done, I generally tried to follow a similar strategy to the analogous methods for type variables

varkor (Apr 17 2019 at 16:59, on Zulip):

so it's possible that there are some confusing "design" choices with regards to normalisation

varkor (Apr 17 2019 at 17:01, on Zulip):

(that is to say, I don't feel I have such a good overview of the situation when it comes to normalisation in general)

nikomatsakis (Apr 17 2019 at 17:02, on Zulip):

ok

nikomatsakis (Apr 17 2019 at 17:02, on Zulip):

but the Unevaluated constant is meant to be a kind of "unnormalized" constant reference, right?

nikomatsakis (Apr 17 2019 at 17:03, on Zulip):
pub enum ConstValue<'tcx> {
    ...
    /// Used in the HIR by using `Unevaluated` everywhere and later normalizing to one of the other
    /// variants when the code is monomorphic enough for that.
    Unevaluated(DefId, SubstsRef<'tcx>),
}
nikomatsakis (Apr 17 2019 at 17:03, on Zulip):

i.e., that :point_up:

varkor (Apr 17 2019 at 17:03, on Zulip):

ah, I think the general principle is to leave it Unevaluated as long as possible

nikomatsakis (Apr 17 2019 at 17:07, on Zulip):

(so, to clarify, the LazyConst discussion/changes are essentially just irrelevant aesthetic changes — it was just making the PR harder to read before it got refactored)

just to double check something -- the changes around LazyConst occurred on master, and this PR was rebased over them?

varkor (Apr 17 2019 at 17:08, on Zulip):

yep

nikomatsakis (Apr 17 2019 at 17:08, on Zulip):

Also, to double check that I am understanding things -- what exactly is this PR doing. I think it is extending inference to include inference variables representing unknown constant values, and permitting unification with those?

varkor (Apr 17 2019 at 17:08, on Zulip):

yes, that's the intention

nikomatsakis (Apr 17 2019 at 17:09, on Zulip):

i.e., it is not really trying to define what const values are exactly? In other words, the ConstVal enum already exists, and we have operations on it etc that already happen.

(Except that it seems to be doing a fair amount of work on the folders etc...)

varkor (Apr 17 2019 at 17:09, on Zulip):

it extends ConstValue a bit

varkor (Apr 17 2019 at 17:10, on Zulip):

I think the ConstValue::Infer variant is already added on master, but it doesn't do very much

varkor (Apr 17 2019 at 17:10, on Zulip):

so this PR also includes the handling of that variant

varkor (Apr 17 2019 at 17:11, on Zulip):

(essentially the equivalent of TyKind::Infer)

nikomatsakis (Apr 17 2019 at 17:11, on Zulip):

OK, I see

nikomatsakis (Apr 17 2019 at 17:12, on Zulip):

I probably won't finish with this review today

nikomatsakis (Apr 17 2019 at 17:12, on Zulip):

but I will schedule another 1.5-2hour block

nikomatsakis (Apr 17 2019 at 17:12, on Zulip):

and I think I'll be able to move much faster now that i've kind of figured out what this PR is even about :P

nikomatsakis (Apr 17 2019 at 17:12, on Zulip):

(so far it all seems to make sense, though)

varkor (Apr 17 2019 at 17:12, on Zulip):

the folder changes are due to constants previously not turning up as inference variables, so that case now needs to be considered

nikomatsakis (Apr 17 2019 at 17:12, on Zulip):

I haven't really gotten to trying to answer @eddyb's questions yet

varkor (Apr 17 2019 at 17:13, on Zulip):

okay, great :)

varkor (Apr 17 2019 at 17:13, on Zulip):

yeah, I appreciate that it's a large one to get through

nikomatsakis (Apr 17 2019 at 17:16, on Zulip):

on a semi-related note, I want to do a call where I talk through how the Lark inference code and type-checker was setup

nikomatsakis (Apr 17 2019 at 17:16, on Zulip):

in part to help me remember :P

nikomatsakis (Apr 17 2019 at 17:16, on Zulip):

but in part because I rejiggered some of the bits here to try and address some of the boilerplate and other things I saw arising in rustc

nikomatsakis (Apr 17 2019 at 17:16, on Zulip):

not sure if where I ended up was better or not really :) but it's worth thinking about

varkor (Apr 17 2019 at 17:18, on Zulip):

there are definitely some places I saw that could use some refactoring as I was going through, but doing it in a more principled fashion would probably be wiser :+1:

nikomatsakis (Apr 17 2019 at 17:18, on Zulip):

yes, confirm

nikomatsakis (Apr 17 2019 at 17:18, on Zulip):

@varkor -- one other question. I don't think I saw any tests yet in this PR

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

maybe I missed them

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

is there a reason that we can't write tests for this code?

varkor (Apr 17 2019 at 17:19, on Zulip):

ah, that's true — most of the tests are still in the main const generics PR

varkor (Apr 17 2019 at 17:19, on Zulip):

but I think with these changes, most of them should work now

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

I would prefer to land the code together with tests

varkor (Apr 17 2019 at 17:19, on Zulip):

I'll try moving them over

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

I would also like to kind of walk through some of the representative tests

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

i.e., I've sort of seen the "trees" now

eddyb (Apr 17 2019 at 17:19, on Zulip):

@nikomatsakis heh, so, LazyConst was an accidental thing in part caused by me not being around: ty::Const is supposed to be a third kind, alongside Ty and ty::Region, but not everyone was aware of that

nikomatsakis (Apr 17 2019 at 17:19, on Zulip):

I was hoping (in the next review session) to try and see the forest

eddyb (Apr 17 2019 at 17:20, on Zulip):

now it's back to what it should be, and ConstValue is just the enum with the variants of ty::Const (which has to be a struct because of the ty: Ty field)

nikomatsakis (Apr 17 2019 at 17:20, on Zulip):

so if you have a sample test or two that would be good to walk through

nikomatsakis (Apr 17 2019 at 17:20, on Zulip):

I'd appreciate it

eddyb (Apr 17 2019 at 17:21, on Zulip):

I am close to wanting to just unify all the kinds :P

varkor (Apr 17 2019 at 17:21, on Zulip):

I'll check the tests tonight and also try to add a few more

eddyb (Apr 17 2019 at 17:21, on Zulip):

and rename Kind to Term

varkor (Apr 17 2019 at 17:21, on Zulip):

they've been added on quite an ad hoc basis at the moment

varkor (Apr 17 2019 at 17:21, on Zulip):

@eddyb: that doesn't sound like an enviable task

eddyb (Apr 17 2019 at 17:22, on Zulip):

it's tempting, but not necessarily enticing

centril (Apr 17 2019 at 17:22, on Zulip):

@eddyb what is your next step after that? cumulative universes? :P

eddyb (Apr 17 2019 at 17:22, on Zulip):

or is it the other way around? :P

eddyb (Apr 17 2019 at 17:22, on Zulip):

@centril how would you have those with no dependent typing :P

centril (Apr 17 2019 at 17:22, on Zulip):

Type0, Type1, ... here we go

centril (Apr 17 2019 at 17:23, on Zulip):

@nikomatsakis @varkor Yay for tests! They will definitely be more important as the feature nears completion, especially for a feature this big

varkor (Apr 17 2019 at 17:23, on Zulip):

@centril: if you like writing tests... :)

centril (Apr 17 2019 at 17:24, on Zulip):

@varkor apparently @Alexander Regueiro assigned me as Mr. Test :P

Alexander Regueiro (Apr 17 2019 at 17:24, on Zulip):

:-D

Alexander Regueiro (Apr 17 2019 at 17:24, on Zulip):

Either one of you is fine!

Alexander Regueiro (Apr 17 2019 at 17:24, on Zulip):

oh, this is consnt generics. I thought you were talking about upcasting again

yodal (Apr 17 2019 at 17:26, on Zulip):

hello, folks. I'm a tad late to the party

yodal (Apr 17 2019 at 17:28, on Zulip):

re: tests, we have not included tests up until this point because most uses of const generics would crash the compiler. With theses changes, we might have enough for some to pass, though I don't know how much we have outside of this PR left to merge in from our massive PR

yodal (Apr 17 2019 at 17:29, on Zulip):

also, we definitely need more tests, specifically more tests that target a specific aspect of the feature

varkor (Apr 17 2019 at 17:29, on Zulip):

yeah, I was planning to compare master with the main PR once the infer changes had landed; I'm not sure how many changes remain after that either

Last update: Nov 22 2019 at 05:55UTC