-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] T | object == object
#16088
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,13 @@ impl<'db> UnionBuilder<'db> { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Collapse the union to a single type: `object`. | ||||||
| fn collapse_to_object(mut self) -> Self { | ||||||
| self.elements.clear(); | ||||||
| self.elements.push(Type::object(self.db)); | ||||||
| self | ||||||
| } | ||||||
|
|
||||||
| /// Adds a type to this union. | ||||||
| pub(crate) fn add(mut self, ty: Type<'db>) -> Self { | ||||||
| match ty { | ||||||
|
|
@@ -53,7 +60,12 @@ impl<'db> UnionBuilder<'db> { | |||||
| self = self.add(*element); | ||||||
| } | ||||||
| } | ||||||
| // Adding `Never` to a union is a no-op. | ||||||
| Type::Never => {} | ||||||
| // Adding `object` to a union results in `object`. | ||||||
| ty if ty.is_object(self.db) => { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could also be
Suggested change
but I somehow doubt that it would be much faster(?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wondered the same thing; I think it could be unpacked even further to match against |
||||||
| return self.collapse_to_object(); | ||||||
| } | ||||||
| _ => { | ||||||
| let bool_pair = if let Type::BooleanLiteral(b) = ty { | ||||||
| Some(Type::BooleanLiteral(!b)) | ||||||
|
|
@@ -76,7 +88,10 @@ impl<'db> UnionBuilder<'db> { | |||||
| break; | ||||||
| } | ||||||
|
|
||||||
| if ty.is_same_gradual_form(*element) || ty.is_subtype_of(self.db, *element) { | ||||||
| if ty.is_same_gradual_form(*element) | ||||||
| || ty.is_subtype_of(self.db, *element) | ||||||
| || element.is_object(self.db) | ||||||
| { | ||||||
| return self; | ||||||
| } else if element.is_subtype_of(self.db, ty) { | ||||||
| to_remove.push(index); | ||||||
|
|
@@ -88,9 +103,7 @@ impl<'db> UnionBuilder<'db> { | |||||
| // `element | ty` must be `object` (object has no other supertypes). This means we can simplify | ||||||
| // the whole union to just `object`, since all other potential elements would also be subtypes of | ||||||
| // `object`. | ||||||
| self.elements.clear(); | ||||||
| self.elements.push(KnownClass::Object.to_instance(self.db)); | ||||||
| return self; | ||||||
| return self.collapse_to_object(); | ||||||
| } | ||||||
| } | ||||||
| match to_remove[..] { | ||||||
|
|
@@ -416,7 +429,7 @@ impl<'db> InnerIntersectionBuilder<'db> { | |||||
| Type::Never => { | ||||||
| // Adding ~Never to an intersection is a no-op. | ||||||
| } | ||||||
| Type::Instance(instance) if instance.class.is_known(db, KnownClass::Object) => { | ||||||
| Type::Instance(instance) if instance.class.is_object(db) => { | ||||||
| // Adding ~object to an intersection results in Never. | ||||||
| *self = Self::default(); | ||||||
| self.positive.insert(Type::Never); | ||||||
|
|
@@ -481,7 +494,7 @@ impl<'db> InnerIntersectionBuilder<'db> { | |||||
|
|
||||||
| fn build(mut self, db: &'db dyn Db) -> Type<'db> { | ||||||
| match (self.positive.len(), self.negative.len()) { | ||||||
| (0, 0) => KnownClass::Object.to_instance(db), | ||||||
| (0, 0) => Type::object(db), | ||||||
| (1, 0) => self.positive[0], | ||||||
| _ => { | ||||||
| self.positive.shrink_to_fit(); | ||||||
|
|
@@ -534,7 +547,7 @@ mod tests { | |||||
| let db = setup_db(); | ||||||
|
|
||||||
| let intersection = IntersectionBuilder::new(&db).build(); | ||||||
| assert_eq!(intersection, KnownClass::Object.to_instance(&db)); | ||||||
| assert_eq!(intersection, Type::object(&db)); | ||||||
| } | ||||||
|
|
||||||
| #[test_case(Type::BooleanLiteral(true))] | ||||||
|
|
@@ -548,7 +561,7 @@ mod tests { | |||||
| // We add t_object in various orders (in first or second position) in | ||||||
| // the tests below to ensure that the boolean simplification eliminates | ||||||
| // everything from the intersection, not just `bool`. | ||||||
| let t_object = KnownClass::Object.to_instance(&db); | ||||||
| let t_object = Type::object(&db); | ||||||
| let t_bool = KnownClass::Bool.to_instance(&db); | ||||||
|
|
||||||
| let ty = IntersectionBuilder::new(&db) | ||||||
|
|
||||||
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.
I also thought about extending the
UnionBuilderto keep track of the fact that it representedobject. It could then short-circuit any.add(…)into a no-op. Not sure if it's worth the extra complexity though, as we probably don't union withobjecta lot in reality.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.
This is the sort of thing IMO best left to testing with actual benchmarks once we ideally have a broader ecosystem check that we can also benchmark.