Skip to content

gh-148829: Implement PEP 661#148831

Open
JelleZijlstra wants to merge 21 commits intopython:mainfrom
JelleZijlstra:pep661
Open

gh-148829: Implement PEP 661#148831
JelleZijlstra wants to merge 21 commits intopython:mainfrom
JelleZijlstra:pep661

Conversation

@JelleZijlstra
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra commented Apr 21, 2026

Comment thread Objects/sentinelobject.c Outdated
Comment thread Lib/typing.py
Comment thread Objects/sentinelobject.c Outdated
Comment thread Objects/sentinelobject.c Outdated
Comment thread Objects/sentinelobject.c Outdated
Comment thread Objects/sentinelobject.c Outdated
Comment thread Objects/sentinelobject.c
Comment thread Objects/sentinelobject.c
Comment thread Objects/sentinelobject.c Outdated
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@sunmy2019 sunmy2019 self-requested a review April 23, 2026 19:42
Comment thread Doc/c-api/sentinel.rst Outdated
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 24, 2026
@bedevere-bot
Copy link
Copy Markdown

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F148831%2Fmerge

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 24, 2026
Copy link
Copy Markdown
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

The C code and the corresponding documentation look good to me. I didn't look very closely at the tests.

Comment thread Objects/sentinelobject.c
Copy link
Copy Markdown
Member

@sunmy2019 sunmy2019 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@sunmy2019 sunmy2019 left a comment

Choose a reason for hiding this comment

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

Clarify the pickle document. So that people will see X = sentinel("Y") is not picklable.

Comment thread Doc/c-api/sentinel.rst Outdated
Comment thread Doc/library/functions.rst Outdated
@JelleZijlstra
Copy link
Copy Markdown
Member Author

JelleZijlstra commented Apr 24, 2026

Buildbot run: A few failures look unrelated

In file included from ./Include/internal/pycore_backoff.h:12,
                 from ./Include/internal/pycore_code.h:11,
                 from ./Include/internal/pycore_call.h:11,
                 from ./Modules/_ctypes/_ctypes.c:109:
./Modules/_ctypes/_ctypes.c: In function ‘set_stginfo_ffi_type_pointer’:
./Modules/_ctypes/_ctypes.c:2234:40: error: ‘FFI_TYPE_COMPLEX’ undeclared (first use in this function); did you mean ‘FFI_TYPE_DOUBLE’?
         assert(fmt->pffi_type->type == FFI_TYPE_COMPLEX);
                                        ^~~~~~~~~~~~~~~~
./Modules/_ctypes/_ctypes.c:2234:40: note: each undeclared identifier is reported only once for each function it appears in

But I also see

Traceback (most recent call last):
  File "/home/ubuntu/buildarea/pull_request.stan-aarch64-ubuntu.oddballs/build/Lib/test/pickletester.py", line 3254, in test_builtin_types
    s = self.dumps(t, proto)
  File "/home/ubuntu/buildarea/pull_request.stan-aarch64-ubuntu.oddballs/build/Lib/test/test_xpickle.py", line 190, in dumps
    return self.send_to_worker(python, data)
           ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/pull_request.stan-aarch64-ubuntu.oddballs/build/Lib/test/test_xpickle.py", line 172, in send_to_worker
    raise exception
AttributeError: Can't get attribute 'sentinel' on <module 'builtins' (built-in)>

https://buildbot.python.org/#/builders/1865/builds/54

Which is a bit mysterious, is this looking at the builtins of another build?

@sunmy2019
Copy link
Copy Markdown
Member

Which is a bit mysterious, is this looking at the builtins of another build?

Yes. We need to patch this test. It tests pickle compatiblity acrosses python versions.

./python -m test test_xpickle -m test_builtin_types -u xpickle

@sunmy2019
Copy link
Copy Markdown
Member

diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 6366f12257..c2018c9785 100644
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -3244,6 +3244,7 @@ def test_builtin_types(self):
             'BuiltinImporter': (3, 3),
             'str': (3, 4),  # not interoperable with Python < 3.4
             'frozendict': (3, 15),
+            'sentinel': (3, 15),
         }
         for t in builtins.__dict__.values():
             if isinstance(t, type) and not issubclass(t, BaseException):

@sunmy2019
Copy link
Copy Markdown
Member

Buildbot run: A few failures look unrelated

I opened #148967 to track it.

Comment thread Doc/c-api/sentinel.rst Outdated
*name* and :attr:`~sentinel.__module__` set to *module_name*.
Return ``NULL`` with an exception set on failure.

*module_name* should be the name of an importable module if the sentinel
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.

Please mention whether NULL is allowed for module_name to indicate that the sentinel doesn't support pickle.

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.

It is not. Should we support this?

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.

We might as well, since we already set the module to None if caller() doesn't find anything.

Comment thread Doc/library/functions.rst Outdated
Comment thread Include/sentinelobject.h
Comment thread Makefile.pre.in
Comment thread Makefile.pre.in
Comment thread Objects/object.c Outdated
Comment thread Objects/sentinelobject.c Outdated
Comment thread Objects/sentinelobject.c
Comment thread Objects/sentinelobject.c Outdated
@JelleZijlstra
Copy link
Copy Markdown
Member Author

Thanks for the feedback! Pushed a new version.

Comment thread Lib/test/test_capi/test_object.py Outdated
class SentinelTest(unittest.TestCase):

def test_pysentinel_new(self):
import pickle
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.

I think you can put this at the top-level. It's not really important that imports are fast in test modules.

Comment thread Doc/library/functions.rst
Comment on lines +1874 to +1881

.. attribute:: __name__

The sentinel's name.

.. attribute:: __module__

The name of the module where the sentinel was created.
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.

Maybe indicate something like "A sentinel object exposes the following read-only attributes" so that the paragraph about pickle is not glued to it.

Comment thread Lib/test/test_builtin.py
support.gc_collect()
self.assertIsNone(ref())

def test_sentinel_union(self):
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.

Maybe add a test where the RHS/LHS is not a type?

Comment thread Lib/test/test_builtin.py
Comment on lines +1942 to +1943
with self.assertRaises(AttributeError):
missing.__module__ = "changed"
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.

Maybe add tests to check that we can't delete those attributes either

Comment thread Doc/library/functions.rst Outdated
Comment on lines +1844 to +1846
Sentinels importable from their defining module by name preserve their
identity when pickled and unpickled. Sentinels that are not importable by
module and name are not picklable.
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.

How about move this paragraph down to the pickle part?

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.

Actually this is mostly redundant with the later pickle paragraph, I'll remove most of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants