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

gh-92261: Disallow iteration of Union (and other _SpecialForms) #92262

Merged
merged 10 commits into from
May 8, 2022
Merged

gh-92261: Disallow iteration of Union (and other _SpecialForms) #92262

merged 10 commits into from
May 8, 2022
+72 −5

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented May 3, 2022 •
edited
Loading

As #92261 describes, list(Union) currently results in a hang because _SpecialForm.__getitem__ is used for iteration, meaning that list(Union) effectively does Union[0], Union[1], ....

This PR fixes it by implementing _SpecialForm.__iter__, which takes priority over _SpecialForm.__getitem__ when iterating.

Edit: and now also explicitly setting __iter__ = None on a number of other things in typing.py, to prevent similar errors with other things.

JelleZijlstra
JelleZijlstra reviewed May 3, 2022
View reviewed changes
Lib/typing.py Outdated Show resolved Hide resolved
carljm
carljm approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good!

Lib/typing.py Outdated Show resolved Hide resolved
AlexWaygood
AlexWaygood reviewed May 3, 2022
View reviewed changes
Lib/typing.py Outdated Show resolved Hide resolved
serhiy-storchaka
serhiy-storchaka reviewed May 4, 2022
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

What about list(list)?

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra mentioned this pull request May 5, 2022
Open
@mrahtz
Copy link
Contributor Author

mrahtz commented May 7, 2022

[Serhiy]

What about list(list)?

I've added a couple of tests to test_genericalias.py.

@serhiy-storchaka
Copy link
Member

For me, the style is too verbose, but the code does everything right, and I don't want to be too picky.

@serhiy-storchaka serhiy-storchaka merged commit 4739997 into python:main May 8, 2022
@mrahtz mrahtz deleted the disallow-union-iteration branch May 8, 2022 16:28
@JelleZijlstra
Copy link
Member

Should we apply this to 3.11 too?

gvanrossum
gvanrossum reviewed May 9, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

For a pure bugfix it seems rather involved. And since it is only intending to report a better error for something that's clearly invalid, I am not inclined to backport it. (Even though it has so many pieces that it will likely cause a fair amount of merge issues for future, simpler, bugfixes that do deserve being backported.

@JelleZijlstra
Copy link
Member

It's not just an error message though: on 3.11 list(Union) will be an infinite loop that consumes more and more memory. Before 3.10 it would fail quickly because typing._type_check rejects ints. It's unfortunate if <=3.10 and >=3.12 fail quickly on list(Union) but 3.11 produces an infinite hang until you use all available memory.

@gvanrossum
Copy link
Member

Okay, then I'll leave it to your judgment (and that of @pablogsal). Be sure to also backport the one-line fix for slots. :-)

@JelleZijlstra JelleZijlstra added the needs backport to 3.11 only security fixes label May 9, 2022
@miss-islington
Copy link
Contributor

Thanks @mrahtz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2022
@mrahtz @miss-islington
pythongh-92261: Disallow iteration of Union (and other special forms) (…
ea6cf7e 
…pythonGH-92262)

(cherry picked from commit 4739997)

Co-authored-by: Matthew Rahtz <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 9, 2022
@bedevere-bot
Copy link

GH-92582 is a backport of this pull request to the 3.11 branch.

serhiy-storchaka pushed a commit that referenced this pull request May 10, 2022
@miss-islington @mrahtz
gh-92261: Disallow iteration of Union (and other special forms) (GH-9…
7540a43 
…2262) (GH-92582)

(cherry picked from commit 4739997)

Co-authored-by: Matthew Rahtz <[email protected]>
@rruuaanng rruuaanng mentioned this pull request Nov 2, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@gvanrossum gvanrossum gvanrossum left review comments

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@carljm carljm carljm approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Assignees
No one assigned
Labels
topic-typing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mrahtz @serhiy-storchaka @JelleZijlstra @gvanrossum @miss-islington @bedevere-bot @carljm @AlexWaygood