Fix Union[..., NoneType] injection by get_type_hints if a None default value is used.

[Daraan]

Fixes #310

This PR reverts injection of Union[..., NoneType] by typing.get_type_hints in Python <3.11 if a function uses a None default value.

[Daraan]

Alternatively hints[name] = original_value which should be equivalent. I wonder which would be the safer alternative.

[JelleZijlstra]

Using original_value is incorrect as we may have modified the internals of the hint. For example, get_type_hints() turns List["int"] into List[int].

[Daraan]

Thank you for the feedback. Yes, it should have been piped trough _eval_type as well. Can you take a look again?

[JelleZijlstra]

This PR is incorrect for this example:

>>> def f(x: Union[str, None, "str"] = None): pass
... 
>>> typing_extensions.get_type_hints(f)
{'x': <class 'str'>}

I am not sure this approach is viable.

[JelleZijlstra]

Using original_value is incorrect as we may have modified the internals of the hint. For example, get_type_hints() turns List["int"] into List[int].

[Daraan]

What are your thoughts on the "viability"? For that one example or in general?

I tried to improve the recreation of the typing.get_type_hints path that is taken before the injection.

[Daraan]

Thank you for the feedback. Yes, it should have been piped trough _eval_type as well. Can you take a look again?

[JelleZijlstra]

I haven't thought too hard about examples that might break things, but I'm concerned about using == to check whether the two annotations are the same; I don't know if we can rely on equality of annotation objects working reliably for this.

[Daraan]

I haven't thought too hard about examples that might break things, but I'm concerned about using == to check whether the two annotations are the same; I don't know if we can rely on equality of annotation objects working reliably for this.

Do you refer to assertEqual here, or also for the ones in typing_extensions.py?

• I'll also added hash checks, which are fine. str and repr are fine, too.
• assertIs fails in some(to-many) cases depending on the python version.
  In essence the problem can be seen here:

import typing
from typing import List
a = List["str"]

print(typing._eval_type(a, None, None) is typing._eval_type(a, None, None))  # False

typing._eval_type uses _GenericAlias.copy_with or in some cases creates a new GenericAlias which do not make use of the caches.

[JelleZijlstra]

Concatenating these seems a bit odd. In any case, type_hints is a dictionary, so the str() and repr() are the same.

[Daraan]

Thanks for pointing this out I was not 100% sure here and kept a lazy version. Changed to only repr.

[JelleZijlstra]

Why shouldn't this happen? You can put None in annotations.

[Daraan]

At this point a None value should already be converted to NoneType in original_hints. I think these two lines are redundant but to be safe I added the double check.

Added a comment to clarify this.

[JelleZijlstra]

Suggested change

	if sys.version_info < (3, 9) and get_origin(original_evaluated) is Union:
	# Union[str, None, "str"] is not reduced to Union[str, None]
	original_evaluated = Union[original_evaluated.__args__]
	# Compare if values differ
	if original_evaluated != value:
	hints[name] = original_evaluated
	hints[name] = original_evaluated

Would this also work?

[Daraan]

Yes it would. However there are minor differences involving identities and caching.

typing._eval_type uses _GenericAlias.copy_with or in some cases creates a new GenericAlias which do not make use of the caches.

If a ForwardRef is involved original_evaluated can be a complete new instance as _tp_cache is not queried for an existing equivalent one. Not changing hints["x"] will return an object that matches expected["x"] by identity in more cases.

That's a minor feature and I am fine with dropping it if you want; for now added a comment to clarify this.

[JelleZijlstra]

It shouldn't be NoneType already, since we get this directly from __annotations__.

[Daraan]

(Edited for more clarity)
Excuse me I have argued wrongly. You are right that None would be from __annotations__. In such a case value should be NoneType. However, in that case _could_be_inserted_optional(NoneType) will skip this iteration (as get_origin(NoneType) is not Union).

That's why code with a original_value that is None should never propagate until this line, but keeping these two lines to keep it safe; updated the comment.

[Daraan]

I am not sure why its still flagged as changes requested from me. I think this should be again waiting for review :)