classification
Title: Refactor and clean up the union type implementation
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.11, Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, gvanrossum, kj, miss-islington, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-07-16 15:24 by serhiy.storchaka, last changed 2021-07-18 15:51 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27196 merged serhiy.storchaka, 2021-07-16 17:19
PR 27219 merged miss-islington, 2021-07-17 19:44
PR 27223 merged serhiy.storchaka, 2021-07-18 09:09
PR 27225 merged serhiy.storchaka, 2021-07-18 12:57
Messages (7)
msg397635 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-16 15:24
I started reviewing and rewriting Objects/unionobject.c several weeks ago. Some discovered bugs were reported and fixed in separate issues: issue44606, issue44632, issue44635, issue44636, issue44646, issue44652. Before fixing the remaining bugs (issue44633, issue44642, issue44653) I want to merge some minor changes:

* Rename _Py_UnionType to _PyUnion_Type. It is how PyTypeObject instances are named.
* Add _PyUnion_Check() and _PyGenericAlias_Check(). It simplified the code.
* Do not expose _Py_Union(). It is only used locally.
* Move declarations of _Py_make_parameters and _Py_subs_parameters from genericaliasobject.h to internal/pycore_unionobject.h. They are for internal use only.
* Fix a possible leak in dedup_and_flatten_args().
* Significantly simplify union_richcompare(). The Python implementation can be used for comparing types.Union with typing.Union.
* Perform cheaper tests before more expensive tests in is_unionable().
* Optimize __module__ look up in is_typing_module() and support types without __module__.
* Fix outdated comments.
* Minor style fixes.
* Extract related tests to separate class.
* Make some tests more accurate: they should test for TypeError only specific operators, not constructors.

I am going to backport these changes to 3.10 to make backporting of future fixes easier.
msg397637 - (view) Author: Ken Jin (kj) * (Python committer) Date: 2021-07-16 15:42
> I am going to backport these changes to 3.10 to make backporting of future fixes easier.
> * Move declarations of _Py_make_parameters and _Py_subs_parameters ...

Unfortunately, _Py_make_parameters and _Py_subs_parameters are 3.11 only. They were part of https://github.com/python/cpython/pull/26980. And we couldn't backport it to 3.10 (please see the PR thread). Maybe we can ask Pablo to reconsider for addition in 3.10rc1 if we can get a core dev to approve that?

> * Perform cheaper tests before more expensive tests in is_unionable().
> * Optimize __module__ look up in is_typing_module().

Yeah I noticed it re-lookups __module__ unnecessarily multiple times for the same type. I tried fixing this a month ago (and also convert to interned PyId strings) but for some reason microbenchmarks didn't show much gain. Things may be different after you clean things up.

> * Extract related tests to separate class.

Yes please :). Maybe even a separate file altogether. GenericAlias has its own test_genericalias.

A big +1 on everything else. They were bugging me for a while but I didn't have the time to fix them. Thanks for taking this up Serhiy :)!
msg397644 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-16 17:32
> Yeah I noticed it re-lookups __module__ unnecessarily multiple times for the same type.

Actually I thought about this change, but did not implement it. Because it does not look performance critical, and future versions will likely do different tests (for example, testing the existence of special attribute, or check against lazily imported and cached classes from the typing module). Also, the test for TypeVar can be shared between genericaliasobject.c and unionobject.c.

It is mostly simple clean up, a line here, two lines there. genericaliasobject.c and unionobject.c could be merged in a single file, because they share several functions not used anywhere more. We perhaps merge them if more C implementations for typing be added in future.
msg397729 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-17 19:44
New changeset 0fd27375cabd12e68a2f12cfeca11a2d5043429e by Serhiy Storchaka in branch 'main':
bpo-44654: Refactor and clean up the union type implementation (GH-27196)
https://github.com/python/cpython/commit/0fd27375cabd12e68a2f12cfeca11a2d5043429e
msg397730 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-17 21:10
New changeset 03aad3049d1591c76a219dfe089e5367f88f167e by Miss Islington (bot) in branch '3.10':
[3.10] bpo-44654: Refactor and clean up the union type implementation (GH-27196) (GH-27219)
https://github.com/python/cpython/commit/03aad3049d1591c76a219dfe089e5367f88f167e
msg397747 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-18 12:55
New changeset 8f50f44592190b5a8cb115f0d58d577036e68308 by Serhiy Storchaka in branch 'main':
bpo-44654: Do not export the union type related symbols (GH-27223)
https://github.com/python/cpython/commit/8f50f44592190b5a8cb115f0d58d577036e68308
msg397754 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-18 15:37
New changeset a6670cdf77aab2b1ee7be0b9df060dcac2a2dc48 by Serhiy Storchaka in branch '3.10':
[3.10] bpo-44654: Do not export the union type related symbols (GH-27223). (GH-27225)
https://github.com/python/cpython/commit/a6670cdf77aab2b1ee7be0b9df060dcac2a2dc48
History
Date User Action Args
2021-07-18 15:51:27serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-07-18 15:37:49serhiy.storchakasetmessages: + msg397754
2021-07-18 12:57:56serhiy.storchakasetpull_requests: + pull_request25766
2021-07-18 12:55:35serhiy.storchakasetmessages: + msg397747
2021-07-18 09:09:22serhiy.storchakasetpull_requests: + pull_request25764
2021-07-17 21:10:25serhiy.storchakasetmessages: + msg397730
2021-07-17 19:44:40serhiy.storchakasetmessages: + msg397729
2021-07-17 19:44:20miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request25760
2021-07-16 17:32:03serhiy.storchakasetmessages: + msg397644
2021-07-16 17:19:20serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request25731
2021-07-16 16:03:16corona10setnosy: + corona10
2021-07-16 15:42:07kjsetmessages: + msg397637
2021-07-16 15:24:47serhiy.storchakacreate