FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-130827: Support `typing.Self` annotations in `singledispatchmethod()` by ZeroIntensity · Pull Request #130829 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-130827: Support typing.Self annotations in singledispatchmethod() - #130829

Closed
ZeroIntensity wants to merge 8 commits into
python:mainfrom
ZeroIntensity:typing-self-singledispatch
ConversationCommits (8)ChecksFiles changed
Closed

gh-130827: Support typing.Self annotations in singledispatchmethod()#130829
ZeroIntensity wants to merge 8 commits into
python:mainfrom
ZeroIntensity:typing-self-singledispatch

Conversation

ZeroIntensity commented Mar 4, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Mar 4, 2025
Open
picnixz reviewed Mar 4, 2025
View reviewed changes
Comment thread Lib/test/test_functools.py
Update Lib/test/test_functools.py
2e6b46f 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
sobolevn reviewed Mar 4, 2025
View reviewed changes
Comment thread Lib/test/test_functools.py
# But, it shouldn't work on singledispatch()
with self.assertRaises(TypeError):
@test.register
def silly(self: typing.Self, arg: int | str) -> int | str:

sobolevn Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Will 'typing.Self' annotation work the same way?

ZeroIntensity Mar 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good question! It should, yeah. get_type_hints handles forward references.

sobolevn Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we add a test case for that, maybe?

ZeroIntensity Mar 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Possibly, but as long as get_type_hints is working it's probably fine.

tomasr8 reviewed Mar 4, 2025
View reviewed changes
Comment thread Lib/functools.py Outdated
Update Lib/functools.py
f571ec7 
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
JelleZijlstra reviewed Mar 4, 2025
View reviewed changes
Comment thread Lib/functools.py Outdated

ZeroIntensity commented Apr 2, 2025

Copy link
Copy Markdown
Member Author

@vstinner I'd like to get this fixed before 3.12 goes security-only next week. Would you mind reviewing?

vstinner commented Apr 2, 2025

Copy link
Copy Markdown
Member

@vstinner I'd like to get this fixed before 3.12 goes security-only next week. Would you mind reviewing?

That's out of my domain expertise.

@ambv would be a better reviewer for this task ;-)

JelleZijlstra commented Apr 2, 2025

Copy link
Copy Markdown
Member

Wait why would we want to change this in 3.12 at all? Seems like a new feature to me.

sobolevn removed needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Apr 2, 2025

sobolevn commented Apr 2, 2025

Copy link
Copy Markdown
Member

I agree, this is a new feature and should not be backported.

ambv commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

Agreed with Jelle, this is a feature best left for a new release. Otherwise it's too hard to guard against in user code.

ZeroIntensity commented Apr 2, 2025

Copy link
Copy Markdown
Member Author

Makes sense. I've updated the issue.

ZeroIntensity commented Apr 29, 2025

Copy link
Copy Markdown
Member Author

Bump. I'd like to land this one before the beta freeze.

JelleZijlstra requested changes May 4, 2025
View reviewed changes

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think special-casing the Self annotation is the wrong approach. The self argument may be annotated with Self, but it may also be annotated with e.g. a TypeVar or Annotated.

We should be ignoring annotations not on the basis of what the annotation is, but on the basis of what parameter they're on. I think the correct approach would be to figure out the name of the first non-self argument (i.e., the second argument for a method), then retrieve the annotation for that argument.

If I'm not mistaken, singledispatch also incorrectly (or at least unintuitively) handles cases where the annotation is on some later argument, but the first one is unannotated.

@register
def f(x, random_arg: str = ...):

bedevere-app Bot commented May 4, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ZeroIntensity commented May 4, 2025

Copy link
Copy Markdown
Member Author

I think the correct approach would be to figure out the name of the first non-self argument

Wouldn't that break if it's named something other than self?

JelleZijlstra commented May 4, 2025

Copy link
Copy Markdown
Member

I mean the first argument that's not conceptually self. We shouldn't look at the name.

johnslavik mentioned this pull request Oct 17, 2025
Closed

github-actions Bot commented Apr 22, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 22, 2026

ZeroIntensity commented Apr 22, 2026

Copy link
Copy Markdown
Member Author

I think @johnslavik has a different solution that he's been working on -- I'm going to close this in favor of his work.

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tomasr8 tomasr8 left review comments
picnixz picnixz left review comments
JelleZijlstra JelleZijlstra requested changes
rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
sobolevn Awaiting requested review from sobolevn

Assignees

No one assigned

Labels

awaiting changes stale Stale PR or inactive for long period of time. topic-typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL