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

Shouldn't typing.get_type_hints() default globalns to {} instead of skipping base classes? #88634

Closed
will-ca mannequin opened this issue Jun 21, 2021 · 7 comments
Closed
Labels
3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@will-ca
Copy link
Mannequin

will-ca mannequin commented Jun 21, 2021

BPO 44468
Nosy @gvanrossum, @ilevkivskyi, @JelleZijlstra, @miss-islington, @Fidget-Spinner, @will-ca
PRs
  • bpo-44468: Never skip base classes in typing.get_type_hints(), even with invalid .__module__. #26862
  • [3.10] bpo-44468: Never skip base classes in typing.get_type_hints(), even with invalid .__module__. (GH-26862) #26920
  • 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-06-27.02:32:00.150>
    created_at = <Date 2021-06-21.04:11:39.691>
    labels = ['type-bug', '3.10', '3.11']
    title = "Shouldn't `typing.get_type_hints()` default `globalns` to `{}` instead of skipping base classes?"
    updated_at = <Date 2021-06-27.02:32:00.150>
    user = 'https://github.com/will-ca'

    bugs.python.org fields:

    activity = <Date 2021-06-27.02:32:00.150>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-06-27.02:32:00.150>
    closer = 'gvanrossum'
    components = []
    creation = <Date 2021-06-21.04:11:39.691>
    creator = 'WCA'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44468
    keywords = ['patch']
    message_count = 7.0
    messages = ['396205', '396222', '396279', '396315', '396376', '396559', '396561']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'levkivskyi', 'JelleZijlstra', 'miss-islington', 'kj', 'WCA']
    pr_nums = ['26862', '26920']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44468'
    versions = ['Python 3.10', 'Python 3.11']

    @will-ca
    Copy link
    Mannequin Author

    will-ca mannequin commented Jun 21, 2021

    An issue was recently closed that caused synthetic classes and base classes with invalid __module__ attributes to raise KeyError() in typing.get_type_hints():

    https://bugs.python.org/issue41515

    However, the implemented solution appears to be to skip those classes completely with a continue statement, instead of getting the annotations that may still be present by using an empty globals dictonary:

    https://github.com/python/cpython/pull/25352/files#diff-ddb987fca5f5df0c9a2f5521ed687919d70bb3d64eaeb8021f98833a2a716887

    In order to work around this issue in my local install of Blender, I had to change .get_type_hints() to use an empty dictionary for globalns when encountering invalid modules, rather than skipping them:

    https://developer.blender.org/T88986#1179812

    From reading the commit where the broken behaviour was first introduced— Which was described/designed as "backwards compatible"— It looks like the original behaviour was also to use an empty dictionary, and never skip:

    f350a26#diff-ddb987fca5f5df0c9a2f5521ed687919d70bb3d64eaeb8021f98833a2a716887R1501

    Using an empty dictionary also seemed to be mentioned in the bug report linked above.

    IMO using an empty dictionary and still returning annotations from classes with invalid modules seems like it'd be more sensible, predictable, and backwards-compatible, while skipping base classes is likely to just replace the obvious KeyError() with less reproducible and nastier errors caused by returning incomplete type hints.

    @will-ca will-ca mannequin added 3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error labels Jun 21, 2021
    @Fidget-Spinner
    Copy link
    Member

    Will, I think what you say makes sense.

    @guido / Ivan / Jelle

    What do you think?

    @gvanrossum
    Copy link
    Member

    I agree with the OP, we can do better here. Ken, if you feel like it, a fix
    should not be too complicated, right?

    @Fidget-Spinner
    Copy link
    Member

    Ken, if you feel like it, a fix should not be too complicated, right?

    Yup. Will's proposal is to change the try-except to:

    base_globals = getattr(sys.modules.get(base.__module__, None), '__dict__', {})

    @will, would you like to submit a PR for this? You'd need to sign a CLA. However, if you'd prefer I do it instead I'm happy to make the change.

    3.10.0 beta 4 is releasing on 2021-07-10, so preferably we should get the change in by then. It's the last beta before the release candidates.

    @will-ca
    Copy link
    Mannequin Author

    will-ca mannequin commented Jun 22, 2021

    I opened a PR with a fix and a couple comments on its method:

    #26862

    I also *think* I signed the CLA correctly. Not sure. I used my login name here, rather than my Github login, but my account is linked.

    Didn't add any blurb/changelog notes. Would that be needed?

    Thanks for looking at this issue quickly! Let me know if there's anything else I can or should do.

    @gvanrossum
    Copy link
    Member

    New changeset 7569c0f by will-ca in branch 'main':
    bpo-44468: Never skip base classes in typing.get_type_hints(), even with invalid .__module__. (GH-26862)
    7569c0f

    @miss-islington
    Copy link
    Contributor

    New changeset 3df23b5 by Miss Islington (bot) in branch '3.10':
    [3.10] bpo-44468: Never skip base classes in typing.get_type_hints(), even with invalid .__module__. (GH-26862) (GH-26920)
    3df23b5

    @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 3.11 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants