Skip to content

/ cpython

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

Jump to bottom

bpo-41428: Implementation for PEP 604 #21515

Merged
merged 62 commits into from Sep 9, 2020
Merged

bpo-41428: Implementation for PEP 604 #21515

merged 62 commits into from Sep 9, 2020
+694 −18

Conversation

@MaggieMoss
Copy link
Contributor

@MaggieMoss MaggieMoss commented Jul 17, 2020 •
edited by gvanrossum

PEP 604 - Proposed Implementation

https://www.python.org/dev/peps/pep-0604/

Building off of the comments left on this PR here: https://github.com/pprados/cpython/tree/PEP604

https://bugs.python.org/issue41428
MaggieMoss added 10 commits Jul 10, 2020
@MaggieMoss
Add new type object for union types
4c4c701
@MaggieMoss
Add test.
07b5c29
@MaggieMoss
Add basic tp_* fields to union object
32c207b
@MaggieMoss
Squash this commit.
50b33c2
@MaggieMoss
Squash this commit.
8901145
@MaggieMoss
Add union_traverse method.
6450321
@MaggieMoss
Handle Py_None in union.
8a09a92
@MaggieMoss
Clean up syntax with Py_None
e3c80e9
@MaggieMoss
Update getattro method.
165c323
@MaggieMoss
Reduce code duplication when adding union types together.
Loading status checks…
7f0018e
@MaggieMoss MaggieMoss requested review from gvanrossum, ilevkivskyi and python/windows-team as code owners Jul 17, 2020
@the-knights-who-say-ni

This comment has been hidden.

Sign in to view
@MaggieMoss MaggieMoss marked this pull request as draft Jul 17, 2020
MaggieMoss referenced this pull request in pprados/cpython Jul 17, 2020
@pprados
Implement a POC of the specification describe in the PEP0604
e3e99bc
@MaggieMoss
MaggieMoss reviewed Jul 17, 2020
View changes
Objects/typeobject.c Outdated Show resolved Hide resolved
@MaggieMoss MaggieMoss changed the title Pep605 implementation Pep604 - Draft Implementation Jul 17, 2020
MaggieMoss added 4 commits Jul 17, 2020
@MaggieMoss
Add __or__ method to _SpecialGenericAlias
cd584d5
@MaggieMoss
Add support for NewType with unions.
Loading status checks…
ba031c1
@MaggieMoss
Remove old code from abstract.c
Loading status checks…
e62de73
@MaggieMoss
Fix warning for incompatible pointer type.
f6ab05b
@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jul 17, 2020

Thank you so much! Give me a few days to review this. Ping me if you haven't heard from me by Wednesday!
@gvanrossum gvanrossum changed the title Pep604 - Draft Implementation PEP 604 - Draft Implementation Jul 18, 2020
@gvanrossum
gvanrossum reviewed Jul 18, 2020
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Great start! Here's a boatload of review comments, from whitespace nits to feature requests.
Lib/test/test_isinstance.py Outdated Show resolved Hide resolved
Lib/test/test_types.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Objects/abstract.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Show resolved Hide resolved
Objects/unionobject.c Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
@MaggieMoss
Add support for union use in subclasses.
Loading status checks…
9c20fbc
@gvanrossum
gvanrossum reviewed Jul 19, 2020
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

(Presumably you were going to add more commits before asking for another review?)
Lib/test/test_isinstance.py Outdated Show resolved Hide resolved
Objects/abstract.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
@MaggieMoss
Copy link
Contributor Author

@MaggieMoss MaggieMoss commented Jul 21, 2020

@gvanrossum thanks so much for the comments! I'll implement your suggestions and clean this up :)
@pablogsal
pablogsal reviewed Aug 17, 2020
View changes
Objects/unionobject.c Show resolved Hide resolved
@pablogsal
pablogsal reviewed Aug 17, 2020
View changes
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated
goto done;
}

if (_PyObject_LookupAttrId(p, &PyId___origin__, &tmp) < 0) {

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Aug 17, 2020
Member

It seems to me like we're also updating the tmp variable here, and using it later in the function.

But you throw it away inmediatele:

    if (tmp != NULL) {
        Py_DECREF(tmp);

and then you substitute it for another one:

	        if (_PyObject_LookupAttrId(p, &PyId___args__, &tmp) < 0) {
            goto exit;

that then you throw away immediately:

        if (tmp != NULL) {
            Py_DECREF(tmp);
Objects/unionobject.c Outdated Show resolved Hide resolved
MaggieMoss added 2 commits Aug 18, 2020
@MaggieMoss
Implement code review suggestions
385a338 
- Use goto target to reduce code repetition.
- Fix style nits.
- Rename method check_args -> is_generic_alias_in_args.
- Fix potential reference leaks.
- Check for error result from is_unionable.
- Small refactor to repr method
@MaggieMoss
Refactor and remove PyUnion_New
Loading status checks…
dddc598
@MaggieMoss
Copy link
Contributor Author

@MaggieMoss MaggieMoss commented Aug 18, 2020 •
edited

But users can call Py_Union without calling union_new no? Is part of the public C-API

Yup, this was how it worked. I refactored so when calling Py_Union the arguments are checked, and we no longer need a separate union_new method. Let me know if you have any comments / questions here.

@pablogsal thanks for another round of reviews :)
@MaggieMoss MaggieMoss requested a review from pablogsal Aug 18, 2020
@MaggieMoss
Copy link
Contributor Author

@MaggieMoss MaggieMoss commented Aug 20, 2020

@gvanrossum

I had thought of another case we might need to handle here, and was hoping to get your input. Should this:

typing.Union[int, str] is (int | str)

evaluate to true?
@MaggieMoss
Copy link
Contributor Author

@MaggieMoss MaggieMoss commented Aug 23, 2020

@pablogsal let me know if there are any other changes you'd like to see here.
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Aug 23, 2020

@pablogsal let me know if there are any other changes you'd like to see here.

I just need to find some time to do another iteration 😉 Will try to do another pass by tomorrow, but we are very close! Thanks for the patience and tye good work :)
@pablogsal
pablogsal reviewed Sep 8, 2020
View changes
Objects/unionobject.c Outdated Show resolved Hide resolved
@pablogsal
pablogsal reviewed Sep 8, 2020
View changes
Include/unionobject.h Outdated Show resolved Hide resolved
@pablogsal
General cleanup, fix reference leaks, add new test for possible crashes
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
8c06c1d
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 8, 2020

@MaggieMoss I have pushed a new commit (8c06c1d) to do some simplifications and to fix some minor reference count issues and crashes. Please, check out that everything seems ok in that commit :)
pablogsal added 2 commits Sep 8, 2020
@pablogsal
Add newline
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
37699f1
@pablogsal
Make unionobject APIs private
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
47a4cb3
@pablogsal
pablogsal approved these changes Sep 9, 2020
View changes
Copy link
Member

@pablogsal pablogsal left a comment

This looks good to me!

Thanks for all the work and patience @MaggieMoss! Great job :)

@gvanrossum I won't land it myself in case you or Maggie want to take a last check.
@pablogsal
pablogsal reviewed Sep 9, 2020
View changes
Objects/unionobject.c Outdated Show resolved Hide resolved
@pablogsal
Add minor test for GenericAlias
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
7f89abd
@gvanrossum
gvanrossum approved these changes Sep 9, 2020
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

I think it's good! Thanks so much Maggie for all your work.
@MaggieMoss
Remove ellipsis from repr.
Loading status checks…
d0be56f
@MaggieMoss
Copy link
Contributor Author

@MaggieMoss MaggieMoss commented Sep 9, 2020

Thanks so much for all of your reviews and comments, I've really appreciated it. 🎉
@MaggieMoss
Remove commented out test code.
Loading status checks…
59f06f7
@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 9, 2020

@MaggieMoss
I noticed that some tests failed on Windows. Could it be that you need to rebase to the latest master?

"test.test_asyncio.test_sendfile.ProactorEventLoopTests.test_sendfile_close_peer_in_the_middle_of_receiving"
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 9, 2020

@MaggieMoss
I noticed that some tests failed on Windows. Could it be that you need to rebase to the latest master?

"test.test_asyncio.test_sendfile.ProactorEventLoopTests.test_sendfile_close_peer_in_the_middle_of_receiving"

That's an unrelated race condition in asyncio in Windows, we can ignore it and just land
@pablogsal pablogsal merged commit 1b4552c into python:master Sep 9, 2020
9 of 10 checks passed
9 of 10 checks passed
Docs
Details
Check for source changes
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200909.28 failed
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 41428 found
Details
bedevere/news News entry found in Misc/NEWS.d
@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 9, 2020

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

@pablogsal pablogsal

@gvanrossum gvanrossum

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@MaggieMoss @the-knights-who-say-ni @gvanrossum @pablogsal @bedevere-bot
You can’t perform that action at this time.