Skip to content
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

PEP 604 NewType #88519

Closed
wyfo mannequin opened this issue Jun 8, 2021 · 44 comments
Closed

PEP 604 NewType #88519

wyfo mannequin opened this issue Jun 8, 2021 · 44 comments
Labels
3.10 only security fixes 3.11 only security fixes

Comments

@wyfo
Copy link
Mannequin

wyfo mannequin commented Jun 8, 2021

BPO 44353
Nosy @gvanrossum, @ambv, @serhiy-storchaka, @ilevkivskyi, @JelleZijlstra, @pablogsal, @miss-islington, @uriyyo, @wyfo, @domdfcoding, @Fidget-Spinner, @dlax, @AlexWaygood
PRs
  • bpo-44353: Refactor typing.NewType into callable class #27250
  • [3.10] bpo-44353: Refactor typing.NewType into callable class (GH-27250) #27258
  • bpo-44353: Add test to cover __or__ of two typing.NewType #27259
  • [3.10] bpo-44353: Add test to cover __or__ of two NewType (GH-27259) #27261
  • bpo-44353: Implement typing.NewType __call__ method in C #27262
  • bpo-44353: Add tests to cover typing.NewType pickling #27302
  • bpo-44353: Fix memory leak introduced by #27262 #27305
  • bpo-44353: Expand NewType tests for complex __qualname__. #27311
  • bpo-34963: Make the repr of the typing.NewType() result more meaningful. #9951
  • bpo-44353: Document that typing.NewType is now a class #27319
  • [3.10] bpo-44353: Document that typing.NewType is now a class (GH-27319) #27321
  • [3.10] bpo-44353: Expand NewType tests for complex __qualname__ (GH-27311) #27326
  • [3.10] bpo-44353: Improve tests covering typing.NewType pickling (GH-27302) #27328
  • bpo-44353: Correct docstring for NewType #29785
  • [3.10] bpo-44353: Correct docstring for NewType (GH-29785) #29796
  • Superseder
  • bpo-44642: Union of a type and the typing module function
  • 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-24.15:29:29.597>
    created_at = <Date 2021-06-08.20:21:48.412>
    labels = ['3.10', '3.11']
    title = 'PEP 604 NewType'
    updated_at = <Date 2021-11-26.14:58:16.927>
    user = 'https://github.com/wyfo'

    bugs.python.org fields:

    activity = <Date 2021-11-26.14:58:16.927>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-24.15:29:29.597>
    closer = 'kj'
    components = []
    creation = <Date 2021-06-08.20:21:48.412>
    creator = 'joperez'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44353
    keywords = ['patch']
    message_count = 44.0
    messages = ['395359', '395512', '395519', '395538', '397299', '397300', '397825', '397826', '397834', '397835', '397838', '397840', '397842', '397847', '397848', '397849', '397869', '397876', '397877', '397878', '397879', '397880', '397881', '397886', '398005', '398006', '398048', '398057', '398080', '398120', '398123', '398129', '398136', '398138', '398139', '398147', '398148', '398149', '398150', '398151', '398152', '398164', '407026', '407055']
    nosy_count = 13.0
    nosy_names = ['gvanrossum', 'lukasz.langa', 'serhiy.storchaka', 'levkivskyi', 'JelleZijlstra', 'pablogsal', 'miss-islington', 'uriyyo', 'joperez', 'domdfcoding', 'kj', 'dlax', 'AlexWaygood']
    pr_nums = ['27250', '27258', '27259', '27261', '27262', '27302', '27305', '27311', '9951', '27319', '27321', '27326', '27328', '29785', '29796']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = '44642'
    type = None
    url = 'https://bugs.python.org/issue44353'
    versions = ['Python 3.10', 'Python 3.11']

    @wyfo
    Copy link
    Mannequin Author

    wyfo mannequin commented Jun 8, 2021

    typing.NewType doesn't support PEP-604.

    >>> import typing
    >>> A = typing.NewType("A", int)
    >>> B = typing.NewType("B", str)
    >>> A | B
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: unsupported operand type(s) for |: 'function' and 'function'
    

    @wyfo wyfo mannequin added 3.10 only security fixes labels Jun 8, 2021
    @domdfcoding
    Copy link
    Mannequin

    domdfcoding mannequin commented Jun 10, 2021

    It is possible to implement NewType to return a class rather than a function as it does currently. However, the class version uses substantially more memory (1072 bytes vs 144 bytes with sys.getsizeof) and NewType is supposed to have almost no runtime overhead.

    @uriyyo
    Copy link
    Member

    uriyyo commented Jun 10, 2021

    What about to return callable object instead of function from a typing.NewType?

    It will look something like this:

    class _NewType:
        __slots__ = ('__name__', '__supertype__')
    
        def __init__(self, name, supertype):
            self.__name__ = name
            self.__supertype__ = supertype
    
        def __call__(self, val):
            return val
    
        def __or__(self, other):
            return Union[self, other]
    
        def __ror__(self, other):
            return Union[other, self]
    
    
    
    def NewType(name, tp):
        """NewType creates simple unique types with almost zero
        runtime overhead. NewType(name, tp) is considered a subtype of tp
        by static type checkers. At runtime, NewType(name, tp) returns
        a dummy callable object that simply returns its argument. Usage::
    
            UserId = NewType('UserId', int)
    
            def name_by_id(user_id: UserId) -> str:
                ...
    
            UserId('user')          # Fails type check
    
            name_by_id(42)          # Fails type check
            name_by_id(UserId(42))  # OK
    
            num = UserId(5) + 1     # type: int
        """
        return _NewType(name, tp)
    

    With such implementation __or__ will be available for a NewType and actually because of slots size of object will be 48 bytes (with sys.getsizeof) which is less than current 144 bytes (if memory is important).

    @Fidget-Spinner Fidget-Spinner added 3.11 only security fixes labels Jun 10, 2021
    @JelleZijlstra
    Copy link
    Member

    python/typing#746 has some previous discussion of implementing NewType as a class (motivated by __repr__), including benchmarks.

    @uriyyo
    Copy link
    Member

    uriyyo commented Jul 12, 2021

    Jelle I can try to implement it in C to see if it will give better performance.

    @Fidget-Spinner
    Copy link
    Member

    @yurii, I would like to caution against a C accelerator for the typing module. My reasoning follows:

    1. The maintenance burden is higher. typing is already somewhat complex in Python (lots of MRO/metaclass wizardry). A C module would require knowledge of the C API.

    2. Following from 1., I fear it may raise the barrier for contributions. This is purely anecdotal but I think there are more contributors who know Python than Python + C API.

    3. C API code is much more error-prone than Python code.

    4. It's very hard to find available reviewers for typing-related C changes.

    5. Backports become harder due to point 3. and 4. Also C code needs much more scrutiny. If we cause some obscure bug in Python, it raises an exception; in C it potentially segfaults and kills the interpreter.

    6. Third-party monkey-patching becomes harder.

    Unfortunately, I can't offer a good solution to this issue at the moment either.

    @uriyyo
    Copy link
    Member

    uriyyo commented Jul 19, 2021

    @ken thanks for pointing this out.

    The only solution which I see is to convert NewType into callable class but it will lead to performance degradation.

    @ken what do you think about that?

    @gvanrossum
    Copy link
    Member

    Do we have to solve it? Using from __future__ import annotations it should work, at least when used as an annotation. Or is that too inconvenient or inconsistent?

    @JelleZijlstra
    Copy link
    Member

    Using from __future__ import annotations it should work

    Not in type aliases or generic bases. (And if Larry's co_annotations PEP is accepted, it would break in normal annotations too.) I'd prefer to get rid of this sort of subtle difference where the obvious way to write a type only works in some contexts, though I realize that's not possible in all cases.

    That means I think we should move forward with making NewTypes into instances with __call__ instead of functions, though according to my benchmarks in the typing issue that will incur a small performance penalty.

    @uriyyo
    Copy link
    Member

    uriyyo commented Jul 19, 2021

    I opened PR with refactored typing.NewType into callable class.

    Also I have added __module__ attr to typing.NewType so it will become pickable.

    @gvanrossum
    Copy link
    Member

    Jelle, can you post more details about your benchmark?

    @JelleZijlstra
    Copy link
    Member

    This is what I got last year (copied from the typing issues):

    In [83]: from typing import NewType

    In [84]: nt = NewType("nt", int)

    In [85]: class NewTypeClass:
    ...: def __init__(self, name, supertype):
    ...: self.name = name
    ...: self.supertype = supertype
    ...: def __call__(self, obj):
    ...: return obj
    ...: def __repr__(self):
    ...: return f"<NewType: {self.name}>"
    ...:

    In [86]: ntc = NewTypeClass("ntc", int)

    In [87]: ntc
    Out[87]: <NewType: ntc>

    In [88]: %timeit nt(3)
    211 ns ± 2.27 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

    In [89]: %timeit ntc(3)
    253 ns ± 5.35 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

    Not sure what version of Python I used at the time. I just repeated it on 3.9.4 MacOS and got much faster time but a similar ~20% slowdown:

    In [6]: %timeit nt(3)
    112 ns ± 5.11 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

    In [7]: %timeit ntc(3)
    136 ns ± 2.36 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

    @gvanrossum
    Copy link
    Member

    Hm, 20% isn't so bad, but one of the arguments for NewType was that it "disappears" at runtime -- which the current version does, but the class-based version doesn't quite.

    Right now I don't have the cycles to think about this deeply. Maybe a few weeks after I'm back from vacation (~August 8) I will have more time.

    @Fidget-Spinner
    Copy link
    Member

    This issue is now out of date on. After Serhiy's refactoring, any function types can be unioned.

    >>> NewType('x', int)
    >>> int | NewType('x', int)
    int | typing.NewType.<locals>.new_type

    The only problem now is that the repr is weird, but Serhiy also has a fix for that in bpo-34963 (PR 9951) without converting to a class.

    Thus, I am closing the issue. Please go to bpo-44642 for a general discussion on whether union should support arbitrary functions at all, or bpo-34963 for the repr problem. Thanks all!

    @JelleZijlstra
    Copy link
    Member

    Does that work if you try to union two NewTypes?

    @Fidget-Spinner
    Copy link
    Member

    Does that work if you try to union two NewTypes?

    Oh woops I was too eager in closing this issue -- it doesn't work. Sorry, please disregard my previous message. I'm reopening this. Thanks for the reminder Jelle.

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2021

    New changeset 965dd76 by Yurii Karabas in branch 'main':
    bpo-44353: Refactor typing.NewType into callable class (GH-27250)
    965dd76

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2021

    New changeset 4868b94 by Yurii Karabas in branch 'main':
    bpo-44353: Add test to cover __or__ of two NewType (bpo-27259)
    4868b94

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2021

    Hm, 20% isn't so bad, but one of the arguments for NewType was that it "disappears" at runtime -- which the current version does, but the class-based version doesn't quite.

    NewType was described originally in python/typing#189 and the resulting PEP text landed in https://github.com/python/typing/pull/226/files.

    I find the 20% performance impact non-ideal but ultimately worth it because:

    • it solves the issue described in this bug;
    • it allows us to implement issubclass/isinstance in the future if we so choose;
    • it also potentially allows us to chain NewType.

    Since we can't fully rely on from __future__ import annotations to side-step the performance cost, we don't really have a different option than to change it. The alternative is to leave it as is which makes it a non-composable pseudo-type unlike the others.

    Currently adoption of NewType is relatively low, in part due to the feature's obscurity and partially because of its limits. Slower instantiation performance will at worst keep the adoption low, but it can potentially bring more users to NewType since it becomes less hacky.

    I'm +1 to converting to a class as done by the PR.

    Right now I don't have the cycles to think about this deeply. Maybe a few weeks after I'm back from vacation (~August 8) I will have more time.

    Sorry, I overeagerly already merged the change to main. Let's hold off with the 3.10 backport until you had time to think about it. If we decide against the change after all, reverting on main should have no negative impact since we're pre-alpha 1.

    @gvanrossum
    Copy link
    Member

    OTOH, for 3.10 the clock is ticking. Let's go for it. (If it's deemed too slow, we could eventually add an accelerator just for this.)

    @JelleZijlstra
    Copy link
    Member

    I found that replacing __call__ on the NewType class with an identity function written in C makes things faster instead:

    In [54]: %timeit ntc2(1)
    79 ns ± 0.37 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

    In [55]: %timeit ntc(1)
    126 ns ± 0.315 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

    In [56]: %timeit nt(1)
    103 ns ± 4.23 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

    Here ntc2 has __call__ implemented in C, ntc is the previous class-based version and nt is the current function-based version.

    So perhaps we could just stick a private ._idfunc in some C-implemented module (functools? types?) and use it as the call for our NewType class.

    @uriyyo
    Copy link
    Member

    uriyyo commented Jul 20, 2021

    Jelle thanks for pointing this out, I will implement helper function in C and use it as a __call__ method for NewType

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2021

    New changeset c2f33df by Miss Islington (bot) in branch '3.10':
    bpo-44353: Refactor typing.NewType into callable class (GH-27250) (bpo-27258)
    c2f33df

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2021

    New changeset 9ae5ba7 by Miss Islington (bot) in branch '3.10':
    bpo-44353: Add test to cover __or__ of two NewType (GH-27259) (bpo-27261)
    9ae5ba7

    @ambv
    Copy link
    Contributor

    ambv commented Jul 22, 2021

    New changeset 96c4cbd by Yurii Karabas in branch 'main':
    bpo-44353: Implement typing.NewType __call__ method in C (bpo-27262)
    96c4cbd

    @ambv
    Copy link
    Contributor

    ambv commented Jul 22, 2021

    Note: we won't be backporting _typing to Python 3.10 as it is much too late for a new module at this point in the life cycle. Consequently, 3.10 will be a (temporary) performance regression in terms of typing.NewType.

    Thanks everyone, most of all Josepth for reporting and Yurii for implementing the required changes.

    @ambv ambv closed this as completed Jul 22, 2021
    @ambv ambv closed this as completed Jul 22, 2021
    @pablogsal
    Copy link
    Member

    PR27262 introduced reference leaks and currently all refleak buildbots are red:

    Example:

    https://buildbot.python.org/all/#/builders/205/builds/102/steps/5/logs/stdio

    Ran 367 tests in 0.198s
    OK (skipped=1)
    .
    test_typing leaked [220, 220, 220] references, sum=660
    test_typing leaked [69, 68, 68] memory blocks, sum=205
    1 test failed again:
    test_typing
    == Tests result: FAILURE then FAILURE ==
    Per our buildbot policy, if a fix is not performed in 24 h we will need to revert

    @pablogsal pablogsal reopened this Jul 23, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Jul 23, 2021

    New changeset 8f42106 by Yurii Karabas in branch 'main':
    bpo-44353: Fix memory leak introduced by #71449 (GH-27305)
    8f42106

    @ambv ambv closed this as completed Jul 23, 2021
    @serhiy-storchaka
    Copy link
    Member

    PR 27311 copies tests from PR 9951, adds support of nested NewTypes (with __qualname__ containing multiple components) an makes them pickleable by name as functions and classes.

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset e89ef0a by Serhiy Storchaka in branch 'main':
    bpo-44353: Expand NewType tests for complex __qualname__. (bpo-27311)
    e89ef0a

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset 7aac3f6 by Ken Jin in branch 'main':
    bpo-44353: Document that typing.NewType is now a class (bpo-27319)
    7aac3f6

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset d15949a by Miss Islington (bot) in branch '3.10':
    bpo-44353: Document that typing.NewType is now a class (GH-27319) (GH-27321)
    d15949a

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset a22b05d by Yurii Karabas in branch 'main':
    bpo-44353: Improve tests covering typing.NewType pickling (GH-27302)
    a22b05d

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset 05f5d8e by Łukasz Langa in branch '3.10':
    [3.10] bpo-44353: Expand NewType tests for complex __qualname__ (GH-27311) (GH-27326)
    05f5d8e

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    New changeset e8c0174 by Miss Islington (bot) in branch '3.10':
    bpo-44353: Improve tests covering typing.NewType pickling (GH-27302) (GH-27328)
    e8c0174

    @pablogsal
    Copy link
    Member

    The buildbots are still not green. Unfortunately we will need to revert all PRs if this is not fixed soon as this is already making other issues

    @pablogsal pablogsal reopened this Jul 24, 2021
    @Fidget-Spinner
    Copy link
    Member

    @pablo, I don't think this change is causing the buildbots to fail. The test failure on all the currently failing buildbots is:

    ======================================================================
    ERROR: test_absolute_circular_submodule (test.test_import.CircularImportTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/test/test_import/__init__.py", line 1355, in test_absolute_circular_submodule
        import test.test_import.data.circular_imports.subpkg2.parent
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ModuleNotFoundError: No module named 'test.test_import.data.circular_imports.subpkg2'

    Which seems to be #71486.

    @pablogsal
    Copy link
    Member

    Pick any of the failing refleak buildbots. For example:

    https://buildbot.python.org/all/#/builders/280/builds/99

    You will see:

    ----------------------------------------------------------------------
    Ran 106 tests in 0.074s
    OK
    ......
    test_types leaked [2, 2, 2] references, sum=6
    test_types leaked [1, 1, 1] memory blocks, sum=3
    1 test failed again:
    test_types

    @ambv
    Copy link
    Contributor

    ambv commented Jul 24, 2021

    types is not typing. This is a different failure from what we've seen before. I'll fix that today.

    @pablogsal
    Copy link
    Member

    Bisecting points to:

    fe13f0b is the first bad commit
    commit fe13f0b
    Author: Yurii Karabas <1998uriyyo@gmail.com>
    Date: Fri Jul 23 12:47:00 2021 +0300

    bpo-44676: Add ability to serialize types.Union (GH-27244)
    

    Lib/test/test_types.py | 35 +++++++++++++++
    Lib/typing.py | 4 +-
    .../2021-07-19-19-53-46.bpo-44676.WgIMvh.rst | 2 +
    Objects/unionobject.c | 51 ++++++++++++++++++++++
    4 files changed, 90 insertions(+), 2 deletions(-)
    create mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-07-19-19-53-46.bpo-44676.WgIMvh.rst
    bisect run success

    @pablogsal
    Copy link
    Member

    I am closing this one again, apologies for the confusion.

    @Fidget-Spinner
    Copy link
    Member

    Thanks a bunch Yurii, Serhiy, Jelle, Łukasz and Pablo for working on this! I'm re-closing this issue. *Fingers-crossed* we won't have to open this again ;-).

    @gvanrossum
    Copy link
    Member

    New changeset 93c65df by Alex Waygood in branch 'main':
    bpo-44353: Correct docstring for NewType (bpo-29785)
    93c65df

    @miss-islington
    Copy link
    Contributor

    New changeset 3f024e2 by Miss Islington (bot) in branch '3.10':
    bpo-44353: Correct docstring for NewType (GH-29785)
    3f024e2

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes 3.11 only security fixes
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants