Skip to content

Conversation

Gatsik
Copy link
Contributor

@Gatsik Gatsik commented Jul 21, 2025

Needed for python/mypy#19414 as was suggested in python/mypy#19479 (review)


@srittau 2025-07-22 Deferred: stubtest needs special handling for namedtuple's functional form first.

This comment has been minimized.

@Gatsik
Copy link
Contributor Author

Gatsik commented Jul 21, 2025

I suspects stubtests are failing because they run mypy, which does not have these stubs yet, and in mypy PR tests fail because typeshed does not have these stubs yet

@srittau
Copy link
Collaborator

srittau commented Jul 21, 2025

The test failures seem mostly unrelated, probably because new minor Python versions were released for the GitHub Actions runners. But I spotted at least one problem related to this PR in the tests, but we can look at that when the unrelated errors have been cleared.

@brianschubert
Copy link
Member

I think the remaining test failures are due to this PR changing how stubtest analyzes named tuples. Currently, the fact that _tuplegetter doesn't exist in the stubs causes stubtest to skip comparing the runtime values of NamedTuple fields with the stubs (specifically, this branch get taken), but now that _tuplegetter exists it does attempt the comparison, which fails.

This comment has been minimized.

@@ -32,6 +32,19 @@ _VT = TypeVar("_VT")
_KT_co = TypeVar("_KT_co", covariant=True)
_VT_co = TypeVar("_VT_co", covariant=True)

@final
class _tuplegetter(Generic[_T]): # undocumented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not likely to be needed, but for completeness:

Suggested change
class _tuplegetter(Generic[_T]): # undocumented
class _tuplegetter(Generic[_T]): # undocumented
def __init__(self, index: SupportsIndex, doc: str, /) -> None: ...
def __reduce__(self) -> tuple[type[Self], tuple[int, str]]: ...

Comment on lines 38 to 46
@overload
def __get__(self, instance: None, owner: type[Any] | None = None) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None = None) -> _T: ...
else:
@overload
def __get__(self, instance: None, owner: type[Any] | None) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None) -> _T: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__get__ doesn't accept keyword arguments at runtime:

Suggested change
@overload
def __get__(self, instance: None, owner: type[Any] | None = None) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None = None) -> _T: ...
else:
@overload
def __get__(self, instance: None, owner: type[Any] | None) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None) -> _T: ...
@overload
def __get__(self, instance: None, owner: type[Any] | None = None, /) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None = None, /) -> _T: ...
else:
@overload
def __get__(self, instance: None, owner: type[Any] | None, /) -> Self: ...
@overload
def __get__(self, instance: object, owner: type[Any] | None, /) -> _T: ...

@@ -32,6 +32,19 @@ _VT = TypeVar("_VT")
_KT_co = TypeVar("_KT_co", covariant=True)
_VT_co = TypeVar("_VT_co", covariant=True)

@final
class _tuplegetter(Generic[_T]): # undocumented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could mention that at runtime this class is implemented in _collections, but it considers itself to live in collections since Python 3.12

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau
Copy link
Collaborator

srittau commented Jul 22, 2025

I think the remaining test failures are due to this PR changing how stubtest analyzes named tuples. Currently, the fact that _tuplegetter doesn't exist in the stubs causes stubtest to skip comparing the runtime values of NamedTuple fields with the stubs (specifically, this branch get taken), but now that _tuplegetter exists it does attempt the comparison, which fails.

You're right. The functional form of namedtuple will need special handling by stubtest. I don't think we can apply this PR until the necessary stubtest changes have been made, unless we want to add all those namedtuple's to stubtest's ignore list.

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Jul 22, 2025
Gatsik added a commit to Gatsik/mypy that referenced this pull request Jul 22, 2025
@brianschubert
Copy link
Member

I looked a little deeper into the stubtest failures and how this interacts with python/mypy#19479.

Briefly, this is what's going on:

  • Currently, when analyzing attribtues on named tuple classes, stubtest deduces a stub type equal to the field type (per mypy#19414), but resolves no runtime type due to collections._tuplegetter not existing in the stubs. Since there's no runtime type to compare against, stubtest skips any further analysis. Hence no errors are emitted.
  • With this PR, stubtest still deduces the stub type to be the field type, but it now successfully resolves the runtime type to be collections._tuplegetter[Any] (since this type now exists in the stubs). Analysis continues, and since collections._tuplegetter[Any] is not a subtype of the field type, stubtest emits the errors we see above. (note: these failures only happen on 3.12+ since in previous versions _tuplegetter calls itself _collections._tuplegetter, so the lookup still fails)
  • After applying mypy#19479, stubtest starts deducing the stub type to be collections._tuplegetter[<field type>] instead of <field type>. Since collections._tuplegetter[Any] is a subtype of collections._tuplegetter[<field type>], this passes and no errors are emitted.

So, from what I can tell, there's no issue with how stubtest is working, and if both this PR and mypy#19479 were both already merged, everything would run green.

If there's desire to move forward with this, I think there's three ways we could go about it:

  1. Merge mypy#19479 in its current state, and temporarily add this PR to mypy's misc/typeshed_patches. Typeshed would then need to wait for a mypy release that includes mypy#19479, at which point the stubtest errors will go away and this PR can be merged.
  2. Merge this PR without mypy#19479, and temporarily add all named tuple fields to stubtest's allowlist. Mypy could then merge mypy#19479 after a typeshed sync. The allowlist entries would be removed after typeshed updates to a version of mypy that includes mypy#19479
  3. (Temporarily?) patch subtest to ignore _tuplegetter instances at runtime, then wait for a mypy release and follow option (2), sans the allowlist entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants