bpo-44490: Add __parameters__ and __getitem__ to types.Union by uriyyo · Pull Request #26980 · python/cpython

uriyyo · 2021-07-01T09:04:08Z

https://bugs.python.org/issue44490

uriyyo · 2021-07-01T09:04:34Z

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner

@uriyyo thank you for sending this PR so quickly! Excellent work. I only have a few minor readability nits below.

uriyyo · 2021-07-02T08:52:19Z

@Fidget-Spinner Thanks for review, I have added tests that you suggested.

Fidget-Spinner · 2021-07-02T09:51:30Z

@Fidget-Spinner Thanks for review, I have added tests that you suggested.

Thanks for applying the suggestions. This LGTM. I've requested a review from Guido and I'm waiting for him to take a look when he has the time. I can't merge PRs, sorry.

Fidget-Spinner · 2021-07-03T12:53:24Z

@uriyyo can you please rebase this against main branch? I fixed a GC bug in Union in 1097384, and I think you may have to add Py_VISIT(alias->parameters) in union_traverse Thanks.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

…90.xY80VR.rst Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

uriyyo · 2021-07-03T14:29:32Z

@Fidget-Spinner Thanks, fixed

gvanrossum

LG, I would just update two comments slightly.

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>

Fidget-Spinner · 2021-07-05T17:42:17Z

Eventually if this PR is merged, we may have to consider backporting this to 3.10. Since it's a somewhat big change, I'm pinging @pablogsal as the 3.10 RM for his awareness.

TLDR: Some uncommon but valid use cases not covered by PEP 604 were discovered in bpo. This PR adds support for them. Can it land in 3.10 beta 4 too?

pablogsal · 2021-07-05T19:02:21Z

Eventually if this PR is merged, we may have to consider backporting this to 3.10. Since it's a somewhat big change, I'm pinging @pablogsal as the 3.10 RM for his awareness.
TLDR: Some uncommon but valid use cases not covered by PEP 604 were discovered in bpo. This PR adds support for them. Can it land in 3.10 beta 4 too?

We are too far in the development cycle to add new features, specially stuff that is touching c code. I do understand that this is something new in 3.10 so we do not have possibility of regressions and I see that this is an important point. I think we are fine to get this in with the following requirements:

Is merged before the last beta (beta4)
At least 2 core Devs review and approve the PR

gvanrossum

Thanks!

gvanrossum · 2021-07-05T20:50:34Z

@Fidget-Spinner Can you think of another core dev to review this (not Pablo)?

pablogsal · 2021-07-05T22:21:20Z

@Fidget-Spinner Can you think of another core dev to review this (not Pablo)?

I can do a review if you don't find anyone else, but currently, I am flooded with PEP 657 stuff so it will probably be in a couple of days

bedevere-bot · 2021-07-05T22:22:49Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 7799a15 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Fidget-Spinner · 2021-07-05T23:51:02Z

@Fidget-Spinner Can you think of another core dev to review this (not Pablo)?

@serhiy-storchaka , are you alright with reviewing this?

In the meantime, the Windows 10 buildbot failures seem potentially related so @uriyyo please look into them. I'll take a look too in a day or two.

pablogsal · 2021-07-06T00:01:20Z

In the meantime, the Windows 10 buildbot failures seem potentially related so @uriyyo please look into them. I'll take a look too in a day or two.

I am restarting them as I am quite sure this is due to corrupted pyc files from a previous run. New builds here:

https://buildbot.python.org/all/#/builders/577/builds/86
https://buildbot.python.org/all/#/builders/593/builds/89

Fidget-Spinner · 2021-07-06T14:30:05Z

In the meantime, the Windows 10 buildbot failures seem potentially related so @uriyyo please look into them. I'll take a look too in a day or two.
I am restarting them as I am quite sure this is due to corrupted pyc files from a previous run. New builds here:
https://buildbot.python.org/all/#/builders/577/builds/86
https://buildbot.python.org/all/#/builders/593/builds/89

Oh you're right. Sorry @uriyyo if I led you on a wild goose chase. I suspect one of two things

maybe someone forgot to bump the magic number for a Windows-related change.
the worker maybe didn't clean up old files properly -- I suspect it's this one because I cannot reproduce this locally on Windows machine.

Fidget-Spinner · 2021-07-06T15:55:34Z

After spending a few hours investigating this. I think our best bet is to merge latest main into this PR and try running the buildbots again. I have no clue why those 2 buildbots fail and any help would be much appreciated. Some findings:

They are 2 different workers. I find it unlikely that 2 machines could both have corrupted/out of date pyc files coincidentally.
I've tried building this locally with same build flags as the buildbots, but I can't reproduce it.
I noticed the buildbot/clean.bat script fails on both. But they've been like this for at least a month and I suspect it's because there are no leftover pyc files to clean. Maybe the buildbots already clean it up themselves?

gvanrossum · 2021-07-06T17:11:10Z

Yeah, merge latest main is definitely a good first step.

uriyyo · 2021-07-06T17:52:27Z

@gvanrossum @Fidget-Spinner I have merged latest main into PR branch.

And I have a question should we fix more cases in the scope of this PR?

types.Union is not serializable and can not be used with pickle.
types.Union can not be used with typing.Annotated.
typing._collect_type_vars currently do not collect params from the types.Union.

>>>typing.List[list[T] |float].__parameters__
()

UPD. Actually, there is a lot of case at typing module where types.Union is not supported but I believe it should be.

gvanrossum · 2021-07-06T18:04:14Z

Tests pass now, I propose to merge now and do another PR for the other issues.

We still need another core dev approval, but I'm merging now. ("Merged before b4" was one of the requirements, "two core devs" is the other.)

uriyyo · 2021-07-06T18:08:08Z

Great, I will open another PR to cover cases that I mentioned.

…ugfix backports Co-Authored-By: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-Authored-By: Guido van Rossum <gvanrossum@gmail.com> Co-Authored-By: Yurii Karabas <1998uriyyo@gmail.com>

…ythonGH-26980) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>. (cherry picked from commit c45fa1a) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>

serhiy-storchaka · 2021-07-17T07:01:35Z

There are several bugs (bpo-44633, bpo-44653), but I am going to fix them soon.

I am +1 for backporting this feature to 3.10.

uriyyo · 2021-07-17T09:12:16Z

@serhiy-storchaka Do you need help with it? There is also issue with types.Union serialisation that I am going to fix.

pablogsal · 2021-07-17T15:16:32Z

There are several bugs (bpo-44633, bpo-44653), but I am going to fix them soon.
I am +1 for backporting this feature to 3.10.

Please, see my other comment: #27207 (comment)

…H-26980) (GH-27207) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>. (cherry picked from commit c45fa1a) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

the-knights-who-say-ni added the CLA signed label Jul 1, 2021

bedevere-bot added the awaiting review label Jul 1, 2021

Fidget-Spinner approved these changes Jul 1, 2021
View reviewed changes

Comment threadLib/test/test_types.py Outdated
Comment threadMisc/NEWS.d/next/Core and Builtins/2021-07-01-11-59-34.bpo-44490.xY80VR.rst Outdated
Comment threadObjects/unionobject.c Outdated
Comment threadInclude/genericaliasobject.h Outdated

bedevere-bot added awaiting core review and removed awaiting review labels Jul 1, 2021

Fidget-Spinner requested a review from gvanrossum July 1, 2021 15:57

Fidget-Spinner reviewed Jul 1, 2021
View reviewed changes

Comment threadLib/test/test_types.py Outdated

uriyyoand others added 11 commits July 3, 2021 17:26

bpo-44490: Add __parameters__ to types.Union
bb7713b

Add Py_LIMITED_API to genericaliasobject
4d1a60b

Update Lib/test/test_types.py
71b306e
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Update Misc/NEWS.d/next/Core and Builtins/2021-07-01-11-59-34.bpo-444…
09d441c
…90.xY80VR.rst Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Update Objects/unionobject.c
8033e53
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Update Include/genericaliasobject.h
59f54af
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Update genericaliasobject.c
2072f7e

Update unionobject.c
cea63d4

Update Lib/test/test_types.py
27980f1
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Update test_types.py
afbb038

Add parameters to union_traverse
8cfa980

uriyyo force-pushed the fix-issue-44490 branch from cddb7e0 to 8cfa980Compare July 3, 2021 14:29

gvanrossum reviewed Jul 5, 2021
View reviewed changes

Comment threadLib/test/test_types.py
Comment threadObjects/genericaliasobject.c Outdated
Comment threadObjects/unionobject.c Outdated

uriyyoand others added 2 commits July 5, 2021 20:00

Update Objects/genericaliasobject.c
1a1ac84
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>

Add more tests to cover types.Union
7799a15

uriyyo requested a review from gvanrossum July 5, 2021 17:12

gvanrossum approved these changes Jul 5, 2021
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 5, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 5, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 5, 2021

Merge remote-tracking branch 'upstream/main' into fix-issue-44490
0b61365

gvanrossum merged commit c45fa1a into python:mainJul 6, 2021

bedevere-bot removed the awaiting merge label Jul 6, 2021

Fidget-Spinner mentioned this pull request Jul 17, 2021
bpo-44490: Improve typing module compatibility with types.Union #27048
Merged

Fidget-Spinner mentioned this pull request Jul 17, 2021
[3.10] bpo-44490: Partially backport GH-26980's refactoring for easier bugfix backports #27203
Closed

Fidget-Spinner mentioned this pull request Jul 17, 2021
[3.10] bpo-44490: Add __parameters__ and __getitem__ to types.Union (GH-26980) #27207
Merged

pablogsal mentioned this pull request Jul 18, 2021
[3.10] Correct the order of regen-abidump #27228
Closed

Uh oh!

Uh oh!

Conversation

uriyyo commented Jul 1, 2021• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uriyyo commented Jul 1, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uriyyo commented Jul 2, 2021

Uh oh!

Fidget-Spinner commented Jul 2, 2021

Uh oh!

Fidget-Spinner commented Jul 3, 2021

Uh oh!

uriyyo commented Jul 3, 2021

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner commented Jul 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jul 5, 2021

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Jul 5, 2021

Uh oh!

pablogsal commented Jul 5, 2021

Uh oh!

bedevere-bot commented Jul 5, 2021

Uh oh!

Fidget-Spinner commented Jul 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jul 6, 2021

Uh oh!

Fidget-Spinner commented Jul 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner commented Jul 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jul 6, 2021

Uh oh!

uriyyo commented Jul 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jul 6, 2021

Uh oh!

uriyyo commented Jul 6, 2021

Uh oh!

serhiy-storchaka commented Jul 17, 2021• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uriyyo commented Jul 17, 2021

Uh oh!

pablogsal commented Jul 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

uriyyo commented Jul 1, 2021•
edited by bedevere-bot
Loading

Fidget-Spinner commented Jul 5, 2021•
edited
Loading

Fidget-Spinner commented Jul 5, 2021•
edited
Loading

Fidget-Spinner commented Jul 6, 2021•
edited
Loading

Fidget-Spinner commented Jul 6, 2021•
edited
Loading

uriyyo commented Jul 6, 2021•
edited
Loading

serhiy-storchaka commented Jul 17, 2021•
edited by bedevere-bot
Loading