Skip to content

Refactor Sentinel to conform to PEP 661 #617

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

HexDecimal
Copy link
Contributor

@HexDecimal HexDecimal commented Jun 4, 2025

My attempt at implementing PEP 661 since I was unhappy with #594. I'm hoping that this is a PR of decent quality and not just me desperately wanting pickle support.

Changes made to Sentinel:

  • repr parameter removed, explicit repr tests removed, repr was rejected by the PEP, reverted, repr is kept
  • __repr__ modified to match PEP implementation (removed angle brackets)
  • Added module_name parameter following PEP implementation and tweaking that to use the local _caller helper function.
  • name required support for qualified names to follow PEP and ensure forward compatibility, this is tested. Unless sentinels in class scopes are forbidden then qualified names are not optional.
  • Added __reduce__ to track Sentinel by name and module_name.
  • Added a Sentinel registry to preserve Sentinel identity across multiple calls to the class. Added tests for this.
  • Added an import step to allow forward compatibility with other sentinel libraries. Import step is tested.
    This is not from the PEP but it is required for typing_extensions to be forward compatible with any official sentinel type.
  • Added copy and pickle tests. There were several other use-cases for pickle from the PEP 661 discussion. The PEP requires that the sentinel identity is preserved during these operations.
  • Updated documentation for Sentinel.

For a while I still supported the repr parameter and after following the PEP 661 discussion I ended up implementing the truthiness tweaks mentioned there, but then I ended up scrapping all of that so that I could follow the PEP more closely, but they would be easy to reintroduce later if desired.

If you don't like the current __reduce__ function relying on the class directly then I can modify it to rely on a private classmethod instead which will ensure that any later changes to __new__ or __reduce__ won't cause issues. That said it seems the name, module_name parameters are one of the few things most have agreed on.

The _import_sentinel staticmethod can be moved if desired.

I'm unsure of how to mention version changes in the docs. Replacing repr with module_name is technically a breaking change.

_sentinel_registry.setdefault is potentially supposed to be protected with a lock but I'm unsure of if I can import the threading module without causing platform issues.

_sentinel_registry could be replaced with a weak values dictionary but I'm unsure if that's necessary due to how permanent most sentinels are.

`repr` parameter removed, explicit repr tests removed

`__repr__` modified to match PEP implementation (removed angle brackets)

Added `module_name` parameter following PEP implementation and tweaking
to use `_caller` helper function.

`name` required support for qualified names to follow PEP implementation.

Added `__reduce__` to track Sentinel by name and module_name.

Added a Sentinel registry to preserve Sentinel identity across multiple
calls to the class. Added tests for this.

Added an import step to allow forward compatibility with other sentinel libraries.
Import step is tested.
This was not required by the PEP but it is required for
typing_extensions to have a forward compatible type.

Added copy and pickle tests.

Updated documentation for Sentinel.
@python-cla-bot
Copy link

python-cla-bot bot commented Jun 4, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@Viicos
Copy link
Contributor

Viicos commented Jun 5, 2025

Replacing repr with module_name is technically a breaking change.

I think breaking changes are expected for draft PEPs, so imo no deprecation process should be used. Users should be aware that changes are expected. However, I think we can:

  • Improve the documentation about draft PEPs, stating that implementation can change.
  • Explicitly state that Sentinels are from a draft PEP in the Sentinel class documentation, and isn't stable yet.

@Viicos
Copy link
Contributor

Viicos commented Jun 5, 2025

cc @taleinat

@JelleZijlstra
Copy link
Member

I'm not willing to remove the repr parameter from typing_extensions.Sentinel without a deprecation. However, I'm OK to re-export the builtin Sentinel class in 3.15 (if it makes it in), even if it doesn't have this parameter.

@HexDecimal
Copy link
Contributor Author

I'm not willing to remove the repr parameter from typing_extensions.Sentinel without a deprecation. However, I'm OK to re-export the builtin Sentinel class in 3.15 (if it makes it in), even if it doesn't have this parameter.

Then I will change repr into a keyword parameter.

If compatibility is important enough then I can also have the code assume module_name is actually repr if it does not refer to a existing module. Would this be necessary?

Configuration like repr would be defined only with the initial Sentinel object. It wouldn't get stored in the reduce function. This allows changes in repr to be reflected in unpickled sentinels. This means that removing repr later will not break serialization.

Adds tests for both the keyword and old positional repr parameters
Changes in Sentinel such as swapping it with a theoretical
typing.Sentinel will affect private methods, so the callable used with
`__reduce__` must be at the top-level to be stable.
Additional data stored as a dict to ensure backwards and forwards compatibility
@HexDecimal HexDecimal force-pushed the conforming-sentinel branch from 0653d70 to 2c509de Compare July 1, 2025 23:46
Use pickles method of handing singleton objects.
This is simpler and more predictable than a custom unpickle function.

Anonymous sentinels can no longer be pickled and will
raise PicklingError instead of TypeError.
@HexDecimal HexDecimal force-pushed the conforming-sentinel branch from 2c509de to e29210a Compare July 1, 2025 23:49
@HexDecimal
Copy link
Contributor Author

I've replaced the __reduce__ method with a version using pickle's singleton support. This is the most conservative and inoffensive option for a new reduce function since it doesn't add a custom unpickle function, is forward compatible with any future method of pickling, is the standard method of handling singletons via pickle, and is generally strict. This does not support pickling anonymous sentinels which will now raise pickle.PicklingError instead of TypeError. I can revert this if the previous behavior was more desired, but I personally only need to pickle sentinels which have a top-level definition, and the more I work with them the more that anonymous sentinels feel counter intuitive.

I've attempted to add to the discussion of PEP 661. Anonymous sentinels bring a lot of issues which need to be addressed.
https://discuss.python.org/t/pep-661-sentinel-values/9126/251

Before this is merged I'd like to add a strict keyword parameter to Sentinel which prevents sentinels from being accidentally named after existing top-level objects which are not Sentinel as well as ensuring that repr is never given conflicting values. I'll need feedback on if this is appropriate.

return None
try:
module = importlib.import_module(module_name)
return operator.attrgetter(name)(module)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return operator.attrgetter(name)(module)
return getattr(module, name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion breaks qualified names and current tests. operator.attrgetter handles dots while getattr does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant code removed during refactor.

@Viicos
Copy link
Contributor

Viicos commented Jul 2, 2025

Thanks @HexDecimal. I just tested your implementation in Pydantic to implement an UNSET sentinel, and pickling seems to work as expected.

@HexDecimal
Copy link
Contributor Author

HexDecimal commented Jul 4, 2025

Note on behavior regarding the sentinel registry and imported sentinels:

class MISSING:
    pass

assert Sentinel("MISSING") is MISSING  # Sentinel named after an existing non-Sentinel object

Which of these makes the most sense for the above code?

  1. Register and return the existing class as the sentinel identity, the assert will pass (current PR implementation)
  2. Return a new Sentinel identity, the assert will fail (old implementation)
  3. Raise a warning, then do 1
  4. Raise TypeError because MISSING is not a Sentinel (what I recommend, the most strict option)
  5. Add a strict parameter to Sentinel to decide between 1 and 4

Edit: currently going with 2 to simplify the implementation.


2nd behavior involving registered sentinel parameters:

MISSING = Sentinel("MISSING", repr="foo")

assert MISSING is Sentinel("MISSING")  # Implicit 'repr' conflicts with previous 'repr'
assert MISSING is Sentinel("MISSING", repr="bar")  # Explicit 'repr' conflicts with previous 'repr'

Which of these makes the most sense for the above code?

  1. repr="foo" is locked in for all subsequent sentinels, subsequent repr is always ignored, asserts pass (current implementation)
  2. Both asserts raise ValueError as long as repr="foo" is not given again (my recommendation, the most strict option)
  3. repr only checked if explicitly given, 1st assert passes, 2nd assert raises ValueError
  4. Check repr like 2 but raise warnings instead of errors
  5. Add strict parameter to decide between 1 and 2

Edit: currently going with 4 to not break anything.


The truthiness of sentinels is still being discussed. One thing which is unclear is if conversions to bool are common enough to be an issue in the first place (because sentinels are normally checked using identity comparison and little else). It'd be interesting to experiment with the most strict option and disable conversions to bool entirely:

def __bool__(self) -> Never:
    raise TypeError(...)

@JelleZijlstra
Copy link
Member

Which of these makes the most sense for the above code?

I feel strongly that 2 is the right behavior. Constructing a class shouldn't look around in the globals for other stuff that might be using the same name.

@HexDecimal
Copy link
Contributor Author

Constructing a class shouldn't look around in the globals for other stuff that might be using the same name.

That was necessary to handle unpickling until I finally implemented the correct reduce function, but at this point I can remove that code now and revert to the old behavior. I'd accept that the other options are unnecessarily handholdy.

No longer needed due to a correctly implemented reduce method.

Simplifies code and makes syntax more predictable.
Remove documentation implying that multiple definitions of Sentinel
objects are a regular occurrence
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.

3 participants