Nothing Special   »   [go: up one dir, main page]

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

TypedDict inheritance doesn't work with get_type_hints and postponed evaluation of annotations across modules #85421

Closed
keithblaha mannequin opened this issue Jul 8, 2020 · 18 comments
Labels
3.7 (EOL) end of life 3.8 (EOL) end of life 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

@keithblaha
Copy link
Mannequin
keithblaha mannequin commented Jul 8, 2020
BPO 41249
Nosy @gvanrossum, @ambv, @Kronuz, @ilevkivskyi, @miss-islington, @septatrix, @keithblaha, @Fidget-Spinner
PRs
  • bpo-41249: Fix postponed annotations for TypedDict #27017
  • [3.10] bpo-41249: Fix postponed annotations for TypedDict (GH-27017) #27204
  • [3.9] bpo-41249: Fix postponed annotations for TypedDict (GH-27017) #27205
  • 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-07-17.09:37:32.514>
    created_at = <Date 2020-07-08.23:47:38.289>
    labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7', 'library']
    title = "TypedDict inheritance doesn't work with get_type_hints and postponed evaluation of annotations across modules"
    updated_at = <Date 2021-07-17.09:37:32.513>
    user = 'https://github.com/keithblaha'

    bugs.python.org fields:

    activity = <Date 2021-07-17.09:37:32.513>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-17.09:37:32.514>
    closer = 'lukasz.langa'
    components = ['Library (Lib)']
    creation = <Date 2020-07-08.23:47:38.289>
    creator = 'keithblaha'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41249
    keywords = ['patch']
    message_count = 18.0
    messages = ['373360', '373752', '373758', '394729', '394738', '395279', '396296', '396919', '396923', '396926', '396946', '397690', '397692', '397694', '397695', '397710', '397711', '397712']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'lukasz.langa', 'Kronuz', 'levkivskyi', 'miss-islington', 'Nils Kattenbeck', 'keithblaha', 'kj']
    pr_nums = ['27017', '27204', '27205']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue41249'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @keithblaha
    Copy link
    Mannequin Author
    keithblaha mannequin commented Jul 8, 2020

    Copied from python/typing#737

    I came across this issue while using inheritance to express required keys in a TypedDict, as is recommended by the docs.

    It's most easily explained by a minimal example I cooked up. Let's say we have a module foo.py:

    from __future__ import annotations
    
    from typing import Optional
    from typing_extensions import TypedDict
    
    class Foo(TypedDict):
        a: Optional[int]

    And another module bar.py:

    from __future__ import annotations
    
    from typing import get_type_hints
    from foo import Foo
    
    class Bar(Foo, total=False):
        b: int
    
    print(get_type_hints(Bar))

    Note that both foo.py and bar.py have adopted postponed evaluation of annotations (PEP-563) by using the __future__ import.

    If we execute bar.py, we get the error message NameError: name 'Optional' is not defined.

    This is due to the combination of:

    get_type_hints relies on the MRO to resolve types: https://github.com/python/cpython/blob/3.7/Lib/typing.py#L970
    TypedDict does not preserve the original bases, so Foo is not in the MRO for Bar:
    
    typing/typing_extensions/src_py3/typing_extensions.py
    
    Line 1652 in d79edde
    
         tp_dict = super(_TypedDictMeta, cls).__new__(cls, name, (dict,), ns) 

    Thus, get_type_hints is unable to resolve the types for annotations that are only imported in foo.py.

    I ran this example using typing_extensions 3.7.4.2 (released via #709) and Python 3.7.3, but it seems like this would be an issue using the current main branches of both repositories as well.

    I'm wondering what the right approach is to tackling this issue. It is of course solvable by defining Bar in foo.py instead, but it isn't ideal or intuitive to always need to inherit from a TypedDict in the same module.

    I was thinking that similarly to __required_keys__ and __optional_keys__, the TypedDict could preserve its original bases in a new dunder attribute, and get_type_hints could work off of that instead of MRO when it is dealing with a TypedDict. I would be willing to contribute the PRs to implement this if the design is acceptable, but am open to other ideas as well.

    @keithblaha keithblaha mannequin added 3.10 only security fixes 3.7 (EOL) end of life 3.8 (EOL) end of life 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 8, 2020
    @ilevkivskyi
    Copy link
    Member

    I was thinking that similarly to __required_keys__ and __optional_keys__, the TypedDict could preserve its original bases in a new dunder attribute, and get_type_hints could work off of that instead of MRO when it is dealing with a TypedDict. I would be willing to contribute the PRs to implement this if the design is acceptable

    TBH this is not very elegant, but I think you can go ahead with this (at least as a quick fix) since I don't see a better solution yet.

    @keithblaha
    Copy link
    Mannequin Author
    keithblaha mannequin commented Jul 16, 2020

    TBH this is not very elegant, but I think you can go ahead with this (at least as a quick fix) since I don't see a better solution yet.

    Agreed, given that the current workaround of implementing them in the same module works I think I will stick with that while this is brainstormed further.

    @Septatrix
    Copy link
    Mannequin
    Septatrix mannequin commented May 29, 2021

    What is/was the initial reason to not preserve the MRO for a TypedDict?
    The only thing which came to my mind would be instantiation performance but as annotations are not evaluated by default and on the right-hide side of assignment most people will use dict literals I am not sure if this is still relevant. Otherwise it might even be simpler to just preserve the original bases in TypedDict but please correct me if I overlooked something

    @gvanrossum
    Copy link
    Member

    What is/was the initial reason to not preserve the MRO for a TypedDict?

    Hm, I can't say for sure. I believe it had something to do with TypedDict instances being instances of dict at runtime, but I can't actually reconstruct the reason. Maybe it's written up in PEP-589, but I suspect not (I skimmed and couldn't find it). If you ask on typing-sig maybe David Foster (who contributed the initial idea and implementation) remembers.

    @Septatrix
    Copy link
    Mannequin
    Septatrix mannequin commented Jun 7, 2021

    I believe it had something to do with TypedDict instances being instances of dict at runtime, but I can't actually reconstruct the reason.

    Hm that may be true.
    My limited low-level Python knowledge leads me to believe that this could also be done using __new__ but I also read that most magic methods get called as type(Foo).__magic__(bar, ...) so that might not be possible.
    (However also no methods can be declared on a TypedDict class so that might not be a problem?)

    Maybe it's written up in PEP-589, but I suspect not (I skimmed and couldn't find it).

    I read it completely and could not find anything

    If you ask on typing-sig maybe David Foster (who contributed the initial idea and implementation) remembers.

    I asked here on typing-sig but did not yet get any responses.

    @gvanrossum
    Copy link
    Member

    Note that this issue is now only a problem of you use from __future__ import annotations -- we rolled the default behavior for 3.10 back to what it was in 3.9.

    I am out of time to argue about why we chose this behavior, alas.

    @Kronuz
    Copy link
    Mannequin
    Kronuz mannequin commented Jul 3, 2021

    The way I fixed this is I added __forward_module__ to typing.ForwardRef, so that it can resolve the forward reference with the same globals as the ones specified by the module in __forward_module__. TypedDict's metaclass should then pass the dictionary’s module name to the annotations’ forward references via the added module’s keyword argument in typing._type_check(). I can work in a pull request with this solution and discuss any potential problems.

    @Septatrix
    Copy link
    Mannequin
    Septatrix mannequin commented Jul 3, 2021

    The way I fixed this is I added __forward_module__ to typing.ForwardRef, so that it can resolve the forward reference with the same globals as the ones specified by the module in __forward_module__. TypedDict's metaclass should then pass the dictionary’s module name to the annotations’ forward references via the added module’s keyword argument in typing._type_check(). I can work in a pull request with this solution and discuss any potential problems.

    While this seems like a good solution I would still like to figure out why TypedDict do not preserve MRO. Because for now I have not found a reason nor did someone on the typing-sig mailinglist have a clue. Should there (no longer) be a reason for this then this problem has a trivial solution (just re-add the MRO and use that).

    @Kronuz
    Copy link
    Mannequin
    Kronuz mannequin commented Jul 3, 2021

    Nils, unfortunately, fixing the MRO here won’t fix the issue because TypedDict.__annotations__ in the class copies the annotations from the parent classes, and when the type evaluation is made, it’s made using the copied annotation found in the bottommost class (which is thus then expected to be a forward reference in the same module as the class that inherited them. This producing the exact same problem of missing type.

    The most likely reason for incomplete MRO is that TypeDict extends a class’ __annotations__ with all of it’s parent’s __annotations__, so the final class has the complete set of annotations of all of its parents, so having the full inheritance chain made less sense, after all the final dictionary class has all the annotations.

    On Jul 3, 2021, at 13:43, Nils Kattenbeck report@bugs.python.org wrote:

    
    Nils Kattenbeck nilskemail@gmail.com added the comment:

    The way I fixed this is I added __forward_module__ to typing.ForwardRef, so that it can resolve the forward reference with the same globals as the ones specified by the module in __forward_module__. TypedDict's metaclass should then pass the dictionary’s module name to the annotations’ forward references via the added module’s keyword argument in typing._type_check(). I can work in a pull request with this solution and discuss any potential problems.

    While this seems like a good solution I would still like to figure out why TypedDict do not preserve MRO. Because for now I have not found a reason nor did someone on the typing-sig mailinglist have a clue. Should there (no longer) be a reason for this then this problem has a trivial solution (just re-add the MRO and use that).

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue41249\>


    @Kronuz
    Copy link
    Mannequin
    Kronuz mannequin commented Jul 4, 2021

    I added a pull request with my fix here:
    #27017

    @gvanrossum
    Copy link
    Member

    New changeset 889036f by Germán Méndez Bravo in branch 'main':
    bpo-41249: Fix postponed annotations for TypedDict (GH-27017)
    889036f

    @gvanrossum
    Copy link
    Member

    How far can/should we backport this?

    @gvanrossum gvanrossum added the 3.11 only security fixes label Jul 17, 2021
    @Fidget-Spinner
    Copy link
    Member

    How far can/should we backport this?

    It will work in 3.10 and 3.9 without issues. However, I don't remember if bugfixes for __future__ features require special treatment/are excluded from normal bugfix backports. I vaguely remember us not backporting from __future__ annotations very far back (since they usually broke backwards compatibility).

    Maybe 3.10 is enough?

    @gvanrossum
    Copy link
    Member

    Let’s both, since this feels like a real bug fix to me.

    @ambv
    Copy link
    Contributor
    ambv commented Jul 17, 2021

    New changeset 480f29f by Miss Islington (bot) in branch '3.10':
    bpo-41249: Fix postponed annotations for TypedDict (GH-27017) (bpo-27204)
    480f29f

    @ambv
    Copy link
    Contributor
    ambv commented Jul 17, 2021

    New changeset fa674bd by Ken Jin in branch '3.9':
    [3.9] bpo-41249: Fix postponed annotations for TypedDict (GH-27017) (GH-27205)
    fa674bd

    @ambv
    Copy link
    Contributor
    ambv commented Jul 17, 2021

    Thanks! ✨ 🍰 ✨

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 (EOL) end of life 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

    4 participants