[red-knot] Allow subclasses of Any to be assignable to Callable types
#17717
Conversation
Kalmaegi
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
|
There was a problem hiding this comment.
Thanks for the PR!
As @sharkdp mentioned on the issue, there should be some exceptions here. One of those exceptions (subclass of Any can't be a subclass of a final class) we already have a test for, which is causing the tests to fail on this PR. I think all literal types should also be excluded (an instance of an Any subclass can't possibly inhabit Literal[1] or any other literal type).
I am not 100% sure that a separate catchall check at the top of the function will still be the clearest implementation here, once we account for all the exceptions. It depends on the balance of how many types we have to carve out of this catchall, vs how many we'd have to add a dedicated check for inheritance-from-Any to.
And yes, Python code samples should be added to the mdtests in this PR demonstrating the cases we want to handle that were not previously handled.
I checked for existing tests and found that assignments from Any to both Final and Literal are already covered. I just added a test for the Callable case. |
Where do we cover assignments to literals? It looks like it is possibly to assign to a literal on your branch. You will need a test that makes sure that instances of from typing import Any, Literal
class C(Any):
pass
x: Literal[1] = C() # this should fail, but does not on your branch.As @carljm mentioned, it might be better to follow a "opt in" strategy, i.e. to handle special cases to the existing match statement. This way, we make sure that we exhaustively handle all type variants; and that we properly treat union and intersection types. For example: from typing import final, Any
@final
class F1: ...
@final
class F2: ...
class Subclass(Any): ...
f: F1 | F2 = Subclass() # this should fail, but does not on your branch. |
Ah, sorry about that—I misread the test. I’ll update the code accordingly. |
sharkdp
force-pushed
the
fix-any-subclass-callable
branch
from
af5630d to
46d6758
Compare
sharkdp
force-pushed
the
fix-any-subclass-callable
branch
from
46d6758 to
7fa5767
Compare
There was a problem hiding this comment.
@Kalmaegi I didn't really see any evidence in the newly added tests here that we need the catch-all case. In fact, there were some new ecosystem false-positives that looked incorrect (see below). It might be that we're still missing some cases where we should allow assignability, but until then, I'd rather be conservative and only allow assignability to Callable specifically. I changed the code to reflect this, hope that's fine for you. These false positives are now gone after the update:
pytest-robotframework (https://github.com/detachhead/pytest-robotframework)
+ error[lint:invalid-argument-type] pytest_robotframework/_internal/pytest/plugin.py:374:26: Argument to this function is incorrect: Expected `PytestRuntestProtocolHooks`, found `PytestRuntestProtocolHooks`
- Found 322 diagnostics
+ Found 323 diagnostics
mypy-protobuf (https://github.com/dropbox/mypy-protobuf)
+ error[lint:invalid-argument-type] mypy_protobuf/main.py:134:64: Argument to this function is incorrect: Expected `FileDescriptorProto`, found `FileDescriptorProto`
+ error[lint:invalid-argument-type] mypy_protobuf/main.py:135:59: Argument to this function is incorrect: Expected `FileDescriptorProto`, found `FileDescriptorProto`
+ error[lint:invalid-argument-type] mypy_protobuf/main.py:941:22: Argument to this function is incorrect: Expected `FieldDescriptorProto`, found `FieldDescriptorProto`Thank you for working on this.
sharkdp
changed the title
red-knot] Allow subclasses of Any to be assignable to all typesred-knot] Allow subclasses of Any to be assignable to Callable types
@sharkdp Thank you so much for your help. I'm still familiarizing myself with the project structure, so some of my ideas might be a bit rudimentary. |
* main: Improve messages outputted by py-fuzzer (#17764) [`red-knot`] Allow subclasses of Any to be assignable to Callable types (#17717) [red-knot] Increase durability of read-only `File` fields (#17757) [red-knot] Cache source type during semanic index building (#17756) [`flake8-use-pathlib`] Fix `PTH116` false positive when `stat` is passed a file descriptor (#17709) Sync vendored typeshed stubs (#17753)
Summary
Fixes #17701. I'm uncertain whether this Python code needs to be added to the test cases. I'd be happy to supplement it if needed.