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

repr of NewType #746

Open
hoodmane opened this issue Aug 14, 2020 · 10 comments
Open

repr of NewType #746

hoodmane opened this issue Aug 14, 2020 · 10 comments

Comments

@hoodmane
Copy link

@hoodmane hoodmane commented Aug 14, 2020

Currently NewType has a rather unsatisfactory repr:

from typing import NewType
A = NewType("A", int)
print(repr(A))

outputs:

<function NewType.<locals>.new_type at MEMORYADDRESS>

This (1) doesn't refer to the name of the NewType, (2) doesn't refer to the wrapped type, (3) doesn't refer to the module the NewType was defined in, and (4) is confusing.
If a function is defined with the NewType in its signature, this repr appears in inspect.getsignature(signature_of_function).
For example:

from typing import NewType
A = NewType("A", int)
def test(a : A):
    pass
import inspect
sig = inspect.signature(test)
annotation = list(sig.parameters.values())[0].annotation
print(inspect.formatannotation(annotation))

prints

<function NewType.<locals>.new_type at MEMORYADDRESS>

This shows up in Jedi output:

from typing import NewType
A = NewType("A", int)
def test(a : A):
    pass
import jedi
test_jedi = jedi.Interpreter("test", [globals()]).complete()[0]
print(test_jedi.docstring())

prints

test(a: <function NewType.<locals>.new_type at MEMORYADDRESS>)

Presumably you could fix this by turning NewType into a class:

class NewType:
    def __init__(self, name, tp):
        self.__name__ = name
        self.__supertype__ = tp

    def __call__(self, x):
        return x

    def __repr__(self):
        return self.__name__

This change would also make it less opaque how to test whether an object is a NewType (I think currently the best way to make this check is to say hasattr(x, "__qualname__") and x.__qualname__ == 'NewType.<locals>.new_type' which is not exactly transparent.

I imagine this change could break typecheckers which are likely to care about the implementation details of NewType, but the fix for the type checkers shouldn't be too complicated.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Aug 14, 2020

Since NewType is 99.9% meant for the benefit of static type checkers, I'm not sure that I care much about how it is rendered at runtime. If you have a specific issue with this related to Jedi maybe it's best to solve this in Jedi.

@hoodmane
Copy link
Author

@hoodmane hoodmane commented Aug 14, 2020

In principle, NewType should be beneficial for both the static type checker and for people using the API. If say Union[A,B,C] of several types comes up a lot, replacing Union[A,B,C] with a semantic name improves the code clarity a lot (particularly if the types A, B, and C have long names). If you say MySemanticName = Union[A,B,C] then you can use MySemanticName in the code but when Jedi looks up the documentation for the function it will show Union[A,B,C] instead of MySemanticName. I had hoped that with NewType I could get Jedi to show MySemanticName but instead Jedi shows <function NewType.<locals>.new_type at 0xblahblahblah>.

In practice, I think these fiddly ergonomics details can make a huge difference to how easy it is to use an API.

For my personal needs a satisfactory solution is to monkey patch NewType conditioned on TYPE_CHECKING:

import typing
if not TYPE_CHECKING:
    class NewType:
        def __init__(self, name, tp):
            self.__name__ = name
            self.__supertype__ = tp

        def __call__(self, x):
            return x

        def __repr__(self):
            return self.__name__
    typing.NewType = NewType
@hoodmane
Copy link
Author

@hoodmane hoodmane commented Aug 14, 2020

What about the possibility of updating formatannotation to know about NewType?

def formatannotation(annotation, base_module=None):
    # Add the following two lines? 
    # If the annotation is a NewType, return the name of the NewType
    if hasattr(annotation, "__qualname__") and annotation.__qualname__ == 'NewType.<locals>.new_type':
        return annotation.__name__
    if getattr(annotation, '__module__', None) == 'typing':
        return repr(annotation).replace('typing.', '') # <== the path taken in NewType case.
    if isinstance(annotation, type):
        if annotation.__module__ in ('builtins', base_module):
            return annotation.__qualname__
        return annotation.__module__+'.'+annotation.__qualname__
    return repr(annotation)
@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Aug 14, 2020

The more I read about your use case the more I think this should be solved in your tooling, not in typing.

Regarding whether it's better to show MySemanticName or Union[A, B, C] (or A | B | C with PEP 604 :-), I'm not sure that I would prefer seeing MySemanticName in the docs, unless that is a very well-known name. Otherwise it just sends me on a wild goose chase looking up the definition of MySemanticName.

But as I am not a Jedi user, I'll take your work for it -- I just don't think we should fix this in typing.

@hoodmane
Copy link
Author

@hoodmane hoodmane commented Aug 14, 2020

I'm not sure that I would prefer seeing MySemanticName in the docs, unless that is a very well-known name. Otherwise it just sends me on a wild goose chase looking up the definition of MySemanticName.

Absolutely a fair point.

What about the normal use case for NewType? If I say UUID = NewType("UUID", str) for example and then I define a function with signature do_something(uuid : UUID), to inspect.formatannotation, the signature of do_something looks like garbage -- if it either showed the annotation as UUID or str that would be acceptable but all NewTypes show more or less identical junk, and it's impossible to tell from this junk whether the function was expecting an int or as str.

Isn't the point of formatannotation to allow access to human readable type annotations at runtime? Currently it doesn't do that when the type annotation contains a NewType.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Aug 15, 2020

It seems the inspect module was not really updated to deal with PEP 484 -- and because PEP 484 does not really care about runtime use of annotations very much.

By the time we could fix this we wouldn't need the fix any more, since in 3.10 we'll have from __future__ import annotations on by default (per PEP 563).

I worry that your proposed fix (turning NewType into a class) would slow down common usage -- the docstring promises "almost zero runtime overhead." I fear that an instance with a __call__ method comes out slower than the new function created by the current implementation. OTOH maybe instances are cheaper than function objects -- this deserves some benchmarking. Maybe if this is decided in favor of the class we can fix it after all.

@JelleZijlstra
Copy link
Contributor

@JelleZijlstra JelleZijlstra commented Aug 16, 2020

It's about 20% slower:

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)

I do agree that it would be useful to get a nicer repr for NewTypes; at my company we use NewTypes heavily in annotations and the current repr() makes it harder to read function signatures in IPython.

I wonder if it would be faster if we implemented NewTypeClass in C.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Aug 16, 2020

Maybe we could have a NewType function that special-cases int and str (which I presume are the bulk of NewType calls):

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}>"

class IntNewType(NewTypeClass):
    __call__ = int

class StrNewType(NewTypeClass):
    __call__ = str

def NewType(name, supertype):
    if supertype is int:
        return IntNewType(name, supertype)
    if supertype is str:
        return StrNewType(name, supertype)
    return NewTypeClass(name, supertype)
@devdoomari3
Copy link

@devdoomari3 devdoomari3 commented Sep 26, 2020

@hoodmane

you can solve 'is this type a NewType'? like this:

https://github.com/pycodegen/python-type-extractor/blob/develop/py_type_extractor/type_extractor/middlewares/newtype_found.py#L16

( the repo isn't ready-for-public, and needs a lot more work on refactoring, renaming, etc,
-- but it's tested for python 3.6-3.8 )

note that I couldn't find a way to get modules from this (relevant issue: #757 )

ps: are you building some kind of 'runtime-type-extraction' for codegens/etc?
I'm also hoping python will get standardized codegen to sort out all kinds of "mypy-plugin" vs "pyright-plugin" vs "pycharm-plugin" -- something like dart's codegens: https://github.com/dart-lang/source_gen

@hoodmane
Copy link
Author

@hoodmane hoodmane commented Sep 27, 2020

@devdoomari3 Thanks for the suggestion, I also made a "is this a NewType" test but it is worse than the one you linked. I think from __future__ import annotations is sufficient for my needs though, I was just trying to get Jedi type annotations to work correctly when NewType occurs in the type signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.