Skip to content

bpo-37806:Add check recursion in typeing.ForwardRef when get hash. #15559

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.

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

Conversation

hongweipeng
Copy link
Contributor

@hongweipeng hongweipeng commented Aug 28, 2019

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks! I have few comments. Also I think this requires some tests.

@@ -0,0 +1 @@
Fix infinite recursion from :func:`typing.get_type_hints`.Patch by hongweipeng.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Fix infinite recursion from :func:`typing.get_type_hints`.Patch by hongweipeng.
Fix infinite recursion from :func:`typing.get_type_hints`. Patch by hongweipeng.

Lib/typing.py Outdated
if isinstance(val, _GenericAlias):
self._check_recursion(val, forward_args)
elif self is val and self.__forward_evaluated__:
forward_args.append(...)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to create an actually recursive object? Or is it too dangerous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does an actually recursive object mean? I added some test code in new commit. Can you have a look and see what other tests need to be added. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a = List['int']
for i in range(sys.getrecursionlimit() + 100):
    a = List[a]

It raises a RecursionError both in this PR and master branch. Is an actually recursive object mean this?

Copy link
Member

Choose a reason for hiding this comment

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

No. Consider the initial example:

ValueList = List['Value']
Value = Union[str, ValueList]

In this example I mean trying to construct an object Value such that Value.__args__[1].__args__[0] is Value. I understand this may be tricky, but I don't think it is impossible.

Also it indeed looks like this may be "dangerous" in the sense that we need to make __repr__(), __eq__(), and __hash__() aware of the possible recursion.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand:

>>> x = [1]
>>> x.append(x)
>>> x
[1, [...]]
>>> y = [1]
>>> y.append(y)
>>> x == y
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RecursionError: maximum recursion depth exceeded in comparison

So maybe it would be enough to just update the __repr__().

@mangrisano
Copy link
Contributor

/cc @ilevkivskyi

@hongweipeng
Copy link
Contributor Author

PR #15650 can solve this issue.

@hongweipeng hongweipeng closed this Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants