[wip] Expand Impl API

[algesten]

This is the second part of this discussion.

The first part was to emit data::Impl from save-analysis.

This PR is to use that new data and expose it in a palatable API.

[algesten]

@nrc @steveklabnik I need guidance here of what the API needs to be. A number of issues comes to mind as I started looking at it.

• Impl have another id system than everything else. I reflected this by introducing the type ImplId. Is this a wanted addition to the API? The alternative would be to try derive unique global Id somehow... (effectively moving the problem we saw in save-analysis to here). The main problem with introducing a new id type is that current lookups like span -> Id will also take into account span -> ImplId
• Do we need a way of navigating from an Impl to its children? I introduced ImplDef for this, but is that something we want in the API? If we manage to derive a unique global Id for an Impl, we could use Def instead.
• host.crate_local_id(span_of_trait_fn) in this test case currently returns the Id of the trait method. Is this supposed to change to the impl method instead?
• I assume to solve this issue, we need to expand refs_span for to also include the methods of the impl?

[nrc]

Is this a wanted addition to the API?

I would really like to avoid exposing this to users. Ideally we should structure the API so that we don't need ids for impls (have a more hierarchical approach rather than a relational approach, in DB terms). Failing that we should be able to unify the ids more easily here. We already convert every compiler id into a different save-analysis id, and we should be able to add impl ids to the mix too, I think.

Do we need a way of navigating from an Impl to its children?

Double-check with @steveklabnik and @euclio, but I would think we definitely want this.

host.crate_local_id(span_of_trait_fn) in this test case currently returns the Id of the trait method. Is this supposed to change to the impl method instead?

method references are fun! So, I think that in for any method reference we need two optional ids - a trait id which refers to the method declared in the trait and an impl id which refers to the implementation. They are both optional because an inherent method does not have a trait id, and in generic context, we don't have the impl id.

Impl methods can also be regarded as both a def in their own right and a ref to the trait method.

rls-analysis probably needs to have an API which handles this at different levels of abstraction. For a tool like rustdoc, we know we are dealing with a method and so can supply the different ids etc in a very explicit way. For a tool like the RLS, we have only the concepts of defs and refs (though the LSP has added some concept of implementation recently, but I'm not sure how that affects things yet). A ref can only have one def. I'm wary of supplying multiple refs at the same span because I'm not sure that is treated deterministically in all clients. So I think we want to have a single ref which refers to the 'best' def we can find - the impl method if possible, the trait method otherwise. However, that might screw up 'find all refs' and renaming. Perhaps we should expand every ref to include a def id and an optional impl id?

I assume to solve this issue, we need to expand refs_span for to also include the methods of the impl?

It's complicated, see above.

[algesten]

Thanks @nrc. That gives me enough to get going. The plan:

1. Make a synthetic Id for Impls. (I hear what you say about hierarchy, but I think it makes more sense)
2. Treat Impls as Defs
3. Heuristics for which ref to return from a span (most "specific")
4. Wait further input from the others.

[nrc]

Treat Impls as Defs

I'm not quite sure about this one since it is not possible to reference them, I don't think impls need to be defs. Maybe they should be a thing in its own right (I think we do this for imports, but not sure).

[algesten]

Okay. I keep them separate.

[nrc]

@algesten is this ready for a review?

[algesten]

No sorry. I haven't had the time because of holiday followed by work going crazy this week. I hope to get back to it in the next few days.

[Xanewok]

Closing this due to inactivity, although I believe we'd still like that. cc @nrc
We moved this package to the central RLS monorepository and so we'll archive this repository and close any pending pull requests. If possible, please submit a PR there, thanks!