Skip to content

Comments

[red-knot] type[T] is disjoint from type[S] if the metaclass of T is disjoint from the metaclass of S#15547

Merged
AlexWaygood merged 2 commits intomainfrom
alex/final-type-t
Jan 17, 2025
Merged

[red-knot] type[T] is disjoint from type[S] if the metaclass of T is disjoint from the metaclass of S#15547
AlexWaygood merged 2 commits intomainfrom
alex/final-type-t

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 17, 2025

Summary

This PR further extends the principles established in #15539 -- and allows us to get rid of even more special cases for type[] types in Type::is_disjoint_from()! A class definition class A(B, C): ... can only succeed if either the metaclass of C is a subclass of the metaclass of B or the metaclass of B is a subclass of the metaclass of C. As such, for two types type[T] and type[S], we can conclude that the two types are disjoint if T's metaclass and S's metaclass are disjoint.

Test Plan

  • New mdtests added
  • Ran QUICKCHECK_TESTS=1000000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stable

@sharkdp -- we don't have great coverage for this kind of thing in our property tests, because I know of no standard-library classes that have @final metaclasses, so it's hard for me to think of how to get the property tests to use some type[] types that have disjoint metaclasses. Maybe that just means that this kind of thing is an edge case that we don't need to worry about too much, though 🤷

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Jan 17, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) January 17, 2025 10:40
@AlexWaygood AlexWaygood merged commit 4328df7 into main Jan 17, 2025
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/final-type-t branch January 17, 2025 10:41
@github-actions
Copy link
Contributor

github-actions bot commented Jan 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@sharkdp
Copy link
Contributor

sharkdp commented Jan 17, 2025

@sharkdp -- we don't have great coverage for this kind of thing in our property tests, because I know of no standard-library classes that have @final metaclasses, so it's hard for me to think of how to get the property tests to use some type[] types that have disjoint metaclasses. Maybe that just means that this kind of thing is an edge case that we don't need to worry about too much, though 🤷

Right. I think we might be able to write a custom file to the in-memory FS of the TestDB that would contain a couple of bespoke types that we could then import into the property tests? Might be a bit more difficult due to the Ty layer in between, though.

I've been thinking about changing the input to the property tests for another reason. We could add a layer like

pub enum PropertyTestType {
    Ty(Ty),
    CustomTypeWithFinalMetaclass,
    # …
}

and use that as the input to the property tests. That would have the added benefit that we could write a custom Display (or Debug?) implementation for this in order to get a better output when a property test fails. Instead of showing the Ty representation, we could potentially render the nicer Type Display output that would also contain initial simplifications in case of union types. So instead of showing Ty::union([int,str, Literal[1]]), we could show int | str.

carljm
carljm reviewed Jan 17, 2025
View reviewed changes
crates/red_knot_python_semantic/src/types/subclass_of.rs
/// so the type `type[T]` is a subtype of the instance type `U`.
pub(crate) fn as_instance_type_of_metaclass(&self, db: &'db dyn Db) -> Type<'db> {
match self.subclass_of {
ClassBase::Dynamic(_) => KnownClass::Type.to_instance(db),
Copy link
Contributor

@carljm carljm Jan 17, 2025

Choose a reason for hiding this comment

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

I think this line is OK as-is, as far as this method is used in this PR, because our semantics of non-fully-static types in is_disjoint_from is that we treat them as equivalent to their widest possible type (that is, Any and object are treated the same by is_disjoint_from, in being disjoint from nothing.) But if this method were to be used in other contexts in future, we need to be a little careful about this line, it may not be the right thing; we should maybe return Dynamic instead.

Copy link
Member Author

@AlexWaygood AlexWaygood Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, good point. In my first version of this PR I used this method from multiple places (which was why I created at method)... but it looks like now it's only used in a single location... So maybe I should just inline it at that single location (like it was prior to this PR)?

Copy link
Contributor

@carljm carljm Jan 17, 2025

Choose a reason for hiding this comment

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

Yeah, that might be better, in that it would reduce the risk of reusing this logic where this isn't the right handling of it.

@AlexWaygood AlexWaygood mentioned this pull request Jan 17, 2025
Merged
dcreager added a commit that referenced this pull request Jan 17, 2025
@dcreager
Merge branch 'main' into dcreager/generate-ast
01bac61 
* main:
  [red-knot] Inline `SubclassOfType::as_instance_type_of_metaclass()` (#15556)
  [`flake8-comprehensions`] strip parentheses around generators in `unnecessary-generator-set` (`C401`) (#15553)
  [`pylint`] Implement `redefined-slots-in-subclass` (`W0244`) (#9640)
  [`flake8-bugbear`] Do not raise error if keyword argument is present and target-python version is less or equals than 3.9 (`B903`) (#15549)
  [red-knot] `type[T]` is disjoint from `type[S]` if the metaclass of `T` is disjoint from the metaclass of `S` (#15547)
  [red-knot] Pure instance variables declared in class body (#15515)
  Update snapshots of #15507 with new annotated snipetts rendering (#15546)
  [`pylint`] Do not report methods with only one `EM101`-compatible `raise` (`PLR6301`) (#15507)
  Fix unstable f-string formatting for expressions containing a trailing comma (#15545)
  Support `knot.toml` files in project discovery (#15505)
  Add support for configuring knot in `pyproject.toml` files (#15493)
  Fix bracket spacing for single-element tuples in f-string expressions (#15537)
  [`flake8-simplify`] Do not emit diagnostics for expressions inside string type annotations (`SIM222`, `SIM223`) (#15405)
  [`flake8-pytest-style`] Do not emit diagnostics for empty `for` loops (`PT012`, `PT031`) (#15542)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments

@MichaReiser MichaReiser Awaiting requested review from MichaReiser

@sharkdp sharkdp Awaiting requested review from sharkdp

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexWaygood @sharkdp @carljm