Stream: t-compiler/wg-rls-2.0/chalk

Topic: matching ids


detrumi (Mar 13 2019 at 20:48, on Zulip):

So I started experimenting with Chalk's ItemIda bit, trying to split it into StructId, TypeId etc., similar to how RA does it

detrumi (Mar 13 2019 at 20:49, on Zulip):

It's amazing how well it fits in many places, since the variable names often reflect what kind of ItemId you're dealing with

detrumi (Mar 13 2019 at 20:51, on Zulip):

So currently, my idea is to make ItemId an enum with a variant for each kind of id

detrumi (Mar 13 2019 at 20:54, on Zulip):

There's some places where there's two kinds of ids grouped together, and some of those are kind of tricky. The NameLookup::Type(TypeId) seems to hold either a TraitId or a TypeId. I'm not sure whether to use 2 separate variants for the NameLookup enum, or to create a TypeOrTraitId enum

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

So currently, my idea is to make ItemId an enum with a variant for each kind of id

I think that the rust-analyzer does the same (right @Florian Diebold?)

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

There's some places where there's two kinds of ids grouped together, and some of those are kind of tricky. The NameLookup::Type(TypeId) seems to hold either a TraitId or a TypeId. I'm not sure whether to use 2 separate variants for the NameLookup enum, or to create a TypeOrTraitId enum

Hmm. Seems like we could add a NameLookup::Trait to the NameLookup enum

detrumi (Mar 13 2019 at 21:00, on Zulip):

Right, I think RA's ArenaId does the same thing

detrumi (Mar 13 2019 at 21:06, on Zulip):

Ah, the TypeSort enum (Struct | Trait) looks similar to the type/trait thing

detrumi (Mar 13 2019 at 21:32, on Zulip):

This seems to work out, though it's a bit repetitive:

for (item, &item_id) in self.items.iter().zip(&item_ids) {
    if let (Item::TraitDefn(ref d), ItemId::TraitId(trait_id)) = (*item, item_id) {
detrumi (Mar 13 2019 at 21:37, on Zulip):

Oh wait, that's doesn't make any sense, since the item ids are also generated during lowering, so that needs to change

detrumi (Mar 13 2019 at 21:42, on Zulip):

Hmm, maybe Chalk needs to generate the ids in the same way that rust-analyzer does it (assuming RA does that somewhere, haven't found it yet)

Florian Diebold (Mar 13 2019 at 22:00, on Zulip):

(deleted)

Florian Diebold (Mar 13 2019 at 22:01, on Zulip):

RA has various enums depending on usage for situations where multiple different types of item can be used

Florian Diebold (Mar 13 2019 at 22:02, on Zulip):

e.g. https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir/src/adt.rs#L19, or https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir/src/code_model_api.rs#L73

Florian Diebold (Mar 13 2019 at 22:03, on Zulip):

IDs are generated by interning the locations of items (as in 'nth item of that type in file x'), here: https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir/src/ids.rs

matklad (Mar 14 2019 at 06:58, on Zulip):

I think that the rust-analyzer does the same

Not exactly. We don't have a single "universal ID for everything" enum. As Florian Diebold said, we just create ad-hock eums for a subset of ids when we need them.

detrumi (Mar 14 2019 at 07:23, on Zulip):

Ah, so ids are generated by the interning, gotcha. Then Chalk's way during lowering is slightly simpler, as it simply zips the items iterator with fresh ids

matklad (Mar 14 2019 at 07:33, on Zulip):

I think that the rust-analyzer does the same

Not exactly. We don't have a single "universal ID for everything" enum. As Florian Diebold said, we just create ad-hock eums for a subset of ids when we need them. (EDIT: this actually is hand-written "polymorphic variants" from OCaml I guess?)

detrumi (Mar 14 2019 at 07:36, on Zulip):

So you mean that there's no global counter to generate ids?

matklad (Mar 14 2019 at 07:49, on Zulip):

Yep. This seems like a deep phenomenon of salsa-based systems. You need some kind of "identity", to refer to entities.

One choice is to collect all entities into one big array and use index into the array as an ID (so, a global counter). This is bad for incremental, as removing an entity shifts ids.

The second choice is a fine-grained location, like "file foo.rs, line 92, col 63". In this setup entities are somewhat independent (adding entity shifts ids only within one file), but still locations themselves are fragile

The third choice is a path-based location "field with name foo in a struct namedBar in file baz.rs". This are stable, but big. Like, entities can be recursive, so paths can be arbitrary large. This, however, can be fixed by interning the path, effectively compressing it to u32.

detrumi (Mar 14 2019 at 20:15, on Zulip):

I got things to compile after splitting up the ids: https://github.com/rust-lang/chalk/pull/208/files

detrumi (Mar 14 2019 at 20:16, on Zulip):

Glad to see that tests are failing, which means they're working :slight_smile:

detrumi (Mar 15 2019 at 10:33, on Zulip):

Hmm, do associated types fall under types or traits? Tests are saying that Eq is a type, whereas a trait was expected

detrumi (Mar 15 2019 at 10:37, on Zulip):

Since AssociatedTyDatum has both a trait_id and an id, I guess it should really be a type

detrumi (Mar 22 2019 at 13:40, on Zulip):

That debug_item_id function was tricky to get right after splitting up the ids, but I got the tests green at least

nikomatsakis (Mar 22 2019 at 20:33, on Zulip):

@detrumi sorry for not following up here! This PR looks quite nice, looks like it just needs to be rebased?

detrumi (Mar 22 2019 at 20:36, on Zulip):

Oh that's new, let me rebase again

detrumi (Mar 22 2019 at 20:38, on Zulip):

The debug impls are a bit messy, since there's some lookups in one of the impls

nikomatsakis (Mar 22 2019 at 20:39, on Zulip):

Ah, ok

nikomatsakis (Mar 22 2019 at 20:39, on Zulip):

Maybe leave a comment on what you are referring to?

detrumi (Mar 22 2019 at 21:03, on Zulip):

Actually, the code seems straightforward enough

detrumi (Mar 22 2019 at 21:24, on Zulip):

@nikomatsakis Rebased and all green!

nikomatsakis (Mar 22 2019 at 21:26, on Zulip):

Merged, nice work!

detrumi (Mar 22 2019 at 21:27, on Zulip):

Thanks!

Last update: Nov 15 2019 at 11:00UTC