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

get_type_hints does not provide localns for classes #87070

Closed
pbryan mannequin opened this issue Jan 12, 2021 · 12 comments
Closed

get_type_hints does not provide localns for classes #87070

pbryan mannequin opened this issue Jan 12, 2021 · 12 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@pbryan
Copy link
Mannequin

pbryan mannequin commented Jan 12, 2021

BPO 42904
Nosy @gvanrossum, @larryhastings, @pbryan, @Fidget-Spinner
PRs
  • bpo-42904: Fix get_type_hints for class local namespaces #24201
  • bpo-42904: Change search order of typing.get_type_hints eval #25632
  • 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 2021-04-13.09:48:40.365>
    created_at = <Date 2021-01-12.05:00:15.225>
    labels = ['type-bug', 'library', '3.10']
    title = 'get_type_hints does not provide localns for classes'
    updated_at = <Date 2021-04-29.23:48:45.413>
    user = 'https://github.com/pbryan'

    bugs.python.org fields:

    activity = <Date 2021-04-29.23:48:45.413>
    actor = 'kj'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-13.09:48:40.365>
    closer = 'kj'
    components = ['Library (Lib)']
    creation = <Date 2021-01-12.05:00:15.225>
    creator = 'pbryan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42904
    keywords = ['patch']
    message_count = 12.0
    messages = ['384885', '384887', '384888', '384925', '390871', '390872', '390945', '391955', '392317', '392321', '392351', '392364']
    nosy_count = 4.0
    nosy_names = ['gvanrossum', 'larry', 'pbryan', 'kj']
    pr_nums = ['24201', '25632']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue42904'
    versions = ['Python 3.10']

    @pbryan
    Copy link
    Mannequin Author

    pbryan mannequin commented Jan 12, 2021

    According to PEP-563:

    The get_type_hints() function automatically resolves the correct value of globalns for functions and classes. It also automatically provides the correct localns for classes.

    This statement about providing correct localns for classes does not appear to be true.

    Guido suggested this should be treated as a bug.

    @pbryan pbryan mannequin added 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jan 12, 2021
    @gvanrossum
    Copy link
    Member

    Fidget-Spinner, are you interested in taking this?

    @gvanrossum
    Copy link
    Member

    It's apparently a bug in all versions that support from __future__ import annotations (and only when that is used). Though perhaps we should only fix in in 3.10.

    @Fidget-Spinner
    Copy link
    Member

    Fidget-Spinner, are you interested in taking this?

    Sure thing! Please give me some time to look at it - I don't really use the runtime type validation stuff from typing (I usually defer that to 3rd party libraries), so I need to familiarize myself first.

    @gvanrossum
    Copy link
    Member

    New changeset 852150d by Ken Jin in branch 'master':
    bpo-42904: Fix get_type_hints for class local namespaces (GH-24201)
    852150d

    @gvanrossum
    Copy link
    Member

    Hi Ken Jin, you can close this issue now with your new permissions, right?

    @Fidget-Spinner
    Copy link
    Member

    Yup I can! BTW, I agree with keeping the change in 3.10 only. I suspect it _may_ be backwards-incompatible in small edge cases (re: the long comment on the PR about suspicions with local and global scope).

    @gvanrossum
    Copy link
    Member

    New changeset 1b1f985 by Ken Jin in branch 'master':
    bpo-42904: Change search order of typing.get_type_hints eval (bpo-25632)
    1b1f985

    @larryhastings
    Copy link
    Contributor

    kj: I just added support for default locals to inspect.get_annotation(), and I basically copied-and-pasted your dict(vars(base)) approach.

    Is this "surprising, but required" behavior due specifically to this being a backwards-incompatible change? inspect.get_annotations() will be a new function in Python 3.10, which means I have no existing users to worry about. Does that mean I don't have the problem you're solving by reversing the order of namespace lookup, and I can just pass globals and locals in like normal?

    @Fidget-Spinner
    Copy link
    Member

    @larry

    Is this "surprising, but required" behavior due specifically to this being a backwards-incompatible change?

    Yes. That's the main factor. I've since learnt that there's sadly more to it though :( (see below).

    Does that mean I don't have the problem you're solving by reversing the order of namespace lookup, and I can just pass globals and locals in like normal?

    I think it depends on the ergonomics of the function you're trying to achieve. I admit I haven't been fully keeping up with the inspect.get_annotations issue (sorry!), but here's what I learnt from get_type_hints:

    (Partly copied over from PR 25632)
    Example:

    from typing import TypeVar, Generic
    T = TypeVar('T')
    
    class A(Generic[T]):
        __annotations__ = dict(
            some_b='B'
        )
    
    
    class B(Generic[T]):
        class A(Generic[T]):
            pass
        __annotations__ = dict(
            my_inner_a1='B.A',
            my_inner_a2=A,
            my_outer_a='A'  # unless somebody calls get_type_hints with localns=B.__dict__
        )

    >> get_type_hints(B)

    Currently (globalns then localns):
    {'my_inner_a1': <class '__main__.B.A'>, 'my_inner_a2': <class '__main__.B.A'>, 'my_outer_a': <class '__main__.A'>}

    Swapped (localns then globalns):
    {'my_inner_a1': <class '__main__.B.A'>, 'my_inner_a2': <class '__main__.B.A'>, 'my_outer_a': <class '__main__.B.A'>}

    I realized that looking into globalns then localns is a necessary evil: doing the converse (looking into localns first then globalns) would mean there is no way to point to the shadowed global A: it would always point to the local B.A. Unless of course you pass in localns=module.dict or localns=globals().

    Ultimately I think it's a sacrifice of ergonomics either way; looking into localns then globalns will require passing in the module's dict to refer to a global being shadowed, while the converse (globalns then localns) introduces surprising eval behavior. Both are kind of tacky, but globalns then localns is slightly less so IMO. If the user wants to specify the inner class, they can just annotate 'B.A', if they want the outer, it's 'A'. But the localns then globalns approach will always point to B.A, the only way to access the shadowed global A is to pass in the strange looking argument localns=module.dict.

    Phew, my head's spinning with localns and globalns now. Thanks for reading! I think it's your call. I'm inexperienced with elegant function design :P.

    @larryhastings
    Copy link
    Contributor

    Thank you for your in-depth and thoughtful reply!

    I think that APIs should behave in a predictable way. So, for my use case, I tell the user that I'm passing "globals" and "locals" into eval()--and I think I'd have to have a *very* compelling reason to swap them. Since I don't have the backwards-compatibility problem you do, I think I should keep it simple and predictable and not swap them.

    In reference to your example, I think it's natural enough that the A defined inside B eclipses the module-level A. That's what the user would *expect* to happen. If the user really wants to reference the module-level A, they have lots of options:

    • rename one or the other A to something else
    • establish an alias at module scope, and use that
    • explicitly say globals()['A']

    So I'm not worried about it.

    @Fidget-Spinner
    Copy link
    Member

    Hmm I think you're right - the normal behavior in Python is for local
    variables to shadow global variables, and as a user I expect the same for
    eval in inspect.get_annotation.

    @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.10 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