-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Mixins with conflicting signatures #2125
Comments
Sounds reasonable, but I haven't thought it through much. What if either
load() method were to use super()?
|
@gvanrossum I think that should be OK, right? We would assume the signature we're dispatching to, right? It's definitely not a great practice as it breaks the liskov substitution principle ( I think it would make sense to use the same logic as mypy does for inheritance. For example, in the below code example mypy is OK with class Base(object):
def load(self, payload: str) -> None:
pass
class OKSub(Base):
def load(self, payload: str, something_else=False) -> None:
pass
class BadSub(Base):
def load(self, payload: int, something_else=False) -> None:
pass |
OK, you've convinced me. Unless @JukkaL disagrees we can do this.
|
Mypy treats inheritance as subtyping. In the original example,
See also python/typing#246 and python/typing#241, which are related. |
@JukkaL Totally....This would be a case of weakening the type checker to support existing code. I don't think this is a great practice for the reasons you highlighted, but I've found it to be quite a common pattern in existing code bases. This seems to more arise from using Mixins for composition and code sharing as opposed to using Mixins to express a type hierarchy. In the example above, it's safe to say |
It still wouldn't safe in general. Consider this example: class A(object):
def load(self, payload):
pass
class B(object):
def load(self, payload, return_header=False):
pass
def load2(self, payload):
self.load(payload, return_header=True)
class C(A, B):
pass
C().load2(stuff) # ouch I'm not convinced that mypy should silently accept such code, but it would be nice if there was a documented way of using the idiom with mypy. Here are some possibilities:
The |
Using # type: ignore feels fine to me, but perhaps we could add one tweak:
invent a feature whereby you can ignore a specific error message on a given
line, rather than just ignoring everything on that line. This would require
us to number our error messages though...
|
This mostly adds mypy to the tox process to enable checking of any types added (and code it understands). This will allow type hints to be progressively added to the Werkzeug codebase, with the eventual aim of turning on disallow_untyped_defs. Type annotations are enforced as is the convention of only importing typing required objects within a `if TYPE_CHECKING:` block. This should limit the import time cost of type hinting. Note that, - mypy doesn't understand conditional imports python/mypy#1153 - mypy insists mixins with different kwarg signatures are errors, python/mypy#2125 (Response)
This mostly adds mypy to the tox process to enable checking of any types added (and code it understands). This will allow type hints to be progressively added to the Werkzeug codebase, with the eventual aim of turning on disallow_untyped_defs. Type annotations are enforced as is the convention of only importing typing required objects within a `if TYPE_CHECKING:` block. This should limit the import time cost of type hinting. Note that, - mypy doesn't understand conditional imports python/mypy#1153 - mypy insists mixins with different kwarg signatures are errors, python/mypy#2125 (Response)
@hauntsaninja Would you please elaborate on the reasons to close this issue? Is it not present anymore? |
The resolution here seemed to be that this is unsafe in general, and type ignore is the best option here, especially if you could type ignore a specific message. mypy now supports type ignore with error codes. Also Protocols are a thing now |
I'm finding it's quite common to have Python Libraries using mixins with signatures that mypy considers to be conflicting because one takes a different set of kwargs than the other. For example - https://github.com/pallets/itsdangerous/blob/master/itsdangerous.py#L881-L886 (which is simplified below).
In this case,
mypy
will complain thatA
andB
have conflicting signatures forload
. In practice, Python will always dispatchload
calls toA
soC
's signature forload
should beload(self, payload)
. It would be incorrect to passreturn_header
toC().load
.What are your thoughts on having
mypy
reflect what Python will do at runtime here and just take the definition ofA
'sload
instead of throwing an error? The only way around this currently is to add**kwargs
toA
s signature which is less safe.The text was updated successfully, but these errors were encountered: