bpo-44490: Add parameters and getitem to types.Union #26980

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.

Lib/test/test_types.py

Misc/NEWS.d/next/Core and Builtins/2021-07-01-11-59-34.bpo-44490.xY80VR.rst

Objects/unionobject.c

Include/genericaliasobject.h

Lib/test/test_types.py

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.

Lib/test/test_types.py

Objects/genericaliasobject.c

Objects/unionobject.c

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

Lib/test/test_types.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2021-07-01-11-59-34.bpo-44490.xY80VR.rst Outdated Show resolved Hide resolved

Objects/unionobject.c Outdated Show resolved Hide resolved

Include/genericaliasobject.h Outdated Show resolved Hide resolved

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

Lib/test/test_types.py Show resolved Hide resolved

uriyyo and 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 8cfa980 Compare July 3, 2021 14:29

gvanrossum reviewed Jul 5, 2021

View reviewed changes

Lib/test/test_types.py Show resolved Hide resolved

Objects/genericaliasobject.c Outdated Show resolved Hide resolved

Objects/unionobject.c Outdated Show resolved Hide resolved

uriyyo and 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:main Jul 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-44490: Add parameters and getitem to types.Union #26980

bpo-44490: Add parameters and getitem to types.Union #26980

uriyyo commented Jul 1, 2021 •

edited by bedevere-bot

Loading

uriyyo commented Jul 1, 2021

Fidget-Spinner left a comment

uriyyo commented Jul 2, 2021

Fidget-Spinner commented Jul 2, 2021

Fidget-Spinner commented Jul 3, 2021

uriyyo commented Jul 3, 2021

gvanrossum left a comment

Fidget-Spinner commented Jul 5, 2021 •

edited

Loading

pablogsal commented Jul 5, 2021

gvanrossum left a comment

gvanrossum commented Jul 5, 2021

pablogsal commented Jul 5, 2021

bedevere-bot commented Jul 5, 2021

Fidget-Spinner commented Jul 5, 2021 •

edited

Loading

pablogsal commented Jul 6, 2021

Fidget-Spinner commented Jul 6, 2021 •

edited

Loading

Fidget-Spinner commented Jul 6, 2021 •

edited

Loading

gvanrossum commented Jul 6, 2021

uriyyo commented Jul 6, 2021 •

edited

Loading

gvanrossum commented Jul 6, 2021

uriyyo commented Jul 6, 2021

serhiy-storchaka commented Jul 17, 2021 •

edited by bedevere-bot

Loading

uriyyo commented Jul 17, 2021

pablogsal commented Jul 17, 2021

bpo-44490: Add __parameters__ and __getitem__ to types.Union #26980

bpo-44490: Add __parameters__ and __getitem__ to types.Union #26980

Conversation

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

uriyyo commented Jul 1, 2021

Fidget-Spinner left a comment

Choose a reason for hiding this comment

uriyyo commented Jul 2, 2021

Fidget-Spinner commented Jul 2, 2021

Fidget-Spinner commented Jul 3, 2021

uriyyo commented Jul 3, 2021

gvanrossum left a comment

Choose a reason for hiding this comment

Fidget-Spinner commented Jul 5, 2021 • edited Loading

pablogsal commented Jul 5, 2021

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented Jul 5, 2021

pablogsal commented Jul 5, 2021

bedevere-bot commented Jul 5, 2021

Fidget-Spinner commented Jul 5, 2021 • edited Loading

pablogsal commented Jul 6, 2021

Fidget-Spinner commented Jul 6, 2021 • edited Loading

Fidget-Spinner commented Jul 6, 2021 • edited Loading

gvanrossum commented Jul 6, 2021

uriyyo commented Jul 6, 2021 • edited Loading

gvanrossum commented Jul 6, 2021

uriyyo commented Jul 6, 2021

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

uriyyo commented Jul 17, 2021

pablogsal commented Jul 17, 2021

bpo-44490: Add parameters and getitem to types.Union #26980

bpo-44490: Add parameters and getitem to types.Union #26980

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