Skip to content

refactor the AssociatedItem structures #40697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikomatsakis opened this issue Mar 21, 2017 · 3 comments
Open

refactor the AssociatedItem structures #40697

nikomatsakis opened this issue Mar 21, 2017 · 3 comments
Labels
A-associated-items Area: Associated items (types, constants & functions) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 21, 2017

During the work on #40668, there was some discussion about refactoring the ty::AssociatedItem and hir::{Impl,Trait}ItemRef data structures. The precise plan is a bit unclear, so I'm opening this issue to try and discuss and lay it out.

Observations:

  • Although ty::AssociatedItem lives in ty, it has no real dependencies and is based entirely on the HIR, so it would better live in HIR.
    • The existing ImplItemRef and TraitItemRef are basically specialized variants of AssociatedItem, though they add a Span and use local-ids (ImplItemId) rather than a DefId.
  • There is a need for a query that given a DefId gives you back the AssociatedItem (including across crates). The local-crate portion of this query seems like it could live in hir::map quite nicely.
  • Some of this setup is carefully crafted to avoid incorrect incremental dependencies and can change as we progress on the proposed red/green changes (in particular, the current query goes to some lengths to avoid reading from Hir(X) when computing AssociatedItem(X), instead reading from the containing impl/trait; this is because we don't want to require everything that needed even basic information about X to have to change when X changes).

cc @eddyb @cramertj

@nikomatsakis nikomatsakis added A-associated-items Area: Associated items (types, constants & functions) C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Mar 21, 2017
@nikomatsakis
Copy link
Contributor Author

I think that having item-like things "out of line" makes sense in the HIR, but I'd (eventually perhaps) like to move towards unifying them into one vector, rather than having "items", "impl items", and "trait items" as 3 separate cases. i.e., I'd love to see a single Map<DefId, ItemLike>. To that end, perhaps we should move towards a (local) DefId as the key, instead of the newtype'd NodeId we have now? That would mean that ImplItemRef could just be an AssociatedItem (which includes a DefId) -- and perhaps eventually it would just be a DefId.

(This raises the point that I've been wanting to make some sort of LocalDefId type -- I'd rather not use DefIndex for this purpose, since then it's unclear if this is a def-id with LOCAL_CRATE or a def-index from some other crate.)

@steveklabnik
Copy link
Member

@nikomatsakis is this issue still relevant?

@eddyb
Copy link
Member

eddyb commented Sep 24, 2018

I'd say very much so, and I've been arguing recently that impl Trait should desugar to an item not parented to a module or a block, and that we should extend HIR's notion of item past AST's.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants