Repository navigation
Fix self in identity alias - #22182
Moussa-Mikhail wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
…otation is_expected_self_type() already unwraps Annotated[Self, ...] so a method like "def m(self: Annotated[Self, ...])" is recognized as equivalent to a bare "self: Self". A generic type alias that's equally transparent (e.g. "type Ident[T] = T") had no equivalent handling, so "self: Ident[Self]" was wrongly rejected with "Method cannot have explicit self annotation and Self type". Adds expand_type_alias_for_self_check(), which substitutes the alias's own type parameters with the arguments it was referenced with (using mypy's existing substitution, mypy.expandtype.expand_type) and recurses is_expected_self_type() on the result -- the same way the existing Annotated[...] case recurses on its one argument. This is handled in both places an explicit self-type annotation is checked: the plain/ instance-method branch (self: Ident[Self]) and the classmethod branch, which needs it in two different positions depending on how the alias and type[...] are nested -- cls: type[Ident[Self]] unwraps through the existing TypeType case first and lands in the plain branch, while cls: Ident[type[Self]] needs the same handling in the classmethod branch's own UnboundType case, recursing with is_classmethod=True preserved -- same as the ANNOTATED_TYPE_NAMES case right next to it. Using real substitution instead of hand-matching "is the target exactly the alias's sole type parameter" also covers, without any extra code: an alias with extra, unused type parameters, in any order (e.g. type Extra[T, S] = T used as Extra[Self, int] or Extra[int, Self]); and a target that only reduces to the expected shape once substituted (e.g. type UnionSame[T] = T | T, which collapses once both T's become Self). Self supplied for an alias's *unused* parameter is correctly still rejected, since the substitution never looks at that slot. Also drops the redundant "sym is not None" repeated on every branch below the symbol lookup, in both the classmethod and instance-method cases, in favor of a single early return when the lookup fails -- the control flow in each branch already made the None case fall through to the same result, so this part is a pure refactor with no behavior change. expand_type_alias_for_self_check()'s args parameter takes UnboundType.args directly, which mypy's own types.py types as a Sequence[Type] (stored internally as a tuple), not a list -- declared as list[Type] at first, which only surfaced as a real error under mypy's own self-check (runtests.py self), not under ordinary testing. Fixed to Sequence[Type], imported from collections.abc to match this module's existing convention for Callable/Collection/Iterable/Iterator. Verified: bare self: Self, Annotated[Self, ...], an explicit concrete self-type, and a non-Self alias argument (Ident[int], which should and does still raise a real self-type error) are all unaffected; both classmethod orderings, extra-unused-parameter aliases (both orders), and the union-collapse case all work; Self in an alias's unused slot still correctly errors. Self-type test suite 152/152. runtests.py self: 10 errors, matching an unmodified checkout exactly (pre-existing environment gaps -- missing psutil/tomli/filelock/setuptools stubs), confirmed by running the identical check against the unmodified base commit. Full test suite (mypy/test + mypyc/test): 13627 passed, 410 skipped, 10 xfailed; the 12 failures and 3 collection errors present are pre-existing environment gaps (missing attrs/mypyc-build deps), confirmed identical on an unmodified checkout. The code above and in this commit was written by Claude and reviewed by Moussa Mikhail. The issue and the PR description accompanying it are his own, written by him. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REqZNys4H8tiFU7Bb6Qj2B
Adds testTypingSelfIdentityTypeAlias (mirroring the existing
testTypingSelfAnnotatedType) plus a minimal regression case, covering:
self: Ident[Self], both orderings of the classmethod form
(cls: Ident[type[Self]] and cls: type[Ident[Self]]), and a negative case
confirming a non-Self alias argument (Ident[int]) still raises the real
self-type error.
Adds testTypingSelfIdentityTypeAliasSubstitution covering what the
substitution-based implementation handles beyond the literal "type X[T]
= T" shape, for free: a union that only collapses to Self once
substituted (type UnionSame[T] = T | T), an alias with an extra unused
type parameter in either position (type Extra[T, S] = T and type
Extra2[S, T] = T), the classmethod form of the same, and a negative case
confirming Self supplied for an alias's *unused* parameter is still
correctly rejected, not silently accepted.
Writing the classmethod cases in the first test is what surfaced the gap
the previous commit's classmethod-branch handling fixes -- cls:
Ident[type[Self]] was still being wrongly rejected until that commit
added the same handling to the classmethod branch, not just the plain
branch.
Separately, found and worked around an unrelated pre-existing mypy issue
while writing these tests: a generic type alias whose value is *exactly*
a bare type parameter (type Ident[T] = T) crashes the full checker
(AssertionError: Var(TypeVar), in checker.named_type) under the test
suite's minimal typing.pyi stub, where TypeVar is a dummy (`TypeVar = 0`)
rather than a real class -- checker.py's alias-value-expression check
calls named_type("typing.TypeVar") and asserts it resolves to a TypeInfo.
A wrapped alias (type Wrapped[T] = list[T]) does not trigger it, so it's
specific to the bare-identity shape. [typing fixtures/typing-full.pyi]
(which has a real TypeVar class) sidesteps it; the new cases declare it
for that reason. Not touched here -- it's unrelated to self-annotation
handling and worth its own report.
Verified: all new cases pass; self-type test suite 152/152 (was 149
before either commit). Full test suite (mypy/test + mypyc/test): 13627
passed, 410 skipped, 10 xfailed, with the same 12 failures / 3 collection
errors as before these changes, confirmed identical to an unmodified
checkout (pre-existing environment gaps: missing attrs/mypyc-build
deps).
The code above and in this commit was written by Claude and reviewed by
Moussa Mikhail. The issue and the PR description accompanying it are his
own, written by him.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REqZNys4H8tiFU7Bb6Qj2B
1ecaab8 to
5a326cd
Compare
This comment has been minimized.
This comment has been minimized.
chrikrah
left a comment
There was a problem hiding this comment.
Requesting changes for one crash. The expansion substitutes a plain Self for every alias parameter, and a TypeVarTuple parameter needs an unpacked argument, so this input ends in an INTERNAL ERROR where main prints two ordinary errors.
$ cat self3.py
from typing import Self
type Vt[*Ts] = tuple[*Ts]
class C:
def vt(self: Vt[Self]) -> None: ...
$ python -m mypy --python-version 3.12 --no-incremental --show-traceback self3.py # head 5a326cd6c
self3.py:4: error: INTERNAL ERROR -- Please try using mypy master on GitHub:
RuntimeError: Invalid type replacement to expand: Self?
$ <same command> # base f501cd0b7
self3.py:4: error: Method cannot have explicit self annotation and Self type [misc]
self3.py:4: error: The erased type of self "tuple[Any]" is not a supertype of its class "self3.C" [misc]
$ python -m pytest mypy/test/testcheck.py -n 4 -q -k "SelfType or selftype or Self or Alias or TypeAlias" # Python 3.12.6
648 passed, 1 skipped, 1 xfailed # base
651 passed, 1 skipped, 1 xfailed # head
2 failed, 1 passed # head with semanal.py reverted, -k testTypingSelfIdentityTypeAlias
blocking, @Moussa-Mikhail: add or not all(isinstance(tv, TypeVarType) for tv in node.alias_tvars) to the early return in expand_type_alias_for_self_check. With that line self3.py prints the base errors again and the same selection still gives 651 passed.
non-blocking: the UnionSame case in testTypingSelfIdentityTypeAliasSubstitution can go. It pins self: Self | Self in a reveal, which nobody writes, and a later union simplification would break it.
Excavation: #14041 added the error to keep Self and an explicit self annotation apart, and #21928 carved out Annotated[Self, ...]. This extends the carve-out to aliases that collapse to Self, so the old intent still holds.
@ilevkivskyi the general expansion also covers the Anno[T] alias from the issue without a second special case, so it stays. Is the general expansion acceptable for this PR?
A TypeVarTuple alias parameter needs an unpacked argument to substitute correctly, so passing it through expand_type the same way as a plain TypeVarType crashed with "Invalid type replacement to expand: Self?" instead of reporting the ordinary ill-typed-self errors. Also drops the UnionSame[T] = T | T substitution case, which pins a union that a later simplification pass could legitimately collapse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
fixes #22181
This PR expands type aliases in annotations of self and cls so that
is_expected_self_typecan handle them like any other annotation. It also refactors some repetitive if conditions by early returning when they aren't true. Using mypy's own type substitution means type aliases with strange shapes (e.g. type Extra[T, S] = T, UnionSame[T] = T | T) work without special-casing them.The code in this PR was written by Claude and reviewed by me.