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

typing: get_type_hints on stringified lone ClassVar raises TypeError #90711

Closed
GBeauregard mannequin opened this issue Jan 27, 2022 · 11 comments
Closed

typing: get_type_hints on stringified lone ClassVar raises TypeError #90711

GBeauregard mannequin opened this issue Jan 27, 2022 · 11 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@GBeauregard
Copy link
Mannequin

GBeauregard mannequin commented Jan 27, 2022

BPO 46553
Nosy @gvanrossum, @ericvsmith, @JelleZijlstra, @sobolevn, @Fidget-Spinner, @AlexWaygood, @GBeauregard
PRs
  • bpo-46553: allow bare typing.ClassVar annotations #30983
  • 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 2022-03-17.14:34:07.793>
    created_at = <Date 2022-01-27.20:50:13.975>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'typing: get_type_hints on stringified lone ClassVar raises TypeError'
    updated_at = <Date 2022-03-17.14:34:07.793>
    user = 'https://github.com/GBeauregard'

    bugs.python.org fields:

    activity = <Date 2022-03-17.14:34:07.793>
    actor = 'JelleZijlstra'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-03-17.14:34:07.793>
    closer = 'JelleZijlstra'
    components = ['Library (Lib)']
    creation = <Date 2022-01-27.20:50:13.975>
    creator = 'GBeauregard'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46553
    keywords = ['patch']
    message_count = 11.0
    messages = ['411923', '411924', '411927', '411928', '411934', '411943', '411955', '411959', '411972', '412015', '415413']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'eric.smith', 'JelleZijlstra', 'sobolevn', 'kj', 'AlexWaygood', 'GBeauregard']
    pr_nums = ['30983']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46553'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @GBeauregard
    Copy link
    Mannequin Author

    GBeauregard mannequin commented Jan 27, 2022

    class C:
        a: "ClassVar"
    
    get_type_hints(C, globals())  # TypeError: Plain typing.ClassVar is not valid as type argument
    

    A stringified lone ClassVar raises at runtime, but this pattern is tested for in dataclasses unit tests and used in the wild. The PEP is not clear that it should or should not be used with arguments, and it works fine when not stringified.

    The fix for this is trivial and I can submit a patch if there's agreement.

    @GBeauregard GBeauregard mannequin added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir labels Jan 27, 2022
    @GBeauregard
    Copy link
    Mannequin Author

    GBeauregard mannequin commented Jan 27, 2022

    I think this is needed for moving dataclasses to using typing.py introspection tools to be viable.

    @GBeauregard GBeauregard mannequin added type-bug An unexpected behavior, bug, or error labels Jan 27, 2022
    @ericvsmith
    Copy link
    Member

    dataclasses is no doubt too lenient. But it's just trying to accept valid strings that look like ClassVar. Way back when this was initially implemented, we decided that calling get_type_hints would be too expensive for every dataclass, and would also neccesitate importing typing, which we didn't want to require.

    I think someone needs to do an analysis of how expensive it would be for dataclasses to import typing and to call get_type_hints. Perhaps we'd make a different decision today.

    @GBeauregard
    Copy link
    Mannequin Author

    GBeauregard mannequin commented Jan 27, 2022

    I'm drafting an implementation for the purpose of investigating performance right now; I will share when ready.

    @gvanrossum
    Copy link
    Member

    It seems acceptable to mypy.

    I'm not sure I like the agenda of "moving dataclasses to using typing.py introspection tools".

    @GBeauregard
    Copy link
    Mannequin Author

    GBeauregard mannequin commented Jan 28, 2022

    It's acceptable to mypy, and pyright added support a few months ago when I made an issue and Eric Traut discovered this pattern in the wild too.

    Some of the other type checkers (pyre) still error I believe. My feeling is that since this has apparently become used in practice we should fix the runtime error for when it's stringified, but we don't necessarily need to prescribe that it's a legal type annotation for type checkers (?). If I was prescribing how to use ClassVar in a vacuum, I don't see any good typing reason we should prohibit this.

    re: moving dataclasses: I'm not sure either! I'm trying to look into the issues it brings up in practice to weigh against the difficulties of maintaining the existing bespoke introspection implementation and the problems it has (dealing with stringified annotations, supporting renaming the Annotated symbol).

    I made an implementation that fully moves dataclasses over to using get_type_hints (and got all the tests to pass), but my feeling right now is that a few issues are too serious for it to be viable:

    1. the namespaces are a mess, and issues like get_type_hints() always raises on forward references typing#508 don't have a solution at the moment
    2. get_type_hints paints a wide brush on the entire class when it raises for errors, so it's not very viable to deal with non-typing non-Annotated[] annotations as a one-off

    There's also nontechnical issues, like the potential politics of making dataclasses always import typing. I'll be updating bpo-46511 later with thoughts.

    @gvanrossum
    Copy link
    Member

    Please just don't go there. I beg you. It's not worth it.

    @GBeauregard
    Copy link
    Mannequin Author

    GBeauregard mannequin commented Jan 28, 2022

    I think I miscommunicated my intent with sentence placement. I already posted the thoughts I referred to; they're just my concluding opinion on the technical merit of using get_type_hints in dataclasses to solve the Annotated problem: https://bugs.python.org/msg411945

    In any case, I apologize for the faux pas and I'll be careful in the future.

    I dug up the issue where pyright was changed (to allow bare ClassVar) that has an argument for not allowing it to be bare: microsoft/pyright#2377

    @gvanrossum
    Copy link
    Member

    I
    To be clear: I am okay with this patch, just not with making dataclasses
    import typing.

    --Guido (mobile)

    @gvanrossum
    Copy link
    Member

    New changeset 5445e17 by Gregory Beauregard in branch 'main':
    bpo-46553: allow bare typing.ClassVar annotations (bpo-30983)
    5445e17

    @ericvsmith
    Copy link
    Member

    Is there any reason to keep this issue open? The PR was merged.

    @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.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants