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

Daraan · 2024-10-09T09:22:11Z

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 · 2024-10-09T09:25:32Z

src/typing_extensions.py

+# Values was not modified or original is already Optional
+iforiginal_value==valueor_could_be_inserted_optional(original_value):
+continue
+# NoneType was added to value


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

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

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

src/test_typing_extensions.py

src/typing_extensions.py

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.

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-09T11:58:43Z

src/typing_extensions.py

+# Values was not modified or original is already Optional
+iforiginal_value==valueor_could_be_inserted_optional(original_value):
+continue
+# NoneType was added to value


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 · 2024-10-09T16:49:42Z

src/typing_extensions.py

+# Values was not modified or original is already Optional
+iforiginal_value==valueor_could_be_inserted_optional(original_value):
+continue
+# NoneType was added to value


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

src/typing_extensions.py

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-09T18:19:09Z

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 · 2024-10-10T16:07:34Z

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:

importtypingfromtypingimportLista=List["str"] print(typing._eval_type(a, None, None) istyping._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.

fixed wrong variable and format

JelleZijlstra · 2024-10-21T11:48:10Z

src/test_typing_extensions.py

+withself.subTest("Check str and repr"):
+ifskip_reason=="UnionType not preserved in 3.10":
+self.skipTest(skip_reason)
+self.assertEqual(str(type_hints) +repr(type_hints), str(expected) +repr(expected))


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

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

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-21T11:53:12Z

src/typing_extensions.py

+ ):
+continue
+original_value=original_hints[name]
+iforiginal_valueisNone: # should not happen


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

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 · 2024-10-21T11:57:47Z

src/typing_extensions.py

+ifsys.version_info< (3, 9) andget_origin(original_evaluated) isUnion:
+# Union[str, None, "str"] is not reduced to Union[str, None]
+original_evaluated=Union[original_evaluated.__args__]
+# Compare if values differ
+iforiginal_evaluated!=value:
+hints[name] =original_evaluated


Suggested change
ifsys.version_info< (3, 9) andget_origin(original_evaluated) isUnion:
# Union[str, None, "str"] is not reduced to Union[str, None]
original_evaluated=Union[original_evaluated.__args__]
# Compare if values differ
iforiginal_evaluated!=value:
hints[name] =original_evaluated
hints[name] =original_evaluated
Would this also work?

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.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

+ updated comments

src/test_typing_extensions.py

JelleZijlstra · 2024-10-21T16:56:44Z

src/typing_extensions.py

 continue
 original_value=original_hints[name]
-iforiginal_valueisNone: # should not happen
+iforiginal_valueisNone: # should be NoneType already; check just in case


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

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

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Daraan · 2025-01-22T11:24:10Z

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

Daraan added 3 commits October 9, 2024 11:18

Fix get_type_hints with None default interaction
376ae56

moved section
24c5602

removed empty line
7f000b1

Daraan commented Oct 9, 2024
View reviewed changes

JelleZijlstra requested changes Oct 9, 2024
View reviewed changes

Daraan marked this pull request as draft October 9, 2024 13:07

Daraan added 2 commits October 9, 2024 18:34

Restructured tests and applied suggested changes
8cc4464

+1 test and formatting
881ffee

Daraan marked this pull request as ready for review October 9, 2024 16:46

Daraan commented Oct 9, 2024
View reviewed changes

Daraan added 3 commits October 10, 2024 17:56

Increase test coverage
58082b9

use key access and not values
5c94bd6

str test passes as well
b42e203

removed invalid 3.8 code
ed552e4

Daraan marked this pull request as draft October 10, 2024 17:45

Daraan added 3 commits October 10, 2024 23:34

Extended tests
313ddd8
fixed wrong variable and format

refinement of tests, more tests with aliases
3ba4ee7

Merge 'main' into get_type_hints
6ffcd23

Daraan marked this pull request as ready for review October 11, 2024 15:10

Merge branch 'main' into get_type_hints
403e7ce

JelleZijlstra reviewed Oct 21, 2024
View reviewed changes

Daraanand others added 2 commits October 21, 2024 15:10

Add comment for special case
52c93e9
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Assure UnionType stays UnionType; check repr only
a1777f3
+ updated comments

JelleZijlstra reviewed Oct 21, 2024
View reviewed changes

src/test_typing_extensions.py Outdated Show resolvedHide resolved

JelleZijlstra reviewed Oct 21, 2024
View reviewed changes

Daraanand others added 3 commits October 21, 2024 18:57

Update src/test_typing_extensions.py
9e59796
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Merge remote-tracking branch 'upstream/main' into get_type_hints
1761a43

Corrected and clarified case
54b8eb0

Daraan added 3 commits October 22, 2024 16:14

Merge remote-tracking branch 'upstream/main' into get_type_hints
5612f6f

Merge branch 'main' into get_type_hints
91075ff

Merge branch 'main' into get_type_hints
1eac721

Daraan requested a review from JelleZijlstra November 25, 2024 10:59

Daraan added 5 commits November 27, 2024 12:46

Merge branch 'main' into get_type_hints
1f84c68

Merge branch 'main' into get_type_hints
214dfef

Merge branch 'main' into get_type_hints
00dedc2

Merge branch 'main' into get_type_hints
e929085

Merge branch 'main' into get_type_hints
47e47ad

JelleZijlstra approved these changes Feb 11, 2025
View reviewed changes

JelleZijlstra merged commit 86cf372 into python:mainFeb 11, 2025
23 checks passed

brianschubert mentioned this pull request Aug 27, 2025
fix: Interaction with get_type_hints and Annotated <3.11. #312
Draft

Uh oh!

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

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

Uh oh!

Conversation

Daraan commented Oct 9, 2024

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!

Uh oh!

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Daraan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelleZijlstra commented Oct 9, 2024

Uh oh!

Daraan commented Oct 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaraanOct 21, 2024• 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!

Uh oh!

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!

DaraanOct 21, 2024• 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaraanOct 21, 2024• 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!

Daraan commented Jan 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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

Daraan commented Oct 10, 2024•
edited
Loading

DaraanOct 21, 2024•
edited
Loading

DaraanOct 21, 2024•
edited
Loading

DaraanOct 21, 2024•
edited
Loading