[ty] Remove the Constraints trait
#20355
Conversation
dcreager
added
internal
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| /// Returns a constraint set that never holds | ||
| fn unsatisfiable(db: &'db dyn Db) -> Self; | ||
|
|
||
| /// Returns a constraint set that always holds | ||
| fn always_satisfiable(db: &'db dyn Db) -> Self; |
There was a problem hiding this comment.
These methods (and by extension, from_bool) didn't actually use their db parameter, so I've simplified the API a bit. from_bool is now replaced with a From<bool> impl, and these two constructors are replaced with ConstraintSet::from(true) and from(false).
There was a problem hiding this comment.
In terms of readability, I think I weakly prefer the old spelling, actually! ConstraintSet::unsatisfiable() seems clearer to me about what it means than ConstraintSet::from(false), even if the constructor is strictly-speaking redundant with the new From<bool> implementation. Same for ConstraintSet::always_satisfiable.
There was a problem hiding this comment.
If it's a weak preference, I'll put in a vote in favor of the new spelling (or at least, in favor of not taking time to reintroduce the old methods and updating the PR to use them everywhere.)
There was a problem hiding this comment.
or at least, in favor of not taking time to reintroduce the old methods and updating the PR to use them everywhere
I did that change in a separate commit, so the cost of reverting it would be ~zero if that sways your vote
There was a problem hiding this comment.
I don't feel strongly either, but I think the new spelling is pretty clear, and I like that it generalizes well to cases where we already have a boolean value, not literal true or false (e.g. ConstraintSet::from_bool(relation.is_assignability)). So it seems like we will need to have (and be able to read and understand the concept of) ConstraintSet::from_bool anyway, which makes me dis-inclined to pad the API with special-case methods for literal true and false.
| /// Updates this constraint set to hold the union of itself and another constraint set. | ||
| pub(crate) fn union(&mut self, db: &'db dyn Db, other: Self) -> &Self { | ||
| self.union_set(db, other); | ||
| self | ||
| } | ||
|
|
||
| /// Updates this constraint set to hold the intersection of itself and another constraint set. | ||
| pub(crate) fn intersect(&mut self, db: &'db dyn Db, other: &Self) -> &Self { | ||
| self.intersect_set(db, other); | ||
| self | ||
| } |
There was a problem hiding this comment.
There's now an annoying difference here, where union takes in an owned other, while intersect takes in a reference. This is purely due to internal details about when we can/cannot reuse the existing storage. This didn't exist before, because clippy couldn't see through the trait to trip the "you can pass this by reference" lint. I decided to go ahead and accept clippy's recommendation. If folks prefer, I would be equally happy putting an #[expect] here so that both methods take in an owned other.
There was a problem hiding this comment.
I'm happy to genuflect to Clippy here!
dcreager
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
| /// Returns a constraint set that never holds | ||
| fn unsatisfiable(db: &'db dyn Db) -> Self; | ||
|
|
||
| /// Returns a constraint set that always holds | ||
| fn always_satisfiable(db: &'db dyn Db) -> Self; |
There was a problem hiding this comment.
In terms of readability, I think I weakly prefer the old spelling, actually! ConstraintSet::unsatisfiable() seems clearer to me about what it means than ConstraintSet::from(false), even if the constructor is strictly-speaking redundant with the new From<bool> implementation. Same for ConstraintSet::always_satisfiable.
| /// Updates this constraint set to hold the union of itself and another constraint set. | ||
| pub(crate) fn union(&mut self, db: &'db dyn Db, other: Self) -> &Self { | ||
| self.union_set(db, other); | ||
| self | ||
| } | ||
|
|
||
| /// Updates this constraint set to hold the intersection of itself and another constraint set. | ||
| pub(crate) fn intersect(&mut self, db: &'db dyn Db, other: &Self) -> &Self { | ||
| self.intersect_set(db, other); | ||
| self | ||
| } |
There was a problem hiding this comment.
I'm happy to genuflect to Clippy here!
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This PR removes the
Constraintstrait. We removed theboolimplementation several weeks back, and are usingConstraintSeteverywhere. There have been discussions about trying to include the reason for an assignability failure as part of the result, but that there are no concrete plans to do so soon, and it's not clear that we'll need theConstraintstrait to do that. (We can ideally just update theConstraintSettype directly.)In the meantime, this just complicates the code for no good reason.
This PR is a pure refactoring, and contains no behavioral changes.