Use checkmember.py to check multiple inheritance #18876
Conversation
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas (https://github.com/pandas-dev/pandas)
- pandas/core/series.py:4818: error: Unused "type: ignore" comment [unused-ignore]
- pandas/core/indexes/range.py:1382: error: Unused "type: ignore" comment [unused-ignore]
hydpy (https://github.com/hydpy-dev/hydpy)
+ hydpy/models/evap_pet_ambav1.py:382: error: Definition of "prepare_leafareaindex" in base class "Sub_ETModel" is incompatible with definition in base class "PETModel_V2" [misc]
+ hydpy/models/evap_pet_ambav1.py:382: error: Definition of "prepare_plant" in base class "Sub_ETModel" is incompatible with definition in base class "PETModel_V2" [misc]
+ hydpy/models/evap_aet_morsim.py:500: error: Definition of "prepare_leafareaindex" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
+ hydpy/models/evap_aet_morsim.py:500: error: Definition of "prepare_plant" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
+ hydpy/models/evap_aet_minhas.py:268: error: Definition of "prepare_leafareaindex" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
+ hydpy/models/evap_aet_minhas.py:268: error: Definition of "prepare_plant" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
+ hydpy/models/evap_aet_hbv96.py:297: error: Definition of "prepare_leafareaindex" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
+ hydpy/models/evap_aet_hbv96.py:297: error: Definition of "prepare_plant" in base class "Sub_ETModel" is incompatible with definition in base class "AETModel_V1" [misc]
spark (https://github.com/apache/spark)
+ python/pyspark/ml/regression.py:1599: error: Definition of "getNumTrees" in base class "_TreeEnsembleModel" is incompatible with definition in base class "_RandomForestParams" [misc]
+ python/pyspark/ml/classification.py:2271: error: Definition of "getNumTrees" in base class "_TreeEnsembleModel" is incompatible with definition in base class "_RandomForestParams" [misc]
|
|
New errors in def prepare_plant(self, tree: VectorInputBool) -> None: ...with def prepare_plant(self, plant: VectorInputBool) -> None: ...here https://github.com/hydpy-dev/hydpy/blob/master/hydpy/interfaces/petinterfaces.py#L79, and similarly in few other methods. @JukkaL I think this is ready for review now. |
| ok = is_equivalent(first_type, second_type) | ||
| if not ok: | ||
| second_node = base2[name].node | ||
| if second.node is not None and not self.is_writable_attribute(second.node): |
There was a problem hiding this comment.
This very condition is used as part of the next block - maybe extract to second_is_writeable?
There was a problem hiding this comment.
They are not the same, one has not other doesn't have one.
| ok = is_subtype(first_type, second_type) | ||
| else: | ||
| ok = is_equivalent(first_type, second_type) | ||
| if ok: | ||
| if ( |
There was a problem hiding this comment.
Maybe merge the ladder? if (ok and ...), nested ifs are no longer necessary here
There was a problem hiding this comment.
This is actually intentional, to make it more prominent that we only want to give at most one error here (without a comment).
| and self.is_writable_attribute(second.node) | ||
| and is_property(first.node) | ||
| and isinstance(first.node, Decorator) | ||
| and not isinstance(p_second_type, AnyType) |
There was a problem hiding this comment.
I'm not sure I understand this bit - why should read-only something be compatible with writable Any? Being writable is somewhat more fundamental than a specific type of the attr IMO. Is this behavior mentioned in the spec?
There was a problem hiding this comment.
If the type is Any, we can't always know for sure what the attribute is (maybe it is e.g. an alias to unimported property). Also we have a lot of other places where Any affects "arity", including an equivalent check for single inheritance.
(Also TBH I don't really care about the spec, if someone wants me to change something, they need to convince me, not the other way around.)
| and second.node | ||
| and self.is_writable_attribute(second.node) | ||
| and is_property(first.node) | ||
| and isinstance(first.node, Decorator) |
There was a problem hiding this comment.
Ough, and this one. A property can be an OverloadedFuncDef and still not represent a writable property, does the following copy of an existing test pass?
[case testMixinTypedPropertyReversedWithDel]
class A:
@property
def foo(cls) -> str:
return "no"
@foo.deleter
def foo(cls) -> None: ...
class Mixin:
foo = "foo"
class C(A, Mixin): # E: Cannot override writeable attribute "foo" in base "Mixin" with read-only property in base "A"
pass
[builtins fixtures/property.pyi]
There was a problem hiding this comment.
Again, this is intentional (see PR description), I didn't want this to be stricter that the equivalent single inheritance check. I have an action item to revisit this after I am done with the core of the refactoring, and "straighten" all the writability checks.
Yes, these are definitely simple copy-paste errors. I would have expected Mypy to detect them already, given that the "inner" |
This is the third "major" PR towards #7724
This one is mostly straightforward. I tried to preserve the existing logic about mutable overrides (to minimize fallout), as currently we e.g. don't use the covariant mutable override error code here. In future we can separately "synchronize" mutable override logic across variable override, method override, and multiple inheritance code paths (as currently all three are subtly different).