Conversation
|
One thing I just can't catch is messing everything up, will go over it tmr evening. If anyone has a time for a quick scan it should be in types.rs, but im not too sure |
| self, | ||
| db: &'db dyn Db, | ||
| target: Type<'db>, | ||
| ) -> Result<(), TypeRelationError> { |
There was a problem hiding this comment.
Nit: It feels a bit off to me to use a Result here. I would expect an Err only if two types can't be compared but that's not how Result is used here.
I think I'd use a custom enum over Result with a Yes and No(reason) variants. We can mark the type as #[must_use] if we're worried about callers not handling the error but it seems not handling the error actually seems to be the default?
What's unclear to me is how we plan on aggregating TypeRelationErrors. But maybe that's just because it's not yet clear to me what variants TypeRelationError will have
There was a problem hiding this comment.
I think using Result might have some advantages further down the line: methods like map_err could be useful for bubbling up a low-level assignability failure of a wrapped type into a high-level assignability failure for the wrapping type. But I agree that we need a more ergonomic solution for the default case: I was going to suggest having low-level methods try_type_relation, try_subtype_relation and try_assignability_relation (which you can use if you need to know why the relation doesn't apply between two types), but to also have high-level has_relation_to, is_subtype_of and is_assignable_to methods like we have now:
impl<'db> Type<'db> {
fn has_relation_to(db: &'db dyn Db, other: Type<'db>, relation: TypeRelation) -> bool {
self.try_type_relation(db, other, relation).is_ok()
}
fn is_subtype_of(db: &'db dyn Db, other: Type<'db>) -> bool {
self.has_relation_to(db, other, TypeRelation::Subtyping)
}
fn is_assignable_to(db: &'db dyn Db, other: Type<'db>) -> bool {
self.has_relation_to(db, other, TypeRelation::Assignability)
}
}it's a bit hard to tell what the best design is, however, until we try to start trying to use (in diagnostics) the extra information now available to us.
There was a problem hiding this comment.
We can always implement map_err etc. on the custom enum. I just find is_ok to be too implicit and has_relation_to(db, other, relation) == Ok(()) seems to verbose (and also confusing).
There was a problem hiding this comment.
I don't have a strong opinion
There was a problem hiding this comment.
try_... seems like a good structure and simplifies this PR a lot
There was a problem hiding this comment.
Creating our own enum also means we can implement from bool. Which also simplifies a lot of code
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
CodSpeed Performance ReportMerging #19580 will not alter performanceComparing Summary
|
CodSpeed WallTime Performance ReportMerging #19580 will not alter performanceComparing Summary
|
|
Will mark as ready for some initial feedback |
|
Not sure as of now what to do about the regressions |
|
One thing you could try is ot use But I think I'll need a better understanding for what we want to use this new information and if it's even worth tracking all possible errors. It might be good to start with: What exact information do we need and, from there, backtrack on what information we need to propagate in these relation methods. It might also be that we want separate relation methods to avoid the overhead in the most common path. |
|
It's probably also useful if I turn TypeRelationError into an enum? All of the todo fails right now will create a new vec, but for the most part most vec will have one item |
astral-sh/ty#163 (comment) and astral-sh/ty#163 (comment) show examples of why we need to track this information |
| }, | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
this is a lot more expensive than what we do on main because it means that we now have to iterate through the entire MRO (and allocate a Vec) whereas previously we could short-circuit as soon as we spotted that one class was a subclass of the other. We should avoid .collect() calls in this and similar cases wherever possible, and continue to short-circuit. I think in most cases, it's probably okay if we only provide a single reason why one type is not a subtype of another, even if there are multiple underlying reasons why the relation cannot be satisfied.
Protocols might be an exception to this, but we can deal with that later
There was a problem hiding this comment.
Yeah I realised this, will address later
What's not clear to me from this if this is something we need to integrate into the |
Yeah, that's a good question. It would be interesting to experiment with some different designs here. The nice thing about the current approach in this PR is that it does not increase the maintenance burden of the type-relational methods. I would definitely not want to go back to a situation similar to what we had before #18430: having separate There might also be ways of implementing your suggestion that would not significantly increase the maintenance burden. On the other hand, I also think there are some obvious ways of optimising this PR branch, as I commented above |
|
Nice, thats a bit better |
|
nice!! |
|
There's maybe an argument to rename I'm also not sure if we should have a base ( |
|
Is there any interest in taking this further? |
Yes, definitely! I think we absolutely need something like this. Sorry we haven't got back to you, that's on me :-( There's a lot of things to juggle right now. |
|
All good, when you want to revisit let me know and I can fix the merge conflicts |
3 participants
Summary
Towards astral-sh/ty#163
Test Plan