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

[erlend-aasland]

• Issue: Add type annotations to clinic.py #104050

[erlend-aasland]

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)

[AlexWaygood]

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.

[erlend-aasland]

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!

[AlexWaygood]

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)

[erlend-aasland]

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

[AlexWaygood]

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.)

[erlend-aasland]

Oh, you're right.

[erlend-aasland]

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

[erlend-aasland]

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 (🥁).

[AlexWaygood]

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.

[AlexWaygood]

Fab, thanks!

[erlend-aasland]

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

[AlexWaygood]

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?

[erlend-aasland]

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

[AlexWaygood]

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