bpo-34963: Create callable types in `typing.NewType` by fish2000 · Pull Request #9808 · python/cpython

fish2000 · Oct 12, 2018

This allows the generated types to furnish a real __repr__ function, while retaining the same behavior (specifically, the near-zero runtime overhead).

https://bugs.python.org/issue34963

andreip · Oct 12, 2018

I'd argue that it's best you asserted this inside a test in Lib/test/test_typing.py instead of here since this adds checking overhead every time this is used?

Sounds like a better deal, indeed. I’ll update the PR. Thanks!

andreip · Oct 12, 2018

I believe this should work similar to how namedtuple works, right?

>>> Point = namedtuple('Point', ['x', 'y']) >>> Point.__name__, Point.__qualname__, Point.__module__ ('Point', 'Point', '__main__') >>> Point.__doc__ 'Point(x, y)' >>> repr(Point) "<class '__main__.Point'>"

for __qualname__ I'm not sure, but seems to be same as __name__, so just simplified?

>>> class A: ... Point = namedtuple('Point', ['x', 'y']) ... >>> A.Point.__qualname__ 'Point'

for __module__ I see namedtuple used sys._get_frame as others in this file do as well.

I’m putting __qualname__ in as the same value as __name__ right now… I looked into the __module__ code from namedtuple you linked me to, and that looks like it somewhat violates the whole “almost zero runtime overhead” notion. I am going to implement NewType as a callable class with __slots__ (q.v. notes below) and put the __module__ code with sys._get_frame in a separate PR, so we can bench it out. Yes?

Sure, I haven't looked into sys._get_frame and how fast that would be. I'm not sure if people usually pickle a type itself, but maybe they do expect it to behave like a normal types/classes which are all pickle-able.

ilevkivskyi

I have one comment about the general idea. Could you also please add the benchmark for NewType creation with your fix vs current version?

ilevkivskyi · Oct 14, 2018

TBH, I don't really like this. One of the main motivation for NewType was to avoid any overhead of creating new class objects. So this essentially defeats the purpose of NewType. If you really think nice repr is so important, then you can create a special class once in the typing module, and then just instantiate it. For example:

>>> class NewType: ... def __init__(self, name, tp): ... self.__name__ = name ... self.__supertype__ = tp ... def __call__(self, arg): ... return arg ... def __repr__(self): ... return f"NewType <{self.__name__}:{self.__supertype__.__name__}>" ... >>> >>> CustomerId = NewType("CustomerId", int) >>> >>> CustomerId NewType <CustomerId:int> >>> CustomerId(12) 12

I’d definitely rather do something like this – presumably with __slots__ defined for the __name__ and __supertype__ values, n’est ce pas? I’ll update the PR accordingly, yes!

... also added appropriate tests

fish2000 · Oct 16, 2018

@ilevkivskyi I would be happy to add some benchmarks – can you do me a huge favor, if you could, and point me at an example of how I should set those up, within the cpython codebase? I have tests running OK but I assume there is a separate suite for performance tuning?… please do pardon my naiveté, erm. Thanks!

fish2000 · Oct 16, 2018

@andreip @gvanrossum @ilevkivskyi The new patch has been pushed up – I squashed the previous volley of commits down into just the one – and it incorporates all of your gracious feedback, as well as unit tests for all of the additions. It looks like Travis and AppVeyor are OK with things, so far. Yes!

N.B. I added a __hash__ method, with the idea that if one needs to cache NewType instances down the road, they can be used as e.g. dict keys, to allow for a simpler cache mechanism than having to use another instance of functools.lru_cache or suchlike.

andreip · Oct 16, 2018

@fish2000 I tried to see if mypy (v0.630) behaves the same way with the new updates and it doesn't. I basically did this experiment:

# newtype.py
import typing

# insert the updated NewType class here

UserId = NewType('UserId', int)  # doesn't work as typing.NewType
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

and then do mypy newtype.py with both the class defined and the one from typing.NewType.

For updated class I get an error like Invalid type "newtype.UserId" while for the typing.NewType I get ok errors like:

newtype.py:44: error: Argument 1 to "UserId" has incompatible type "str"; expected "int"
newtype.py:45: error: Argument 1 to "name_by_id" has incompatible type "int"; expected "UserId"

fish2000 · Oct 16, 2018

@andreip OK, I will look into that and check it with mypy – perhaps it needs the __module__ attribute – I had been using pyre to check and everything looks exactly the same in both cases (running pyre check with identical inputs and only the typing changes differing).

fish2000 · Oct 16, 2018

@andreip Yep – it looks like mypy needs to see that the name of the NewType expression is explicitly typing.NewType … this seems to be the key line of code. Let me verify that it’ll work if it’s in the right place with the right name.

ilevkivskyi · Oct 21, 2018

@fish2000 mypy totally doesn't care about what happens at runtime. It never runs your code.

ilevkivskyi · Oct 21, 2018

I would be happy to add some benchmarks – can you do me a huge favor, if you could, and point me at an example of how I should set those up, within the cpython codebase?

I meant just a simple comparison using timeit.timeit().

ilevkivskyi

Just two random comments. But we need to decide which approach we will take. Since now there is a "competing" PR #995, I think we can discuss this in the b.p.o. issue.

ilevkivskyi · Oct 21, 2018

+               f"{self.__supertype__.__name__}>"

+    def __hash__(self):
+        return hash((self.__name__, self.__supertype__))


Why this is needed?

I have the same question.

ilevkivskyi · Oct 21, 2018

-    return new_type
+    def __repr__(self):
+        """ NewType reprs are in the form:
+           “NewTClassName<typename:supertypename>”, e.g.:


Please don't use non-ASCII quotes. Also I am not sure this docstirng is actually needed, the code is self-explanatory.

serhiy-storchaka · Oct 28, 2018

-    new_type.__name__ = name
-    new_type.__supertype__ = tp
-    return new_type
+    def __repr__(self):


I would make the repr in the form NewType("typename", supertype):

def __repr__(self): return f"{type(self).__name__}({self.__name__!r}, {self.__supertype__})"

serhiy-storchaka · Oct 28, 2018

+    __slots__ = ('__name__', '__qualname__', '__supertype__')
+
+    def __init__(self, name, tp):
+        self.__name__ = self.__qualname__ = name


Setting __qualname__ is not needed if it is not a function.

serhiy-storchaka · Oct 28, 2018

+               f"{self.__supertype__.__name__}>"

+    def __hash__(self):
+        return hash((self.__name__, self.__supertype__))


I have the same question.

serhiy-storchaka · Oct 28, 2018

-    by static type checkers. At runtime, NewType(name, tp) returns
-    a dummy function that simply returns its argument. Usage::
+    """NewType creates simple unique types with almost zero runtime
+    overhead. `NewType(name, tp)` is considered a subtype of `tp`


Don't use backquotes in docstrings. They are not interpreted.

serhiy-storchaka · Oct 28, 2018

+        self.assertEqual(UserName.__qualname__, 'UserName')
+        self.assertIs(UserName.__supertype__, str)
+        self.assertIsInstance(UserName, NewType)
+        self.assertNotEqual(hash(UserId), hash(UserName))


We can't guarantee this.

csabella · May 25, 2020

@fish2000, please address the code reviews. Thank you!

fish2000 requested review from gvanrossum and ilevkivskyi as code owners October 12, 2018 03:08

the-knights-who-say-ni added the CLA signed label Oct 12, 2018

bedevere-bot added the awaiting review label Oct 12, 2018

gvanrossum reviewed Oct 12, 2018

View reviewed changes

Comment thread Lib/typing.py Outdated

andreip reviewed Oct 12, 2018

View reviewed changes

ilevkivskyi reviewed Oct 14, 2018

View reviewed changes

bpo-34963: Updated NewType according to latest PR notes

50ca4f1

... also added appropriate tests

Corrected typos in NewType.__repr__(…) docstring

6a75339

ilevkivskyi reviewed Oct 21, 2018

View reviewed changes

ilevkivskyi mentioned this pull request Oct 28, 2018

bpo-34963: Make the repr of the typing.NewType() result more meaningful. #9951

Closed

serhiy-storchaka reviewed Oct 28, 2018

View reviewed changes

csabella added awaiting changes and removed awaiting review labels May 25, 2020

vstinner closed this May 3, 2021

vstinner deleted the branch python:master May 3, 2021 21:29

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

fish2000 commented Oct 12, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fish2000 commented Oct 16, 2018

Uh oh!

fish2000 commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreip commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fish2000 commented Oct 16, 2018

Uh oh!

fish2000 commented Oct 16, 2018

Uh oh!

ilevkivskyi commented Oct 21, 2018

Uh oh!

ilevkivskyi commented Oct 21, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

fish2000 commented Oct 12, 2018 •

edited by bedevere-bot

Loading

ilevkivskyi Oct 14, 2018 •

edited

Loading

fish2000 commented Oct 16, 2018 •

edited

Loading

andreip commented Oct 16, 2018 •

edited

Loading