-
Notifications
You must be signed in to change notification settings - Fork 182
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
Make dyn Trait
implement its super traits as well
#415
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some requests.
chalk-solve/src/clauses.rs
Outdated
builder.push_binders(&qwc, |builder, wc| match &wc { | ||
// For the implemented traits, we need to elaborate super traits and add where clauses from the trait | ||
WhereClause::Implemented(trait_ref) => { | ||
let mut seen_traits = FxHashSet::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is .. ungreat. I think that ideally we'd have a query like "supertraits of this trait def-id" that the db
offers, so that this calculation can be cached. Certainly rustc has such a query. I'm not sure exactly how to manage this -- I guess the pattern is to have the RustIrDatabase
trait have hooks for places we would like to have queries, and to have "default callbacks" that you can use to actually implement the query?
(This preserves independence from salsa -- if we didn't care about that, we could use salsa's existing facilities here I think for composable salsa databases.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this query would have to return something like Binders<Vec<Binders<TraitRef>>>
(outer binders for the original trait's parameters, inner binders for for<'a>
super trait bounds), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... in rustc, we skip the outer binders, but you're correct that ideally it'd have a Binders
type. Perhaps -- just for the purposes of this PR -- you could move this logic to a method on TraitDatum
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this turned out to be more interesting than expected, because if we have Foo: for<'a> Bar<'a>
and Bar<'a>: for<'b> Baz<'a, 'b>
we need to collapse the binders.
I've extracted it to a function for now; I'm not sure whether it makes sense to be on TraitDatum
, but if you want I can move it there.
So I've decided that this is going down the right direction after all. Before landing, though, we should probably convert #203 into a kind of "checklist of rules" to add, so we can track what's done and not done. Also, I think we should probably be extending the chalk book with documenting on these rules we're adding here, though that could be separate items on the check-list. |
f045b57
to
b6f8adb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits (just requests for doc-comments, more for completeness than anything), but otherwise 👍
This implements the impl rules from rust-lang#203, as far as I understood them. It doesn't do the well-formedness or object safety rules.
b6f8adb
to
2a90671
Compare
Rebased and added doc comments. |
This implements the impl rules from #203, as far as I understood them. It doesn't do the well-formedness or object safety rules.