Skip to content

bpo-39990: try resolving type hints in pydoc #19874

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

Closed
wants to merge 2 commits into from
Closed

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented May 3, 2020

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@FFY00

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@FFY00 FFY00 force-pushed the bpo-39990 branch 2 times, most recently from 59cf0cd to c7bb025 Compare May 3, 2020 01:21
@FFY00 FFY00 changed the title bpo-39990: use typing.get_type_hints resolve annotation strings bpo-39990: try to use typing.get_type_hints resolve annotation strings May 3, 2020
@FFY00
Copy link
Member Author

FFY00 commented May 3, 2020

Seems like dataclasses was not happy with this. I had to catch quite a few error types on get_type_hints to get the tests to pass.

Should dataclasses be able to get away with invalid type annotations?

@FFY00 FFY00 requested a review from ericvsmith as a code owner May 3, 2020 02:08
@ericvsmith
Copy link
Member

@FFY00: "Should dataclasses be able to get away with invalid type annotations?": Could you elaborate on what this means?

@FFY00
Copy link
Member Author

FFY00 commented May 4, 2020

It means that right now they accept string annotations, annotations that are not valid types. As per PEP 484 they are deprecated (relevant section), or am I getting anything wrong here?

Stealing from test_dataclasses.py

class TestStringAnnotations(unittest.TestCase):
    def test_classvar(self):
        # Some expressions recognized as ClassVar really aren't.  But
        #  if you're using string annotations, it's not an exact
        #  science.
        # These tests assume that both "import typing" and "from
        # typing import *" have been run in this file.
        for typestr in ('ClassVar[int]',
                        'ClassVar [int]'
                        ' ClassVar [int]',
                        'ClassVar',
                        ' ClassVar ',
                        'typing.ClassVar[int]',
                        'typing.ClassVar[str]',
                        ' typing.ClassVar[str]',
                        'typing .ClassVar[str]',
                        'typing. ClassVar[str]',
                        'typing.ClassVar [str]',
                        'typing.ClassVar [ str]',

                        # Not syntactically valid, but these will
                        #  be treated as ClassVars.
                        'typing.ClassVar.[int]',
                        'typing.ClassVar+',
                        ):
            with self.subTest(typestr=typestr):
                @dataclass
                class C:
                    x: typestr

                # x is a ClassVar, so C() takes no args.
                C()

                # And it won't appear in the class's dict because it doesn't
                # have a default.
                self.assertNotIn('x', C.__dict__)

...

@FFY00
Copy link
Member Author

FFY00 commented May 4, 2020

Anyway, removing support for this in dataclasses would break things. I am not sure this is something we want, and if it is, I am not aware of how this should be dealt with.

@ericvsmith
Copy link
Member

Ah, thanks.

The root of this problem is that I try to avoid importing typing in dataclasses.py. The original reason was for performance. Maybe that's less of an issue now after PEP 560? I haven't checked to see if it makes a difference.

But the other problem is that because dataclasses is looking for ClassVar and InitVar, it would have to either continue with the hacks it uses or call get_type_hints all over the place. That sort of defeats the point of PEP 563, I think.

So far, my preference is to keep the string annotations inspection that dataclasses does. I don't think it's been a problem in practice, but I'm open to discussion on it.

See also https://bugs.python.org/issue33453

@FFY00
Copy link
Member Author

FFY00 commented May 4, 2020

Thanks. This is something I would be happy to discuss and look at.

Is this patch okay as it is? Are there any more implications other than dataclasses?

@ericvsmith
Copy link
Member

I don't know enough about inspect to make an intelligent comment. The change to dataclasses seems reasonable.

@FFY00
Copy link
Member Author

FFY00 commented May 5, 2020

Hum, I think we should add an argument to enable this behavior. That might be the best way to deal with this.

@FFY00 FFY00 changed the title bpo-39990: try to use typing.get_type_hints resolve annotation strings bpo-39990: try resolving type hints in pydoc May 7, 2020
@FFY00 FFY00 force-pushed the bpo-39990 branch 2 times, most recently from 5306ca8 to a0311c5 Compare May 7, 2020 03:50
@AlexWaygood
Copy link
Member

@FFY00, I'm just following up on some old typing-related PRs -- is this PR something you're still interested in working on? (There's now a merge conflict.)

@FFY00
Copy link
Member Author

FFY00 commented Apr 14, 2022

@AlexWaygood thank you for the ping. IIRC I think this might already have been implemented in a separate PR, but I may be confusing something. If you want to confirm, it would be great, otherwise I will try to look at this when I have some time.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 15, 2022

@AlexWaygood thank you for the ping. IIRC I think this might already have been implemented in a separate PR, but I may be confusing something. If you want to confirm, it would be great, otherwise I will try to look at this when I have some time.

I just had a look, and I think this PR is still relevant!

C:\Users\alexw\coding\cpython>python
Running Debug|x64 interpreter...
Python 3.11.0a7+ (main, Apr 14 2022, 10:41:31) [MSC v.1931 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> def foo(bar: 'int') -> 'bool': ...
...
>>> help(foo)
Help on function foo in module __main__:

foo(bar: 'int') -> 'bool'

>>> def baz(bar: int) -> bool: ...
...
>>> help(baz)
Help on function baz in module __main__:

baz(bar: int) -> bool

If I understand correctly, this PR would make the output of help(foo) more like the output of help(baz) (which would be great, in my opinion!).

If you are still interested in working on it, though, looks like some new tests would be needed.

@JelleZijlstra
Copy link
Member

Calling get_type_hints() is very tricky because you need to give the right namespace. It may also make the output harder to read, because type aliases get expanded.

A much simpler solution to make pydoc's output nicer could be simply to not show the quotes around string type annotations.

@AlexWaygood
Copy link
Member

A much simpler solution to make pydoc's output nicer could be simply to not show the quotes around string type annotations.

It would be good to check that the annotation is parseable first, though; doing this all the time could cause confusion if the annotation isn't a valid type. E.g. somebody who doesn't use typing, who's using annotations for documentation or whatever, could plausibly do something like this:

def fly(what: 'a bird, plane, or superman', where: 'this needs to be a country') -> None: ...

Currently this happens, which I think is pretty good:

>>> help(fly)
Help on function fly in module __main__:

fly(what: 'a bird, plane, or superman', where: 'this needs to be a country') -> None

@FFY00
Copy link
Member Author

FFY00 commented Nov 17, 2024

Closing this PR as I am not really interested in following up. Will leave it for someone else.

@FFY00 FFY00 closed this Nov 17, 2024
@JelleZijlstra
Copy link
Member

Note we already made this a bit nicer in 3.14 (#124669).

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

Successfully merging this pull request may close these issues.

7 participants