-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] type[T]
is disjoint from type[S]
if the metaclass of T
is disjoint from the metaclass of S
#15547
Conversation
…T` is disjoint from the metaclass of `S`
3351b2e
to
37f892e
Compare
37f892e
to
0ca4bff
Compare
|
Right. I think we might be able to write a custom file to the in-memory FS of the I've been thinking about changing the input to the property tests for another reason. We could add a layer like
and use that as the input to the property tests. That would have the added benefit that we could write a custom |
/// 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), |
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 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.
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.
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)?
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.
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.
* 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)
Summary
This PR further extends the principles established in #15539 -- and allows us to get rid of even more special cases for
type[]
types inType::is_disjoint_from()
! A class definitionclass A(B, C): ...
can only succeed if either the metaclass ofC
is a subclass of the metaclass ofB
or the metaclass ofB
is a subclass of the metaclass ofC
. As such, for two typestype[T]
andtype[S]
, we can conclude that the two types are disjoint ifT
's metaclass andS
's metaclass are disjoint.Test Plan
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 sometype[]
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 🤷