Refactor and clean up the union type implementation

[serhiy-storchaka]

BPO	44654
Nosy	@gvanrossum, @serhiy-storchaka, @corona10, @miss-islington, @Fidget-Spinner
PRs	bpo-44654: Refactor and clean up the union type implementation #27196 [3.10] bpo-44654: Refactor and clean up the union type implementation (GH-27196) #27219 bpo-44654: Do not export the union type related symbols #27223 [3.10] bpo-44654: Do not export the union type related symbols (GH-27223). #27225

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-07-18.15:51:27.489>
created_at = <Date 2021-07-16.15:24:47.438>
labels = ['interpreter-core', '3.10', '3.11']
title = 'Refactor and clean up the union type implementation'
updated_at = <Date 2021-07-18.15:51:27.488>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-07-18.15:51:27.488>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2021-07-18.15:51:27.489>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2021-07-16.15:24:47.438>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 44654
keywords = ['patch']
message_count = 7.0
messages = ['397635', '397637', '397644', '397729', '397730', '397747', '397754']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'serhiy.storchaka', 'corona10', 'miss-islington', 'kj']
pr_nums = ['27196', '27219', '27223', '27225']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44654'
versions = ['Python 3.10', 'Python 3.11']

[serhiy-storchaka]

I started reviewing and rewriting Objects/unionobject.c several weeks ago. Some discovered bugs were reported and fixed in separate issues: bpo-44606, bpo-44632, bpo-44635, bpo-44636, bpo-44646, bpo-44652. Before fixing the remaining bugs (bpo-44633, bpo-44642, bpo-44653) 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.

[Fidget-Spinner]

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 #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 :)!

[serhiy-storchaka]

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.

[serhiy-storchaka]

New changeset 0fd2737 by Serhiy Storchaka in branch 'main':
bpo-44654: Refactor and clean up the union type implementation (GH-27196)
0fd2737

[serhiy-storchaka]

New changeset 03aad30 by Miss Islington (bot) in branch '3.10':
[3.10] bpo-44654: Refactor and clean up the union type implementation (GH-27196) (GH-27219)
03aad30

[serhiy-storchaka]

New changeset 8f50f44 by Serhiy Storchaka in branch 'main':
bpo-44654: Do not export the union type related symbols (GH-27223)
8f50f44

[serhiy-storchaka]

New changeset a6670cd by Serhiy Storchaka in branch '3.10':
[3.10] bpo-44654: Do not export the union type related symbols (GH-27223). (GH-27225)
a6670cd