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

[red-knot] T | object == object #16088

Merged
merged 2 commits into from
Feb 10, 2025
Merged

[red-knot] T | object == object #16088

merged 2 commits into from
Feb 10, 2025

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Feb 10, 2025

Summary

  • Simplify unions with object to object.
  • Add a new Type::object(db) constructor to abbreviate KnownClass::Object.to_instance(db) in some places.
  • Add a Type::is_object and Class::is_object function to make some tests for a bit easier to read.

Happy to revert the latter two changes if these are not desirable.

closes #16084

Test Plan

New Markdown tests.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Feb 10, 2025
Type::Never => {}
// Adding `object` to a union results in `object`.
ty if ty.is_object(self.db) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be

Suggested change
ty if ty.is_object(self.db) => {
Type::Instance(instance) if instance.class.is_object(self.db) => {

but I somehow doubt that it would be much faster(?)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 class.known of the InstanceType and eliminate the if entirely? But in the absence of evidence that that is faster, it just looks more verbose.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines +46 to +51
/// 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
}
Copy link
Contributor Author

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 UnionBuilder to keep track of the fact that it represented object. 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 with object a lot in reality.

Copy link
Contributor

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.

@sharkdp sharkdp merged commit 0019d39 into main Feb 10, 2025
21 checks passed
@sharkdp sharkdp deleted the david/unions-with-object branch February 10, 2025 22:07
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

dcreager added a commit that referenced this pull request Feb 11, 2025
* main:
  add diagnostic `Span` (couples `File` and `TextRange`) (#16101)
  Remove `Hash` and `Eq` from `AstNodeRef` for types not implementing `Eq` or `Hash` (#16100)
  Fix release build warning about unused todo type message (#16102)
  [`pydocstyle`] Handle arguments with the same names as sections (`D417`) (#16011)
  [red-knot] Reduce usage of `From<Type>` implementations when working with `Symbol`s (#16076)
  Transition to salsa coarse-grained tracked structs (#15763)
  [`pyupgrade`] Handle micro version numbers correctly (`UP036`) (#16091)
  [red-knot] `T | object == object` (#16088)
  [`ruff`] Skip singleton starred expressions for `incorrectly-parenthesized-tuple-in-subscript` (`RUF031`) (#16083)
  Delete left-over `verbosity.rs (#16081)
  [red-knot] User-level configuration (#16021)
  Add `user_configuration_directory` to `System` (#16020)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[red-knot] T | object = object
3 participants