Stream: t-compiler/major changes

Topic: `ty.kind()` -> `ty.data()` compiler-team#359


triagebot (Sep 15 2020 at 20:16, on Zulip):

A new proposal has been announced: ty.kind() -> ty.data() #359. It will be announced at the next meeting to try and draw attention to it, but usually MCPs are not discussed during triage meetings. If you think this would benefit from discussion amongst the team, consider proposing a design meeting.

bjorn3 (Sep 15 2020 at 20:57, on Zulip):

The convention is currently to have X and XKind when you have an enum where one or more fields are shared between enum variants. Those are placed in X together with a field named kind which is an XKind enum. Ty/TyKind is an example of this. TyS has a flags and outer_exclusive_binder field in addition to kind. If you change TyKind, you should also change all other uses of XKind.

Jack Huey (Sep 16 2020 at 16:26, on Zulip):

This is a good point! Just searching through rustc, you find a lot of XKind. But, I actually don't think it makes sense to change most of them.

Jack Huey (Sep 16 2020 at 16:28, on Zulip):

For Ty, Region, and Const, we actually expect these to be interned, whereas the others I don't think so

Jack Huey (Sep 16 2020 at 16:29, on Zulip):

It's a little weird for TyS right now, since we intern that, not TyKind (compared to Chalk, where we don't track flags or outer_exclusive_binder at all right now)

nikomatsakis (Sep 17 2020 at 14:03, on Zulip):

@bjorn3 Yes, I agree it'd be best to change it consistently, and to be honest I'd be ok with that.

nikomatsakis (Sep 17 2020 at 14:04, on Zulip):

The convention doesn't scale to interning, basically, which is "similar but different"

nikomatsakis (Sep 29 2020 at 20:46, on Zulip):

cc @eddyb -- any thoughts on this? maybe you want to second? or object? :)

nikomatsakis (Sep 29 2020 at 20:46, on Zulip):

I guess we should settle whether we are just changing interned things or all enums

nikomatsakis (Sep 29 2020 at 20:46, on Zulip):

I'd be inclined to the former

Jack Huey (Sep 29 2020 at 20:47, on Zulip):

I think the former makes sense

eddyb (Oct 01 2020 at 15:02, on Zulip):

@nikomatsakis I don't mind it either way

eddyb (Oct 01 2020 at 15:02, on Zulip):

as long as it's not sty :P

nikomatsakis (Oct 01 2020 at 15:20, on Zulip):

@eddyb then second it :)

eddyb (Oct 01 2020 at 15:22, on Zulip):

my preference is being able to say Ty::Foo instead of TyKind::Foo or TyData::Foo

eddyb (Oct 01 2020 at 15:22, on Zulip):

but none seem bothersome

triagebot (Oct 01 2020 at 15:24, on Zulip):

@T-compiler: Proposal #359 has been seconded, and will be approved in 10 days if no objections are raised.

Vadim Petrochenkov (Oct 10 2020 at 10:03, on Zulip):

If you change TyKind, you should also change all other uses of XKind.

That would be enormous churn.

Vadim Petrochenkov (Oct 10 2020 at 10:04, on Zulip):

Why is the property of being interned is important for naming here?

Vadim Petrochenkov (Oct 10 2020 at 10:09, on Zulip):

I'm in favor of keeping TyKind because everything else is FooKind, it brings inconsistency, and the motivation for renaming is not strong.
(I don't usually work with compiler parts where it's used though.)

Vadim Petrochenkov (Oct 10 2020 at 10:13, on Zulip):

(I'm talking about #77768 specifically which renames enum TyKind to enum TyData.)

Vadim Petrochenkov (Oct 10 2020 at 10:18, on Zulip):

If there are plans to introduce some trait GetFromTheInternedThingToDataThatWasInterned { fn data() -> Self::Assoc; } like https://github.com/rust-lang/compiler-team/issues/359 tells, then the trait's fn data can still return .kind for specific instances?

pachi (Oct 10 2020 at 12:02, on Zulip):

AFAICT, it's trying to make rustc and chalk naming consistent.

LeSeulArtichaut (Oct 10 2020 at 13:44, on Zulip):

The end goal is to have a shared type library, this was a bikeshed between TyKind in rustc and TyData in chalk

Joshua Nelson (Oct 10 2020 at 13:52, on Zulip):

seems like it would be less churn to rename TyData -> TyKind in chalk

Joshua Nelson (Oct 10 2020 at 13:59, on Zulip):

I agree with Vadim Petrochenkov that I don't really see much benefit here

Jack Huey (Oct 10 2020 at 14:29, on Zulip):

So, the eventual goal is that it's not just TyKind that gets "interned" (I put this in quotes because there are many things that are actually interned, but I'm specifically talking about interned, with respect to the shared type library).If you look at chalk-ir, which we imagine we look something like the eventual shared type library, you'll find the Interner trait. This knows about multiple types of interned data: tys, regions, substitutions, etc. The point of this is that while TyKind -> TyData may not make a ton of sense in isolation, it makes much more sense when you consider the general pattern: you wouldn't necessarily call it SubstitutionKind. I can understand that this seems like churn, but there is a goal here.

Jack Huey (Oct 10 2020 at 14:30, on Zulip):

Another point to make: I would argue that comparing TyKind to all the other "Kind"s is not completely fair, if you consider that only a very small subset of these are interned

Joshua Nelson (Oct 10 2020 at 14:40, on Zulip):

I guess my question is why 'TyData' is more clear than 'TyKind'. I don't know what interning has to do with it.

LeSeulArtichaut (Oct 10 2020 at 15:02, on Zulip):

Vadim Petrochenkov said:

I'm in favor of keeping TyKind because everything else is FooKind, it brings inconsistency, and the motivation for renaming is not strong.
(I don't usually work with compiler parts where it's used though.)

Also, maybe, if we aim to have a shared type library, the rest of the rustc internals should not matter

Jack Huey (Oct 10 2020 at 15:27, on Zulip):

@Joshua Nelson TyData is not necessarily more (or less imo) clear than TyKind in isolation, but in terms of the expected shared type library, its consist with the other things that we want to be a part of that. "Interning" is..maybe not the best word choice. But the chalk-ir Interner is the trait that "keeps trait of" all those parts.

Joshua Nelson (Oct 10 2020 at 15:29, on Zulip):

https://rust-lang.github.io/chalk/book/types/role_of_interner.html?highlight=interner#the-role-of-the-interner

Vadim Petrochenkov (Oct 10 2020 at 15:55, on Zulip):

I see, it's a "data" in generic interner context

trait Interner {
    type InternedTy = MyRef<TyKind<Self>>;
    type TyData = TyKind<Self>;

    fn intern_ty(&self, data: &Self::TyData) -> Self::InternedTy;
    fn ty_data(data: &Self::InternedTy) -> &Self::TyData;
}
Vadim Petrochenkov (Oct 10 2020 at 15:56, on Zulip):

But that interned data type happens to a concrete enum TyKind.

Vadim Petrochenkov (Oct 10 2020 at 15:59, on Zulip):

But the concrete data type could be, for example, a struct with some common data attached to each kind

struct TyKindAndSpan {
    kind: TyKind,
    span: Span,
}

then the interner interface would look like this

trait Interner {
    type InternedTy = MyRef<TyKindAndSpan<Self>>;
    type TyData = TyKindAndSpan<Self>;

    fn intern_ty(&self, data: &Self::TyData) -> Self::InternedTy;
    fn ty_data(data: &Self::InternedTy) -> &Self::TyData;
}
Vadim Petrochenkov (Oct 10 2020 at 16:07, on Zulip):

Looks like right now the concrete data type in rustc is not even TyKind but rather TyS?

pub struct TyS<'tcx> {
    /// ...
    kind: TyKind<'tcx>,
    /// ...
    flags: TypeFlags,
    /// ...
    outer_exclusive_binder: ty::DebruijnIndex,
}

trait Interner {
    type InternedTy = MyRef<TyS<...>>;
    type TyData = TyS<...>>;

    fn intern_ty(&self, data: &Self::TyData) -> Self::InternedTy;
    fn ty_data(data: &Self::InternedTy) -> &Self::TyData;
}

Are there any reasons why do you intern TyKind specifically and not "TyKind + common fields" which is the equivalent of current TyS?

Vadim Petrochenkov (Oct 10 2020 at 16:28, on Zulip):

So, I guess my conclusion here would be to use the "data" terminology in the generic interner context, keep TyKind for the concrete enum and map one to another through a type alias, associated type, or through struct TyData { kind: TyKind } + fn kind(self) -> &TyKind { &self.ty_data().kind }.

Jack Huey (Oct 10 2020 at 16:51, on Zulip):

So, first off, on mobile, so my response here might be as thorough as I might want

Jack Huey (Oct 10 2020 at 16:53, on Zulip):

I'm trying to think about how to best put my thoughts into words

Jack Huey (Oct 10 2020 at 16:55, on Zulip):

I guess, the question here is: do we want Ty to be different than the other interned types

Jack Huey (Oct 10 2020 at 16:55, on Zulip):

Specifically, you do bring up a good point about the TypeFlags

Jack Huey (Oct 10 2020 at 16:56, on Zulip):

This isn't something we support in chalk right now, but we definitely plan on it soonish

Jack Huey (Oct 10 2020 at 16:56, on Zulip):

But, when we do, what is it going to look like?

Jack Huey (Oct 10 2020 at 16:57, on Zulip):

(I'm ignore outer_exclusive_binder at the moment, because I think any logic we have here extends)

Jack Huey (Oct 10 2020 at 17:00, on Zulip):

We either have two functions on Interner: ty_data(interned_ty) -> TyData and ty_flags(interned_ty) -> TypeFlags, where TyData is the enum (so it could also be ty_kind(...) -> TyKind)

Jack Huey (Oct 10 2020 at 17:01, on Zulip):

Or we instead have TyData look like our rustc's TyS. And then to get the enum, it's a field access.

Jack Huey (Oct 10 2020 at 17:02, on Zulip):

The former means we avoid the notion of having that concrete struct in the shared type library

Jack Huey (Oct 10 2020 at 17:04, on Zulip):

The latter makes that each "interned thing" gets one function in Interner (two if you thing intern/data)

Jack Huey (Oct 10 2020 at 17:05, on Zulip):

But the latter also means that for Tys every time we want to match on the data we do match ty.data(interner).kind

Jack Huey (Oct 10 2020 at 17:06, on Zulip):

But maybe that's what we want, since we might want to do let ty_data = ty.data(...); <something with ty_data.flags>; match ty_data.kind

Jack Huey (Oct 10 2020 at 17:08, on Zulip):

However, do we want to be consistent with Region? Should region_data return a struct with a kind field? Or an enum directly?

Jack Huey (Oct 10 2020 at 17:20, on Zulip):

What about things like substitution_data? These return structs currently that has functions like as_slice, iter, etc.

Jack Huey (Oct 10 2020 at 17:21, on Zulip):

I also just briefly looked over some of the things in Interner. Of those that are not an interned slice, there's a good mix of enums and not enums

Jack Huey (Oct 10 2020 at 17:30, on Zulip):

Jack Huey said:

Or we instead have TyData look like our rustc's TyS. And then to get the enum, it's a field access.

I am somewhat more convinced that this is maybe the right approach. I would like to hear what @nikomatsakis thinks. (And of course anyone else). I'm not particularly set on any one approach. I just want to make sure we're thinking about where it and other things are heading in the future, not where they are right now.

nikomatsakis (Oct 12 2020 at 14:00, on Zulip):

Hey all -- catching up. I'm definitely sensitive to not wanting churn.

nikomatsakis (Oct 12 2020 at 14:00, on Zulip):

And I do expect that chalk will require flags and some of the other "goodies" that rustc has

nikomatsakis (Oct 12 2020 at 14:00, on Zulip):

I've got some time marked up later to spend on chalk, so I'll try to sketch this out in a bit more detail and circle back to this thread then

Bram van den Heuvel (Oct 18 2020 at 15:24, on Zulip):

@nikomatsakis gentle reminder!

nikomatsakis (Oct 19 2020 at 13:44, on Zulip):

Thanks!

nikomatsakis (Oct 19 2020 at 14:07, on Zulip):

OK so, I think that the answer is that ty.data() ought to yield the enum, in my opinion

nikomatsakis (Oct 19 2020 at 14:07, on Zulip):

This is how I think it should be setup:

nikomatsakis (Oct 19 2020 at 14:08, on Zulip):

Jack Huey said:

We either have two functions on Interner: ty_data(interned_ty) -> TyData and ty_flags(interned_ty) -> TypeFlags, where TyData is the enum (so it could also be ty_kind(...) -> TyKind)

basically this, I guess

nikomatsakis (Oct 19 2020 at 14:09, on Zulip):

I think my reasoning for this is that it's relatively rare that we want to use the flags, so typing match ty.data().kind { .. } all the time is kind of overkill

nikomatsakis (Oct 19 2020 at 14:10, on Zulip):

but also, idk, it just "feels right" to me that the Ty<I> gives access to methods on the type, and one of those is to be able to match against precisely what kind of type it is

nikomatsakis (Oct 19 2020 at 14:11, on Zulip):

the only advantage I see to ty.data().kind is that we could have an optional trait for rustc that allows you to call data without providing a tcx, and we could implement the Deref trait for Ty<I> if that trait is implemented, which would then permit one to write match ty.kind { just as we do today basically

nikomatsakis (Oct 19 2020 at 14:11, on Zulip):

but in chalk you'd have to write ty.data(tcx).kind

nikomatsakis (Oct 19 2020 at 14:12, on Zulip):

that said, we could do the same trick but make it "deref" to the TyData enum, so that one writes match *ty { , which might be even nicer :) -- well, it's a bit "hacky"

nikomatsakis (Oct 19 2020 at 14:13, on Zulip):

I guess at the end of the day I don't care that much about the name and if we want to keep the kind name to avoid churn I'd be fine with that, but I generally do like the uniformity of data() to access the interned data, even for slices etc, and I generally like not using the term "kind" which has a meaning in type theory

nikomatsakis (Oct 19 2020 at 14:14, on Zulip):

but I don't think it's a big deal; I don't think the use of kind is particularly confusing, for example, and it's not a term we use in rustc (we use GenericArg, which I think is better)

nikomatsakis (Oct 19 2020 at 14:14, on Zulip):

another option would be to defer a bit the renaming and push first on other aspects of aligning the two types -- most notably trying to adjust the set of rustc/chalk variants to be closer together

Jack Huey (Oct 19 2020 at 15:48, on Zulip):

Okay, so for this MCP, what is the path forward?

nikomatsakis (Oct 19 2020 at 20:23, on Zulip):

Basically I think I still prefer to accept as originally proposed

nikomatsakis (Oct 19 2020 at 20:23, on Zulip):

Ultimately though this is a bit of a bikeshed

Joshua Nelson (Oct 19 2020 at 20:29, on Zulip):

personally I think if it's not a clear improvement it's not worth the churn

Joshua Nelson (Oct 19 2020 at 20:29, on Zulip):

for the same reason renaming compiler/rustc_ast to compiler/ast isn't worth it

nikomatsakis (Oct 19 2020 at 20:46, on Zulip):

the question becomes "what to do with slice elements". I guess we can just have two conversions -- call it kind() if it yields an enum, data() otherwise

Jack Huey (Oct 19 2020 at 21:43, on Zulip):

i'm okay with postponing this rename until we get more parity between rustc and Chalk in regards to the structure of types, adding flags, etc.

LeSeulArtichaut (Oct 21 2020 at 21:13, on Zulip):

@Jack Huey Should we close the MCP and the implementation PR?

Jack Huey (Oct 21 2020 at 21:14, on Zulip):

Yes, I think so :(

Jack Huey (Oct 21 2020 at 21:15, on Zulip):

Sorry again @Bram van den Heuvel

LeSeulArtichaut (Oct 21 2020 at 21:22, on Zulip):

At least you're not alone :grinning_face_with_smiling_eyes:

Jack Huey (Oct 21 2020 at 21:28, on Zulip):

Think of it this way: If the PR wasn't opened, we would probably still be on the question of should we

Last update: May 07 2021 at 07:30UTC