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

Postponed annotations break inspection of dataclasses #78957

Closed
drhagen mannequin opened this issue Sep 23, 2018 · 20 comments
Closed

Postponed annotations break inspection of dataclasses #78957

drhagen mannequin opened this issue Sep 23, 2018 · 20 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes deferred-blocker type-bug An unexpected behavior, bug, or error

Comments

@drhagen
Copy link
Mannequin

drhagen mannequin commented Sep 23, 2018

BPO 34776
Nosy @gvanrossum, @gpshead, @scoder, @ericvsmith, @ned-deily, @methane, @ambv, @1st1, @Vlad-Shcherbina, @ilevkivskyi, @drhagen
PRs
  • bpo-34776: Fix dataclasses to support __future__ "annotations" mode #9518
  • [3.8] bpo-34776: Fix dataclasses to support __future__ "annotations" mode (GH-9518) #17531
  • [3.7] bpo-34776: Fix dataclasses to support __future__ "annotations" mode (GH-9518) #17532
  • 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 = 'https://github.com/1st1'
    closed_at = <Date 2019-12-09.16:41:55.416>
    created_at = <Date 2018-09-23.10:45:29.961>
    labels = ['3.8', 'deferred-blocker', 'type-bug', '3.7', '3.9']
    title = 'Postponed annotations break inspection of dataclasses'
    updated_at = <Date 2019-12-09.16:41:55.415>
    user = 'https://github.com/drhagen'

    bugs.python.org fields:

    activity = <Date 2019-12-09.16:41:55.415>
    actor = 'lukasz.langa'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2019-12-09.16:41:55.416>
    closer = 'lukasz.langa'
    components = []
    creation = <Date 2018-09-23.10:45:29.961>
    creator = 'drhagen'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34776
    keywords = ['patch']
    message_count = 20.0
    messages = ['326148', '326164', '326170', '326171', '326266', '326307', '326308', '326314', '327059', '327062', '337014', '337237', '355258', '355268', '355269', '355270', '357987', '358106', '358112', '358113']
    nosy_count = 12.0
    nosy_names = ['gvanrossum', 'gregory.p.smith', 'ivan', 'scoder', 'eric.smith', 'ned.deily', 'methane', 'lukasz.langa', 'yselivanov', 'vlad', 'levkivskyi', 'drhagen']
    pr_nums = ['9518', '17531', '17532']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue34776'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @drhagen
    Copy link
    Mannequin Author

    drhagen mannequin commented Sep 23, 2018

    The new postponed annotations have an unexpected interaction with dataclasses. Namely, you cannot get the type hints of any of the data classes methods.

    For example, I have some code that inspects the type parameters of a class's __init__ method. (The real use case is to provide a default serializer for the class, but that is not important here.)

    from dataclasses import dataclass
    from typing import get_type_hints
    
    class Foo:
        pass
    
    @dataclass
    class Bar:
        foo: Foo
    
    print(get_type_hints(Bar.__init__))
    

    In Python 3.6 and 3.7, this does what is expected; it prints {'foo': <class '__main__.Foo'>, 'return': <class 'NoneType'>}.

    However, if in Python 3.7, I add from __future__ import annotations, then this fails with an error:

    NameError: name 'Foo' is not defined
    

    I know why this is happening. The __init__ method is defined in the dataclasses module which does not have the Foo object in its environment, and the Foo annotation is being passed to dataclass and attached to __init__ as the string "Foo" rather than as the original object Foo, but get_type_hints for the new annotations only does a name lookup in the module where __init__ is defined not where the annotation is defined.

    I know that the use of lambdas to implement PEP-563 was rejected for performance reasons. I could be wrong, but I think this was motivated by variable annotations because the lambda would have to be constructed each time the function body ran. I was wondering if I could motivate storing the annotations as lambdas in class bodies and function signatures, in which the environment is already being captured and is code that usually only runs once.

    Original mailing list discussion: https://mail.python.org/pipermail/python-dev/2018-September/155289.html

    @drhagen drhagen mannequin added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Sep 23, 2018
    @ericvsmith
    Copy link
    Member

    [Adding to nosy people who were on the original email]

    Copying (part of) my response from the email thread:

    These work:
    print(get_type_hints(Bar.__init__, globals()))
    print(get_type_hints(Bar.__init__, Bar.__module__))

    But I agree that maybe doing something with dataclasses to address this would be good. Especially as the first one requires being in the same module as Foo.

    See this for Yury's self-described "hack-ish fix we can use" until we do something better:

    https://gist.github.com/1st1/37fdd3cc84cd65b9af3471b935b722df

    @1st1
    Copy link
    Member

    1st1 commented Sep 23, 2018

    See this for Yury's self-described "hack-ish fix we can use" until we do something better:

    Actually, I think I found a better solution that doesn't require any changes to anything besides dataclasses.

    Currently, dataclasses uses 'exec()' function to dynamically create methods like '__init__'. The generated code for '__init__' needs to access MISSING and _HAS_DEFAULT_FACTORY constants from the dataclasses module. To do that, we compile the code with 'exec()' with globals set to a dict with {MISSING, _HAS_DEFAULT_FACTORY} keys in it. This does the trick, but '__init__.__globals__' ends up pointing to that custom dict, instead of pointing to the module's dict.

    The other way around is to use a closure around __init__ to inject MISSING and _HAS_DEFAULT_FACTORY values *and* to compile the code in a proper __dict__ of the module the dataclass was defined in. Please take a look at the PR.

    @1st1
    Copy link
    Member

    1st1 commented Sep 23, 2018

    And FWIF I don't think we need to use lambdas for annotations to solve issues like this one.

    @ericvsmith
    Copy link
    Member

    Ned: I'm marking this as a release blocker because I'd like to get it in 3.7.1. If I can't do that in the next 5 hours or so, I'll remove the release blocker tag.

    @ericvsmith
    Copy link
    Member

    Unfortunately, I'm not going to be able to give this the attention it deserves before the 3.7.1 cutoff. The changes are sufficiently tricky that I want to make sure I think through the issue in the appropriate detail.

    Ned: I've made it not a release blocker. Please remove yourself as nosy if you don't care about this issue. I don't plan on making it a release blocker again, I just did that because it was so near the deadline.

    Yury: Sorry for not being able to get delve in to it sufficiently. Thanks for your work on it.

    I should be able to review this in the next few days, so we can get this in to 3.7.2.

    @1st1
    Copy link
    Member

    1st1 commented Sep 25, 2018

    NP, Eric, take your time. I agree that the PR isn't simple and needs a very careful review.

    @gpshead gpshead added the 3.8 only security fixes label Sep 25, 2018
    @gpshead
    Copy link
    Member

    gpshead commented Sep 25, 2018

    fwiw, agreed that this should wait for 3.7.2.

    @ambv
    Copy link
    Contributor

    ambv commented Oct 4, 2018

    Yury, thanks for your patch. I'll review it soon.

    Please note that postponed annotations only reveal a problem that we already had if anybody used a string forward reference:

    >>> from dataclasses import dataclass
    >>> from typing import get_type_hints
    >>> class C:
    ...   pass
    ...
    >>> @dataclass
    ... class D:
    ...   c: C
    ...
    >>> @dataclass
    ... class E:
    ...   c: "C"
    ...
    >>> get_type_hints(C.__init__)
    {}
    >>> get_type_hints(D.__init__)
    {'c': <class '__main__.C'>, 'return': <class 'NoneType'>}
    >>> get_type_hints(E.__init__)
    Traceback (most recent call last):
    ...
    NameError: name 'C' is not defined

    @1st1
    Copy link
    Member

    1st1 commented Oct 4, 2018

    Please note that postponed annotations only reveal a problem that we already had if anybody used a string forward reference:

    Yeah, makes sense. It's cool that the PR fixes string forward references as well.

    @Vlad-Shcherbina
    Copy link
    Mannequin

    Vlad-Shcherbina mannequin commented Mar 2, 2019

    Any chance this could get into 3.7.3?

    @ericvsmith
    Copy link
    Member

    I'm finally getting time to look at this. I'll see what I can do.

    @drhagen
    Copy link
    Mannequin Author

    drhagen mannequin commented Oct 23, 2019

    This PR has been sitting for a while. Any chance we can bring it over the finish line?

    @gpshead gpshead added the 3.9 only security fixes label Oct 23, 2019
    @1st1
    Copy link
    Member

    1st1 commented Oct 23, 2019

    FWIW we discussed my patch with Eric at PyCon 2019 and I think he had no issues with it. Eric, can I merge it? Should we backport to 3.8?

    @ericvsmith
    Copy link
    Member

    Yury: I'm okay with merging. If I recall, you were going to add a comment or two about the approach (a closure, if I remember correctly).

    I think we should backport to 3.8 and 3.7: it's a bug.

    @1st1
    Copy link
    Member

    1st1 commented Oct 23, 2019

    Yury: I'm okay with merging. If I recall, you were going to add a comment or two about the approach (a closure, if I remember correctly).

    Sure, I'll rebase and add a comment in a couple of days. I'll elevate the status so that I don't forget before 3.8.1

    @1st1 1st1 assigned 1st1 and unassigned ericvsmith Oct 23, 2019
    @ned-deily
    Copy link
    Member

    I'll elevate the status so that I don't forget before 3.8.1.

    Ping, 3.8.1 cutoff is coming up very soon.

    @ambv
    Copy link
    Contributor

    ambv commented Dec 9, 2019

    New changeset d219cc4 by Łukasz Langa (Yury Selivanov) in branch 'master':
    bpo-34776: Fix dataclasses to support __future__ "annotations" mode (bpo-9518)
    d219cc4

    @ambv
    Copy link
    Contributor

    ambv commented Dec 9, 2019

    New changeset 0d57db2 by Łukasz Langa (Miss Islington (bot)) in branch '3.8':
    bpo-34776: Fix dataclasses to support __future__ "annotations" mode (GH-9518) (bpo-17531)
    0d57db2

    @ambv
    Copy link
    Contributor

    ambv commented Dec 9, 2019

    New changeset 66d7a5d by Łukasz Langa (Miss Islington (bot)) in branch '3.7':
    bpo-34776: Fix dataclasses to support __future__ "annotations" mode (GH-9518) (bpo-17532)
    66d7a5d

    @ambv ambv closed this as completed Dec 9, 2019
    @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.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes deferred-blocker type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants