[ty] Understand classes that inherit from subscripted Protocol[] as generic
#17832
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
2 times, most recently
from
be076b3 to
0a17ac2
Compare
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
0a17ac2 to
d08b776
Compare
|
I obviously need to investigate the panic found in mypy_primer, but apart from that I think this is ready for review, so I'll take it out of draft. |
I haven't looked into it yet, but I'm removing |
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
5ad6b10 to
5af7e81
Compare
AlexWaygood
changed the title
Protocol[] as genericProtocol[] as generic
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
3 times, most recently
from
76ea267 to
f90aae3
Compare
|
|
Okay, so all's good here except that we panic on 5 new ecosystem projects and emit a catastrophic number of new false-positive errors on real-world code. And some benchmark regressions of up to 2%. |
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
2 times, most recently
from
ba2c100 to
648e258
Compare
|
The five new primer crashes are not new bugs introduced by this PR. I minimized the new from typing import Union
class GenericClass[T]: ...
class Foo: ...
Bar = GenericClass[Union["Bar", Foo]]The reason why we have five new primer projects panicking on this branch is that we suddenly understand many more stdlib classes as being generic, because we previously did not understand that classes inheriting from subscripted protocols were generic. Note that |
|
Here is a minimal repro of one of the new false-positive diagnostics we're emitting on from typing import Any
parser_cache: dict[str, Any] = {}
for path_to_item_map in parser_cache.values():
reveal_type(path_to_item_map) # revealed: `_VT_co`, but should be `Any`
# error: Type `_VT_co` has no attribute `items`
# revealed: Unknown
reveal_type(path_to_item_map.items())and here's an even more minimal repro that doesn't depend on the complicated typeshed stubs for from typing import Any
class Baz[T]:
x: T
class Bar[T]:
def method(self) -> Baz[T]: ...
def test2(b: Bar[Any]):
reveal_type(b.method().x) # revealed: T, but should be Any!So it does look like most of the new false-positive diagnostics are also pre-existing issues being exposed by this PR rather than new bugs being introduced by this PR. |
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
648e258 to
f1f0e79
Compare
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
e21d81b to
de14158
Compare
|
Rebasing this PR on top of #17865 has hugely improved the primer report (thanks so much for the quick fix @dcreager!!). There are still a bunch of new false-positive diagnostics however, many along the lines of this: + error[lint:invalid-parameter-default] mypy_primer/model.py:295:36: Default value of type `tuple[()]` is not assignable to annotated parameter type `Sequence[str]`or this: + error[lint:invalid-return-type] nion/utils/ListModel.py:983:16: Return type does not match returned value: Expected `Sequence[Any]`, found `list[Unknown]`These suggest to me that we might have some gaps in our assignability logic for generic instance types? |
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
244c4f2 to
5bea49e
Compare
AlexWaygood
commented
May 9, 2025
crates/ruff_benchmark/benches/ty.rs
Outdated
There was a problem hiding this comment.
The new tomllib diagnostics are:
error: lint:invalid-argument-type: Argument to this function is incorrect
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:270:33
|
268 | if char == "#":
269 | return skip_until(
270 | src, pos + 1, "\n", error_on=ILLEGAL_COMMENT_CHARS, error_on_eof=False
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Expected `frozenset[str]`, found `Unknown | frozenset[Unknown | _S]`
271 | )
272 | return pos
|
info: Function defined here
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:241:5
|
241 | def skip_until(
| ^^^^^^^^^^
242 | src: str,
243 | pos: Pos,
244 | expect: str,
245 | *,
246 | error_on: frozenset[str],
| ------------------------ Parameter declared here
247 | error_on_eof: bool,
248 | ) -> Pos:
|
info: `lint:invalid-argument-type` is enabled by default
error: lint:invalid-argument-type: Argument to this function is incorrect
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:516:24
|
514 | start_pos = pos
515 | pos = skip_until(
516 | src, pos, "'", error_on=ILLEGAL_LITERAL_STR_CHARS, error_on_eof=True
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Expected `frozenset[str]`, found `Unknown | frozenset[Unknown | _S]`
517 | )
518 | return pos + 1, src[start_pos:pos] # Skip ending apostrophe
|
info: Function defined here
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:241:5
|
241 | def skip_until(
| ^^^^^^^^^^
242 | src: str,
243 | pos: Pos,
244 | expect: str,
245 | *,
246 | error_on: frozenset[str],
| ------------------------ Parameter declared here
247 | error_on_eof: bool,
248 | ) -> Pos:
|
info: `lint:invalid-argument-type` is enabled by default
error: lint:invalid-argument-type: Argument to this function is incorrect
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:532:13
|
530 | pos,
531 | "'''",
532 | error_on=ILLEGAL_MULTILINE_LITERAL_STR_CHARS,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Expected `frozenset[str]`, found `Unknown | frozenset[Unknown | _S]`
533 | error_on_eof=True,
534 | )
|
info: Function defined here
--> /Users/alexw/dev/tomllib/tomllib/_parser.py:241:5
|
241 | def skip_until(
| ^^^^^^^^^^
242 | src: str,
243 | pos: Pos,
244 | expect: str,
245 | *,
246 | error_on: frozenset[str],
| ------------------------ Parameter declared here
247 | error_on_eof: bool,
248 | ) -> Pos:
|
info: `lint:invalid-argument-type` is enabled by default
Found 3 diagnostics
I wouldn't expect us to be able to infer frozenset[str] for the ILLEGAL_COMMENT_CHARS frozenset just yet (we haven't implemented inferring generator expressions as evaluating to generic types.GeneratorType instances yet). But I also wouldn't expect us to infer the frozenset as frozenset[Unknown | _S] -- shouldn't it just be frozenset[Unknown] if we can't infer what the TypeVar should be solved as? Cc. @dcreager
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, here is (finally!) a minimal repro for the new tomllib diagnostics, that repros on main:
from __future__ import annotations
from typing import TypeVar, Any, reveal_type
S = TypeVar("S")
class Foo[T]:
def method(self, other: Foo[S]) -> Foo[T | S]: ... # type: ignore[invalid-return-type]
def f(x: Foo[Any], y: Foo[Any]):
reveal_type(x.method(y)) # revealed: `Foo[Any | S]`, but should be `Foo[Any]`
There was a problem hiding this comment.
Thanks for the minimal repro, I can take a look! That's strange that the PEP-695 equivalent works correctly.
There was a problem hiding this comment.
This should be fixed by #18052
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
5bea49e to
e4209fe
Compare
AlexWaygood
force-pushed
the
alex/generic-protocol-mro
branch
from
e4209fe to
3697f8d
Compare
This was referenced May 9, 2025
dcreager
added a commit
that referenced
this pull request
May 13, 2025
…enerics (#18052) We were not inducting into instance types and subclass-of types when looking for legacy typevars, nor when apply specializations. This addresses #17832 (comment) ```py from __future__ import annotations from typing import TypeVar, Any, reveal_type S = TypeVar("S") class Foo[T]: def method(self, other: Foo[S]) -> Foo[T | S]: ... # type: ignore[invalid-return-type] def f(x: Foo[Any], y: Foo[Any]): reveal_type(x.method(y)) # revealed: `Foo[Any | S]`, but should be `Foo[Any]` ``` We were not detecting that `S` made `method` generic, since we were not finding it when searching the function signature for legacy typevars.
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
May 21, 2025
…enerics (astral-sh#18052) We were not inducting into instance types and subclass-of types when looking for legacy typevars, nor when apply specializations. This addresses astral-sh#17832 (comment) ```py from __future__ import annotations from typing import TypeVar, Any, reveal_type S = TypeVar("S") class Foo[T]: def method(self, other: Foo[S]) -> Foo[T | S]: ... # type: ignore[invalid-return-type] def f(x: Foo[Any], y: Foo[Any]): reveal_type(x.method(y)) # revealed: `Foo[Any | S]`, but should be `Foo[Any]` ``` We were not detecting that `S` made `method` generic, since we were not finding it when searching the function signature for legacy typevars.
|
@AlexWaygood as a workaround for First through was wrapping Next I tried messing with from collections.abc import Callable
from typing import Protocol, reveal_type
class WithCallBad(Protocol):
__call__: Callable[..., str]
class WithCallGood(Protocol):
def __call__(self, *args, **kwargs) -> str: ...
def check(c: WithCallBad) -> str:
reveal_type(c)
res = c()
reveal_type(res)
return res
Is that part of If that works, we could use Of course doesn't solve the bigger issue discussed here, but I tried |
Attributes are looked up on instances of protocol classes in the same way as they're looked up on instances of nominal classes! Any implicit dunder call in Python always looks up the dunder on the class rather than on the instance. If
Yeah, that looks like a less ambiguous way of annotating My only quibble with pytest-dev/pytest@8370971 is that I think the |
karlicoss
added a commit
to karlicoss/pytest
that referenced
this pull request
May 28, 2025
This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols) This helps with potential issues/ambiguity with `__call__` semanics in type checkers -- according to python spec, `__call__` needs to be present on the class, rather than as an instance attribute to be considered callable. This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences) However, `ty` type checker is stricter/more correct about it and every `pytest.skip` usage results in: `error[call-non-callable]: Object of type `_WithException[Unknown, <class 'Skipped'>]` is not callable` For more context, see: - astral-sh/ruff#17832 (comment) - https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 Testing: Tested with running mypy against the following snippet: ``` import pytest reveal_type(pytest.skip) reveal_type(pytest.skip.Exception) reveal_type(pytest.skip(reason="whatever")) ``` Before the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` After the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[[reason: builtins.str =, *, allow_module_level: builtins.bool =], Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` All types are matching and propagated correctly.
karlicoss
added a commit
to karlicoss/pytest
that referenced
this pull request
May 28, 2025
This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols) This helps with potential issues/ambiguity with `__call__` semanics in type checkers -- according to python spec, `__call__` needs to be present on the class, rather than as an instance attribute to be considered callable. This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences) However, `ty` type checker is stricter/more correct about it and every `pytest.skip` usage results in: `error[call-non-callable]: Object of type `_WithException[Unknown, <class 'Skipped'>]` is not callable` For more context, see: - astral-sh/ruff#17832 (comment) - https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 Testing: Tested with running mypy against the following snippet: ``` import pytest reveal_type(pytest.skip) reveal_type(pytest.skip.Exception) reveal_type(pytest.skip(reason="whatever")) ``` Before the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` After the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[[reason: builtins.str =, *, allow_module_level: builtins.bool =], Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` All types are matching and propagated correctly.
bluetech
pushed a commit
to karlicoss/pytest
that referenced
this pull request
Jun 24, 2025
This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols) This helps with potential issues/ambiguity with `__call__` semanics in type checkers -- according to python spec, `__call__` needs to be present on the class, rather than as an instance attribute to be considered callable. This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences) However, `ty` type checker is stricter/more correct about it and every `pytest.skip` usage results in: `error[call-non-callable]: Object of type `_WithException[Unknown, <class 'Skipped'>]` is not callable` For more context, see: - astral-sh/ruff#17832 (comment) - https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 Testing: Tested with running mypy against the following snippet: ``` import pytest reveal_type(pytest.skip) reveal_type(pytest.skip.Exception) reveal_type(pytest.skip(reason="whatever")) ``` Before the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` After the change: ``` test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[[reason: builtins.str =, *, allow_module_level: builtins.bool =], Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` All types are matching and propagated correctly.
karlicoss
added a commit
to karlicoss/pytest
that referenced
this pull request
Jun 25, 2025
…thException wrapper with __call__ attribute This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols) This helps with potential issues/ambiguity with `__call__` semanics in type checkers -- according to python spec, `__call__` needs to be present on the class, rather than as an instance attribute to be considered callable. This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences) However, `ty` type checker is stricter/more correct about it and every `pytest.skip` usage results in: `error[call-non-callable]: Object of type `_WithException[Unknown, <class 'Skipped'>]` is not callable` For more context, see: - astral-sh/ruff#17832 (comment) - https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 Testing: Tested with running mypy against the following snippet: ``` import pytest reveal_type(pytest.skip) reveal_type(pytest.skip.Exception) reveal_type(pytest.skip(reason="whatever")) ``` Before the change: ``` $ mypy test_pytest_skip.py test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` After the change: ``` $ mypy test_pytest_skip.py test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._Skip" test_pytest_skip.py:3: note: Revealed type is "type[_pytest.outcomes.Skipped]" test_pytest_skip.py:4: note: Revealed type is "Never" ``` `ty` type checker is also inferring the same types correctly now. All types are matching and propagated correctly (most importantly, `Never` as a result of `pytest.skip(...)` call
bluetech
pushed a commit
to pytest-dev/pytest
that referenced
this pull request
Jun 27, 2025
…thException wrapper with __call__ attribute (#13445) This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols) This helps with potential issues/ambiguity with __call__ semanics in type checkers -- according to python spec, __call__ needs to be present on the class, rather than as an instance attribute to be considered callable. This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences) However, ty type checker is stricter/more correct about it and every pytest.skip usage results in: ``` error[call-non-callable]: Object of type _WithException[Unknown, <class 'Skipped'>] is not callable ``` For more context, see: [ty] Understand classes that inherit from subscripted Protocol[] as generic astral-sh/ruff#17832 (comment) https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 Testing: Tested with running mypy against the following snippet: import pytest reveal_type(pytest.skip) reveal_type(pytest.skip.Exception) reveal_type(pytest.skip(reason="whatever")) Before the change: ``` $ mypy test_pytest_skip.py test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]" test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped" test_pytest_skip.py:4: note: Revealed type is "Never" ``` After the change: ``` $ mypy test_pytest_skip.py test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._Skip" test_pytest_skip.py:3: note: Revealed type is "type[_pytest.outcomes.Skipped]" test_pytest_skip.py:4: note: Revealed type is "Never" ``` ty type checker is also inferring the same types correctly now. All types are matching and propagated correctly (most importantly, Never as a result of pytest.skip(...) call).
|
Btw the pytest.skip fix is merged, so should be in the next release! pytest-dev/pytest#13445 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Protocol[]are generic in exactly the same way as classes that inherit fromGeneric[]. This fixes our MRO inference for a large number of stdlib classes.tupleclass: althoughtupleis only generic over a single type variable in typeshed, a homogeneous tuple is spelledtuple[int, ...]in type annotations rather thantuple[int].NamedTuplesyntax, e.g.Person = NamedTuple("Person", [("Name", int)])tuple[int, str]*tuple[int, ...]Test Plan
Existing mdtests updated