Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 5 commits into from
+118 −0

Conversation

Daraan
Copy link
Contributor

@Daraan Daraan commented Oct 9, 2024

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
Daraan commented Oct 9, 2024
View reviewed changes
src/typing_extensions.py Outdated
# Values was not modified or original is already Optional
if original_value == value or _could_be_inserted_optional(original_value):
continue
# NoneType was added to value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
@@ -1645,6 +1645,37 @@ def test_final_forward_ref(self):
self.assertNotEqual(gth(Loop, globals())['attr'], Final[int])
self.assertNotEqual(gth(Loop, globals())['attr'], Final)

def test_annotation_and_optional_default(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that more tests are necessary? Do you have ideas where something could go wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More tests have been added, covering the mentioned cases.

src/typing_extensions.py
_NoneType = type(None)

def _could_be_inserted_optional(t):
"""detects Union[..., None] pattern"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_UnionGenericAlias does not exist in the whole version range. Is this sufficient to assure that it's a Union? I am not 100% sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to get_origin.

JelleZijlstra
JelleZijlstra requested changes Oct 9, 2024
View reviewed changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/typing_extensions.py
# when a None default value is used.
# see https://github.com/python/typing_extensions/issues/310
original_hints = getattr(obj, '__annotations__', None)
defaults = typing._get_defaults(obj)
Copy link
Member

Choose a reason for hiding this comment

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

Should exit early if there are no defaults. The current code risks accessing .__annotations__ directly on classes, which could have unwanted effects.

src/typing_extensions.py Outdated Show resolved Hide resolved
src/typing_extensions.py Outdated
# Values was not modified or original is already Optional
if original_value == value or _could_be_inserted_optional(original_value):
continue
# NoneType was added to value
Copy link
Member

Choose a reason for hiding this comment

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

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 Daraan marked this pull request as draft October 9, 2024 13:07
@Daraan Daraan marked this pull request as ready for review October 9, 2024 16:46
Daraan
Daraan commented Oct 9, 2024
View reviewed changes
Copy link
Contributor Author

@Daraan Daraan left a comment

Choose a reason for hiding this comment

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

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.

src/typing_extensions.py Outdated
# Values was not modified or original is already Optional
if original_value == value or _could_be_inserted_optional(original_value):
continue
# NoneType was added to value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/test_typing_extensions.py
@@ -1645,6 +1645,37 @@ def test_final_forward_ref(self):
self.assertNotEqual(gth(Loop, globals())['attr'], Final[int])
self.assertNotEqual(gth(Loop, globals())['attr'], Final)

def test_annotation_and_optional_default(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More tests have been added, covering the mentioned cases.

src/typing_extensions.py
_NoneType = type(None)

def _could_be_inserted_optional(t):
"""detects Union[..., None] pattern"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to get_origin.

@JelleZijlstra
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Annotated/get_type_hints interaction in python <3.11
2 participants
@Daraan @JelleZijlstra