Skip to content
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

refine RustIrDatabase interface #241

Closed
nikomatsakis opened this issue Sep 13, 2019 · 9 comments
Closed

refine RustIrDatabase interface #241

nikomatsakis opened this issue Sep 13, 2019 · 9 comments
Labels
good first issue A good issue to start working on Chalk with

Comments

@nikomatsakis
Copy link
Contributor

This issue is for discussing the design of RustIrDatabase trait.

@nikomatsakis
Copy link
Contributor Author

First objection:

The impls_for_trait method should supply hints about the types of impls we're interested in -- e.g., the self type. This is a crucial performance optimization in rustc.

This method is used for three purposes:

  • finding "all local" impls for coherence purposes
  • finding impls relevant to solving Implemented(T: Trait) goals
  • finding impls relevant to projection goals -- here we are really looking for the associated type values found inside impls

@nikomatsakis
Copy link
Contributor Author

Second objection:

The all_structs method should be removed as well. We want to avoid any kind of "give me all the things" methods, I think, if possible.

This method is used to enumerate possible instantiations of auto traits where the self type is an unbounded inference variable; it's also used for some other kinds of internal goals, like DownstreamType, with similar reasons.

@nikomatsakis
Copy link
Contributor Author

I started the remove-unselected-projection branch working towards these two goals. It does a few things:

  • Introduced a method to use for coherence, which is kind of a special case
  • Insisted that we flounder all goals where the "self type" is an unbounded inference variable
    • I think we could refine this rule in the future, but it matches what rustc does

I'm currently working through the impact of that rule. It makes some of the coherence related tests fail, but I'm not sure how important that is.

If we take this step, though, we still don't quite reach the goal I wanted because of interactions with lazy normalization. See this comment for more details.

@nikomatsakis
Copy link
Contributor Author

Another concern:

I think I would prefer to get away from removing "complex structs"...maybe? Not entirely sure about this. But it seems like it might be nice to have relatively "flat" methods, so that we can map more easily to whatever "HIR-like" data structures exist on the other side (and perhaps for better incremental tracking).

@flodiebold
Copy link
Member

@nikomatsakis The branch you linked doesn't contain anything related to this as far as I can see; is it the right one?

Agree about both points; all_structs is actually still a stub returning vec![] in rust-analyzer.

I think I would prefer to get away from removing "complex structs"...maybe? Not entirely sure about this. But it seems like it might be nice to have relatively "flat" methods, so that we can map more easily to whatever "HIR-like" data structures exist on the other side (and perhaps for better incremental tracking).

Not sure how that would look...

@flodiebold
Copy link
Member

By the way, I actually need this for what I'm working on in RA currently -- I need to make every closure type implement the Fn traits, so I would need to find all closures in the whole crate or even project to satisfy the current interface.

@flodiebold
Copy link
Member

Also, I ran into the problem that in the process of inferring types for an Option::map call, I try to solve Normalize(<?1 as FnOnce<(u32,)>>::Output => ?0) and get back that it isn't solvable (instead of ambiguous), so the inference gives up... This might also be solved by the rule you mentioned?

@jackh726 jackh726 added design Needs design work needs triage Needs to be discussed labels Feb 7, 2020
@jackh726
Copy link
Member

Another concern:

I think I would prefer to get away from removing "complex structs"...maybe? Not entirely sure about this. But it seems like it might be nice to have relatively "flat" methods, so that we can map more easily to whatever "HIR-like" data structures exist on the other side (and perhaps for better incremental tracking).

Triage: this is what this issue needs to be closed. The idea is ,for example, rather than return an AdtDatum, we instead have separate functions for adt_flags and adt_def, etc.

@jackh726 jackh726 added good first issue A good issue to start working on Chalk with and removed design Needs design work needs triage Needs to be discussed labels Jun 23, 2020
@jackh726
Copy link
Member

Actually, closing in favor of #506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good issue to start working on Chalk with
Projects
None yet
Development

No branches or pull requests

3 participants