Skip to content

Refactor and clean up the union type implementation #88820

Closed
Closed
Refactor and clean up the union type implementation#88820
Labels
3.10 (EOL)end of life3.11only security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)

Description

@serhiy-storchaka
serhiy-storchaka
opened on Jul 16, 2021
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']

    Activity

    1. serhiy-storchaka commented on Jul 16, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 16, 2021
      MemberAuthor

      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.

    2. serhiy-storchaka
      added
      3.10 (EOL)end of life
      3.11only security fixes
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      on Jul 16, 2021

    3. Fidget-Spinner commented on Jul 16, 2021

      @Fidget-Spinner
      Fidget-Spinner
      on Jul 16, 2021
      Member

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

    4. serhiy-storchaka commented on Jul 16, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 16, 2021
      MemberAuthor

      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.

    5. serhiy-storchaka commented on Jul 17, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 17, 2021
      MemberAuthor

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

    6. serhiy-storchaka commented on Jul 17, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 17, 2021
      MemberAuthor

      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

    7. serhiy-storchaka commented on Jul 18, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 18, 2021
      MemberAuthor

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

    8. serhiy-storchaka commented on Jul 18, 2021

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 18, 2021
      MemberAuthor

      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

    9. serhiy-storchaka
      closed this as completedon Jul 18, 2021
    10. serhiy-storchaka
      closed this as completedon Jul 18, 2021
    11. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.10 (EOL)end of life3.11only security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions