-
-
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
Why is "incompatible with supertype" an error? #1237
Comments
It violates the Liskov Substitution Principle. You may want to Google that. In particular the problem is that if we have a Foo instance and we call its factory() method, the typechecker expects that it returns a str; but when that Foo instance is actually a Bar instance, this assumption is incorrect. Note that |
On 24 Feb 2016, at 7:32 pm, Guido van Rossum [email protected] wrote:
What does that mean regarding the effectivness of mypy on code that exhibits that sort of behaviour? Is it still useful? My actual use case is in stub files describing a C++ API wrapped in Python. factory() is non-virtual in both Foo and Bar - they are completely unrelated and just happen to have the same name. Is there a case for treating stub files differently, maybe by providing something like @overload that tells mypy that Bar.factory() is not an override of Foo.factory()? Phil |
You can also use |
Python doesn't have the concept of non-virtual functions, so what you're suggesting is unfortunately unsound. For example, you could have some function: def f(x: Foo) -> str:
return x.factory() Because of the way subtyping relationships work in mypy, it's valid to pass a |
At the moment I'm using mypy to validate stub files I am generating. I'm not sure if this error is saying that, in an otherwise correct stub file, mypy can't do its job properly? Or is it saying that the stub file is incorrect no matter what tool is using it? |
Phil, does the real code where you hit this happen to use Alternatively (if you don't like the Regarding whether the stub files are valid or not, I think that a stub file containing the 6 lines of code you showed us should be considered objectively invalid. Whether you can read that in the letter of PEP 484 is a different issue. |
Let me try to clarify some more. Suppose we have a file a.py like this:
And suppose we have b.py:
There is a type error here, since at run time Perhaps the best solution really is to make it so that Bar is not a subclass of Foo. Is that reasonable? |
On 24 Feb 2016, at 10:53 pm, Guido van Rossum [email protected] wrote:
In some cases but not all.
I have no control over the API I am trying to describe. I don't like #type because (I'm guessing) other tools (like an IDE using the stub files to create auto-completion lists) would also ignore it.
The following is from my real stub files... class QPixmap(QPaintDevice): class QBitmap(QPixmap): ...as far I understand PEP 484 that is the correct way to describe the API in a stub file. If you are saying that it is not possible to create a PEP 484-compliant stub file for this API the I would suggest that PEP 484 isn't much use for real applications. Even if the stub file is PEP 484-compliant, but the API makes it difficult for mypy to do the type checking it wants to, then it should still handle it more gracefully than it does at the moment. The stub files are for PyQt so, as it stands, mypy cannot be used on PyQt applications. I don't see PyQt as being special in this regard, I imagine that there could be many 3rd-party libraries that break these rules, particularly if they wrap C++ libraries. |
Understood. Do you have control over the stub generating tool?
Well, PyCharm is sprinting to support all of PEP 484, and their latest (5.1-EAP) even supports the Python 2.7 signature type comments (e.g. Of course I don't know what your favorite IDE is or what its support plan is for any of this. Regarding this example: class QPixmap(QPaintDevice):
def swap(self, other: 'QPixmap') -> None: ...
class QBitmap(QPixmap):
def swap(self, other: 'QBitmap') -> None: ... That's a slightly different situation again. Here it seems you have arguments that vary covariantly, which goes against the "Liskov" model that I explained in an earlier comment here. Interestingly, while most languages (and PEP 484*) declare this invalid, in Eiffel this is actually valid (and they insert a runtime check in case a QBitmap down-casted to a QPixmap is called with a QPixmap argument, since the compiler doesn't catch that error).
That came across unnecessarily harsh (and doesn't match my experience at Dropbox, where we are successfully introducing it to what I consider very much a "real application".) I would like to propose two ways forward -- a short-term way and a long-term approach. In the short term, I think you can solve this by adding a few well-aimed In the longer term, I think we can add a directive to PEP 484 to accommodate your use case. For example, if we were to let you write from typing import covariant_args
class QPixmap... # unchangd
class QBitmap(QPixmap):
@covariant_args
def swap(self, other: 'QBitmap') -> None: ... That should allow you to choose the Eiffel model over the Liskov model for a specific method. Or if you have use cases where the arguments are just entirely different we could come up with some other decorator name.
|
Side note: as a temporary fix, you can set the retuen type of both |
On 26 Feb 2016, at 9:36 pm, Guido van Rossum [email protected] wrote:
Complete control - it's what I am trying to test.
I think you misunderstood. I'm not concerned that an IDE would not recognise the ignore directive - quite the opposite. I'm concerned that it would recognise it and omit the method from generated auto-completion lists.
Apologies - wait till you get to the PyQt parts of your application :)
Do you have a contact on the PyCharm development so I can ask them what works best for them. In this context I'm a supplier of stub files, not a user.
My problem with that is that it seems too specific - there may be lots of other ways that the stub files are invalid that I haven't discovered yet. Assuming that stub files will be used for more than mypy-like type checking (eg. auto-completion lists) then having something @ignore(reason) with a standard set of reasons might work. Different tools could then selectively ignore a signature.
From reading the PEP I don't get any feeling that it's only intended to cover a conforming sub-set of Python code. I read it and think here is a specification for describing any Python API in a standard way. |
My contact at PyCharm is Andrey Vlasovskikh. You can probably reach him by calling out to him in the tracker here: https://github.com/python/typing/issues (he's subscribed to all issues there) or in the PyCharm tracker at youtrack.jetbrains.com. I'll try to address the rest later. |
It's not possible to describe all possible valid Python API's statically, except perhaps with an exceedingly complex type system. For example, you could have some function: def f(x: int) -> ???:
if x < 0: return 0
else: return "hello" and call it like this: print(f(5) + " world") This program will execute without errors, but the only way to type it properly is with dependent types, which are rather difficult to understand/work with. |
On 27 Feb 2016, at 12:39 am, David Fisher [email protected] wrote:
Understood - if the context is restricted to type checking - but there may be other contexts (eg. generating boilerplate documentation) where ??? is Union[int, str] is a useful piece of information. |
There's a distinction here that can be useful and relevant which is subclassing vs subtyping. By subclass I mean "making A a subclass of B gives A all the code from B unless overridden". By subtype I mean Liskov substitution "A is a subtype of B when I can use an A to anything declared of type B". In many languages, both things happen at the same type. One of the nice things of Python and dynamic languages is that you can have subtyping with no subclassing (i.e., duck-typing: if you implement the same interface you can implicitly have Liskov substitution). Something that has been done (also in Eiffel, but unrelated to the covariance scenario above) is having subclassing without subtyping. You can have a subclass to get all the method definitions from the parent, but the new class is not a subtype of the other. In that case you're applying inheritance for code reuse, not for Liskov substitution. They call it "implementation inheritance" In this scenario, the original code could be accepted, but QBitmap wouldn't be a class of QPixmap. So the following happens: p1 = QPixmap(); p2 = QPixmap()
b1 = QBitmap(); b2 = QBitmap()
lp = [] # type: List[QPixmap]
lb = [] # type: List[QBitmap]
lp.append(p1) # this is ok
lp.append(b1) # this is rejected, a bitmap is not a pixmap even with the subclassing
lb.append(b1) # this is ok
lp[0].swap(p2) # OK
lb[0].swap(p2) # wrong, swap of a pixmap with a bitmap The main point is that I'm quite sure that there are probably many pieces of Python around that use subclassing but where Liskov substitution was not required nor intended, so probably it shouldn't be demanded. If you agree with this, probably it makes sense to force the programmers to make explicit that they actually want implementation inheritance. For example class QBitmap(Implementation[QPixmap]):
def swap(self, other: 'QBitmap') -> None: ... once you make explicit that you inherit implementation you can change types in any direction (covariant, contravariant, change number of arguments, etc) |
That's a good way to frame it. I would recommend a class decorator to indicate this. We should move this discussion to https://github.com/python/typing as a proposal to add such a class decorator to PEP 484 and typing.py. |
OK, I'll create an issue there (I didn't see one) |
I think that implementation inheritance can be type checked by re-type-checking the bodies of all base class methods as if they were written in the subclass (after translating type variables suitably if the base class is generic). |
@JukkaL that should be right (and that's actually how they were implemented in the Eiffel compiler I used to work in) I added python/typing#241 to continue the discussion |
Thank you all! Concluding:
Contra-variant behavior of method arguments with Union is best illustrated: class Field:
def to_python(self, value: Union[str, int]): ...
class MultiValueField:
def to_python(self, value: Union[str, int, List]): ... # OK Even though it feels natural that the base class should be less specific, one must actually think that the specialized class is capable of handling more. |
Is there any way currently to extend this property to other methods on demand? The repository i'm trying to type has a pattern of At the moment I can bypass via |
|
So if i silence an error at the function definition, i'll still get type errors if I use that function in a way that doesn't respect the definition elsewhere? |
Yes. |
Firstly, it's worth noting that this only ignores the typing error. Things that call these APIs will still be checked! The problem is that the checking may be incorrect because of the API itself. There's a long discussion about this and related Liskov issues at: python/mypy#1237 The short version is that this API is wrong-ish, but there's no mechanism within mypy to correctly annotate what's going on here. There *is* some of this around, because mypy treats __init__ and __new__ differently itself, but we can't apply that treatment to our constructors. This would be better if we actually knew which methods were constructors, instead of the name-based guessing here... but that's way too much complexity for me right now.
I don't think that there is anything actionable here. It's now possible to use |
Why does mypy consider...
to be an error? While it may be poor design, it isn't wrong.
Thanks,
Phil
The text was updated successfully, but these errors were encountered: