bpo-34963: Create callable types in typing.NewType by fish2000 · Pull Request #9808 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation26 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
| new_type.__supertype__ = tp |
|---|
| new_type = NewTypeClass() |
| assert new_type.__name__ == name |
| assert new_type.__supertype__ is tp |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a better deal, indeed. I’ll update the PR. Thanks!
| '__name__' : name, |
|---|
| '__repr__' : lambda self: f"<{self.__name__}:{tp.__name__}>", |
| '__call__' : staticmethod(lambda x: x) |
| }) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usedsys._get_frameas others in this file do as well.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one comment about the general idea. Could you also please add the benchmark for NewType creation with your fix vs current version?
| @@ -1429,11 +1430,16 @@ def name_by_id(user_id: UserId) -> str: |
|---|
| num = UserId(5) + 1 # type: int |
| """ |
| def new_type(x): |
| return x |
| NewTypeClass = type(name, (object,), { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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!
@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.
@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"
@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).
@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.
@fish2000 mypy totally doesn't care about what happens at runtime. It never runs your code.
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().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| def __hash__(self): |
| return hash((self.__name__, self.__supertype__)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question.
| return new_type |
|---|
| def __repr__(self): |
| """ NewType reprs are in the form: |
| “NewTClassNametypename:supertypename”, e.g.: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use non-ASCII quotes. Also I am not sure this docstirng is actually needed, the code is self-explanatory.
| new_type.__name__ = name |
|---|
| new_type.__supertype__ = tp |
| return new_type |
| def __repr__(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make the repr in the form NewType("typename", supertype):
def __repr__(self):
return f"{type(self).__name__}({self.__name__!r}, {self.__supertype__})"| __slots__ = ('__name__', '__qualname__', '__supertype__') |
|---|
| def __init__(self, name, tp): |
| self.__name__ = self.__qualname__ = name |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting __qualname__ is not needed if it is not a function.
| def __hash__(self): |
| return hash((self.__name__, self.__supertype__)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question.
| 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` |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use backquotes in docstrings. They are not interpreted.
| self.assertEqual(UserName.__qualname__, 'UserName') |
|---|
| self.assertIs(UserName.__supertype__, str) |
| self.assertIsInstance(UserName, NewType) |
| self.assertNotEqual(hash(UserId), hash(UserName)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't guarantee this.
@fish2000, please address the code reviews. Thank you!