Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116126: Implement PEP 696 #116129

Merged
merged 56 commits into from
May 3, 2024
Merged

gh-116126: Implement PEP 696 #116129

merged 56 commits into from
May 3, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Feb 29, 2024

This implements both the grammar and compiler changes and changes to typing.py behavior (the latter copied from typing-extensions).

@JelleZijlstra
Copy link
Member Author

This should be ready for review now, I'll take it out of draft once the tests pass. Notes:

  • This PR intentionally does not cover documentation or typing.py behavior; I'll leave that to another PR.
  • The PEP 695 test suite helped me find some very interesting issues with the symtable effects of this change. I think the current solution should work, but definitely didn't expect that to be the trickiest aspect of the change.
  • It's unfortunate that the new AST field is called default_ in Python. It can't be default in C because that's a keyword. I may need to change the AST generation machinery to support aliasing a field so the Python-visible name can be just "default".

@pablogsal
Copy link
Member

Will review this weekend

@JelleZijlstra
Copy link
Member Author

@pablogsal have you had a chance to look at this? The grammar changes should be quite straightforward, so hopefully it's not complicated to review.

I'll spend some time today trying to make it so the field is called "default" not "default_" in Python, because having "default_" in the Python-visible APIs would be ugly.

Doc/library/typing.rst Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 23, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit ef0616b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 23, 2024
Doc/reference/executionmodel.rst Outdated Show resolved Hide resolved
Lib/test/test_unparse.py Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/reference/compound_stmts.rst Outdated Show resolved Hide resolved
JelleZijlstra and others added 2 commits April 28, 2024 13:30
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I would say "is not the typing.NoDefault singleton" rather than "is not equal to typing.NoDefault", since we usually argue that an identity check is more idiomatic for singleton objects such as None, NotImplemented and Ellipsis

Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Comment on lines +3244 to +3248
>>> T.__default__ is typing.NoDefault
True
>>> S = TypeVar("S", default=None)
>>> S.__default__ is None
True
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> T.__default__ is typing.NoDefault
True
>>> S = TypeVar("S", default=None)
>>> S.__default__ is None
True
>>> T.has_default()
False
>>> T.__default__ is typing.NoDefault
True
>>> S = TypeVar("S", default=None)
>>> S.has_default()
True
>>> S.__default__ is None
True

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of that but it felt out of place in the docs for NoDefault, since the cases you added don't use NoDefault at all.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough -- I thought it was quite nice to see together in one example how the two concepts interrelate, but I definitely don't feel strongly!

JelleZijlstra and others added 2 commits April 28, 2024 14:29
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@JelleZijlstra
Copy link
Member Author

@ericsnowcurrently the C globals check is failing because this PR now adds a new static type and global singleton, typing.NoDefault (closely analogous to builtins.NotImplemented). What do you think we should do? We could simply allowlist the new globals, or somehow make NoDefault not rely on C globals—but the latter feels like it would be a lot more complicated.

@ericsnowcurrently
Copy link
Member

For now let's just whitelist the new singleton, as long as it is stateless and immortal. We can circle back later if the whitelist is a problem.

@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 30, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 5f6fdfd 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 30, 2024
The previous logic would add the size of the pointer target to the pointer value, which might point to another pointer being used as a scope key. Now, we increment the value as an integer instead, which means it can never be a valid pointer.
@JelleZijlstra JelleZijlstra merged commit ca269e5 into python:main May 3, 2024
36 checks passed
@JelleZijlstra JelleZijlstra deleted the pep696v2 branch May 3, 2024 13:17
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants