Fix type checking decorated method overrides #3918
Conversation
The fix turned out to be pretty complicated, as there were a bunch of untested method overriding scenarios which weren't quite right. I fixed any related issues that I encountered, though I'm not certain whether some of the issues were hidden previously by other bugs. I also added tests for some related, previously untested scenarios. Fixes #1441.
|
This is ready for review, but don't merge this yet -- I have some outstanding fixes to internal Dropbox code bases that I want to land first before merging this. |
|
It looks like you didn't update all tests, now all types are wrapped in quotes |
There was a problem hiding this comment.
Not a real review, just two random comments. Also this reminds me that there is an old related PR #3263, @kirbyfan64 are you still working on it?
mypy/checker.py
Outdated
| elif isinstance(original_type, AnyType): | ||
| context) | ||
| elif is_equivalent(original_type, typ): | ||
| # Assume invariance for a non-callable attribute here. |
There was a problem hiding this comment.
Wait, so does this PR also fix #3208?
There was a problem hiding this comment.
No -- this only affects decorators that return a non-callable value (but @property is unaffected). This is a rare edge case. This also means that this will have only minor backward compatibility issues.
mypy/checker.py
Outdated
| elif is_equivalent(original_type, typ): | ||
| # Assume invariance for a non-callable attribute here. | ||
| # | ||
| # TODO: Allow covariance for read-only attributes? |
There was a problem hiding this comment.
FWIW, protocols currently allow covariant overrides of read-only attributes (like @property).
There was a problem hiding this comment.
@property is special and it can be overridden covariantly even outside protocols. I'll update the comment to mention that this doesn't affect @property.
There was a problem hiding this comment.
What about writable properties?
mypy/checker.py
Outdated
| elif is_equivalent(original_type, typ): | ||
| # Assume invariance for a non-callable attribute here. | ||
| # | ||
| # TODO: Allow covariance for read-only attributes? |
There was a problem hiding this comment.
What about writable properties?
These should be invariant, similar to regular attributes. |
The fix turned out to be pretty complicated, as there were a bunch of
untested method overriding scenarios which weren't quite right. I
fixed any related issues that I encountered, though I'm not certain
whether some of the issues were hidden previously by other bugs. I
also added tests for some related, previously untested scenarios.
Fixes #1441.