gh-104050: Add type annotations to sentinels in Argument Clinic by erlend-aasland · Pull Request #104589 · python/cpython · GitHub

Issue: Add type annotations to clinic.py #104050

I get complaints about isinstance checks now. Using type(sentinel) works, but feel insanely clumsy. Surely there must be a better way to do this.

$ mypy Tools/clinic
Tools/clinic/clinic.py:2697: error: Name "Unknown" is not defined  [name-defined]
Tools/clinic/clinic.py:3478: error: Name "Null" is not defined  [name-defined]
Tools/clinic/clinic.py:3632: error: Name "Null" is not defined  [name-defined]
Tools/clinic/clinic.py:3656: error: Name "Null" is not defined  [name-defined]
Found 4 errors in 1 file (checked 2 source files)

Rather than doing if isinstance(x, Unknown) as the test, you can do if x is unknown. Since unknown is a sentinel value, the identity check is idiomatic.

Rather than doing if isinstance(x, Unknown), you can do if x is unknown. Since unknown is a sentinel value, the identity check is idiomatic.

Doh. Thanks!

The usage of NULL seems a little different from the other two, though, so maybe that one does need its own class. (Not sure about that -- I'm unfamiliar with this code!)

E.g. I can't say I fully understand what's going on with the Null usage here:

cpython/Tools/clinic/clinic.py

Line 3483 in aed643b

default_type = (str, Null, NoneType)

Yeah, the NULL usages need a different treatment. I'll have a look later today.

- Fix variable names - Add NullType - Fix instance check

Hmm, The type of Sentinels.Null is just Sentinels. For Python enums, all enum members are instances of the enum class. (This is why I was wondering if maybe the Null class should just be left as it is; it maybe needs to be its own class, rather than sharing the same class as the other sentinel values.)

Oh, you're right.

Perhaps we should amend this instead:

cpython/Tools/clinic/clinic.py

Lines 2605 to 2607 in dcdc90d

    
           # If not None, default must be isinstance() of this type. 
        
           # (You can also specify a tuple of types.) 
        
           default_type: bltns.type[Any] | tuple[bltns.type[Any], ...] | None = None

I think my approach is fundamentally flawed. This is pretty simple really: we simply want distinct (sentinel) types for default_type and the instance check (🥁).

I'm not sure if we're in agreement or not (possibly my brain's going fuzzy at the end of a long day 😆), but I think for this PR, I might keep the refactor for unspecified and unknown, but leave Null as it is on main.

Fab, thanks!

The annotation for default_type is probably also incorrect.

cpython/Tools/clinic/clinic.py

Lines 2598 to 2607 in dcdc90d

    
           # The Python default value for this parameter, as a Python value. 
        
           # Or the magic value "unspecified" if there is no default. 
        
           # Or the magic value "unknown" if this value is a cannot be evaluated 
        
           # at Argument-Clinic-preprocessing time (but is presumed to be valid 
        
           # at runtime). 
        
           default: bool | Unspecified = unspecified 
        
           # If not None, default must be isinstance() of this type. 
        
           # (You can also specify a tuple of types.) 
        
           default_type: bltns.type[Any] | tuple[bltns.type[Any], ...] | None = None

The annotation for default_type is probably also incorrect.

It looks correct to me based on the comment and usage. But maybe I'm not seeing something. What makes you think it's incorrect?

With a custom converter, you can override pretty much anything (AFAIK), so I think bltns.type[Any] is too narrow. I might be wrong.

The comment indicates that default has to be an instance of default_type if default_type is not None, and isintance(x, Foo) for any given x will fail if Foo is not an instance of type or a tuple of objects which are instances of type

pythongh-104050 : Add type annotations to sentinels in Argument Clinic

3b12493

erlend-aasland added the skip news label May 17, 2023

erlend-aasland requested a review from AlexWaygood May 17, 2023 20:31

bedevere-bot mentioned this pull request May 17, 2023

Add type annotations to clinic.py #104050

Closed

Merge branch 'main' into typed-clinic/sentinels

5c1c2df

Fix stuff

d145ff8

- Fix variable names - Add NullType - Fix instance check

erlend-aasland marked this pull request as ready for review May 18, 2023 19:44

bedevere-bot added the awaiting core review label May 18, 2023

Revert unneeded change

5833421

This comment was marked as outdated.

Sign in to view

AlexWaygood reviewed May 18, 2023

View reviewed changes

Fix local rebase and write nicer if for line 2700

703c417

erlend-aasland closed this May 18, 2023

erlend-aasland deleted the typed-clinic/sentinels branch May 18, 2023 20:24

               NULL: Final = Sentinels.NULL
               unknown: Final = Sentinels.unknown
+              NullType = type(Sentinels.NULL)

	# If not None, default must be isinstance() of this type.
	# (You can also specify a tuple of types.)
	default_type: bltns.type[Any] \| tuple[bltns.type[Any], ...] \| None = None

	# The Python default value for this parameter, as a Python value.
	# Or the magic value "unspecified" if there is no default.
	# Or the magic value "unknown" if this value is a cannot be evaluated
	# at Argument-Clinic-preprocessing time (but is presumed to be valid
	# at runtime).
	default: bool \| Unspecified = unspecified

	# If not None, default must be isinstance() of this type.
	# (You can also specify a tuple of types.)
	default_type: bltns.type[Any] \| tuple[bltns.type[Any], ...] \| None = None

gh-104050: Add type annotations to sentinels in Argument Clinic by erlend-aasland · Pull Request #104589 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-104050: Add type annotations to sentinels in Argument Clinic - #104589

gh-104050: Add type annotations to sentinels in Argument Clinic#104589erlend-aasland wants to merge 5 commits intopython:mainpython/cpython:mainfrom erlend-aasland:typed-clinic/sentinelserlend-aasland/cpython:typed-clinic/sentinelsCopy head branch name to clipboard

Conversation

erlend-aasland commented May 17, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 17, 2023

Uh oh!

AlexWaygood commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 17, 2023

Uh oh!

AlexWaygood commented May 17, 2023

Uh oh!

erlend-aasland commented May 18, 2023

Uh oh!

This comment was marked as outdated.

AlexWaygood May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-104050: Add type annotations to sentinels in Argument Clinic#104589
erlend-aasland wants to merge 5 commits into
python:mainfrom
erlend-aasland:typed-clinic/sentinels

erlend-aasland commented May 17, 2023 •
edited by bedevere-bot
Loading

AlexWaygood commented May 17, 2023 •
edited
Loading

AlexWaygood May 18, 2023 •
edited
Loading

AlexWaygood May 18, 2023 •
edited
Loading