Navigation Menu

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

Recursive evaluation of ForwardRef (and PEP 563) #85513

Closed
wyfo mannequin opened this issue Jul 19, 2020 · 7 comments
Closed

Recursive evaluation of ForwardRef (and PEP 563) #85513

wyfo mannequin opened this issue Jul 19, 2020 · 7 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@wyfo
Copy link
Mannequin

wyfo mannequin commented Jul 19, 2020

BPO 41341
Nosy @gvanrossum, @vstinner, @ericvsmith, @ambv, @ilevkivskyi, @miss-islington, @isidentical, @wyfo
PRs
  • bpo-41341: Recursive evaluation of ForwardRef in get_type_hints #21553
  • [3.9] bpo-41341: Recursive evaluation of ForwardRef in get_type_hints (GH-21553) #21629
  • 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 2020-07-26.15:35:12.210>
    created_at = <Date 2020-07-19.11:28:27.457>
    labels = ['type-bug']
    title = 'Recursive evaluation of ForwardRef (and PEP 563)'
    updated_at = <Date 2020-07-26.15:35:12.209>
    user = 'https://github.com/wyfo'

    bugs.python.org fields:

    activity = <Date 2020-07-26.15:35:12.209>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-07-26.15:35:12.210>
    closer = 'gvanrossum'
    components = []
    creation = <Date 2020-07-19.11:28:27.457>
    creator = 'joperez'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41341
    keywords = ['patch']
    message_count = 7.0
    messages = ['373960', '373978', '374102', '374104', '374318', '374324', '374327']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'vstinner', 'eric.smith', 'lukasz.langa', 'levkivskyi', 'miss-islington', 'BTaskaya', 'joperez']
    pr_nums = ['21553', '21629']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue41341'
    versions = []

    @wyfo
    Copy link
    Mannequin Author

    wyfo mannequin commented Jul 19, 2020

    (This issue is already broached in https://bugs.python.org/issue38605, and a in some way in https://bugs.python.org/issue35834, but only as a secondary subject, that's why I've opened a ticket on this particular issue)

    ForwardRef of ForwardRef are not currently evaluated by get_type_hints, only the first level is, as illustrated in these examples:

    from typing import ForwardRef, Optional, get_type_hints
    def func(a: "Optional[\"int\"]"):
        pass
    assert get_type_hints(func)["a"] == Optional[ForwardRef("int")] 
    # one would expect get_type_hints(func)["a"] == Optional[int] 
    from __future__ import annotations
    from typing import ForwardRef, Optional, get_type_hints
    def func(a: Optional["int"]):
        pass
    assert get_type_hints(func)["a"] == Optional[ForwardRef("int")]
    # one would expect get_type_hints(func)["a"] == Optional[int] (which is the case without the import of __future__.annotations!)

    On the one hand I find this behavior quite counter-intuitive; I rather think ForwardRef as kind of internal (and wonder why there is no leading underscore, like _GenericAlias where it's used) and I don't understand the purpose of exposing it as the result of the public API get_type_hints. By the way, if ForwardRef can be obtained by retrieving annotations without get_type_hints, stringified annotations (especially since PEP-563) make get_type_hints kind of mandatory, and thus make ForwardRef disappeared (only at the first level so …)

    On the other hand, the second example show that adoptions of postponed annotations can change the result of get_type_hints; several libraries relying of get_type_hints could be broken.

    An other issue raised here is that if these ForwardRef are not evaluated by get_type_hints, how will be done their evaluatation by the user? It would require to retrieve some globalns/localns — too bad, it's exactly what is doing get_type_hints. And if the ForwardRef is in a class field, the class globalns/localns will have to be kept somewhere while waiting to encounter these random ForwardRef; that's feasible, but really tedious.

    Agreeing with Guido Von Rossum (https://bugs.python.org/msg370232), this behavior could be easily "fixed" in get_type_hints.
    Actually, there would be only one line to change in ForwardRef._evaluate:

    # from
    self.__forward_value__ = _type_check(
        eval(self.__forward_code__, globalns, localns),
        "Forward references must evaluate to types.",
        is_argument=self.__forward_is_argument__)
    # to
    self.__forward_value__ = _eval_type(
        _type_check(
            eval(
                self.__forward_code__, globalns, localns),
                "Forward references must evaluate to types.",
                is_argument=self.__forward_is_argument__,
            ),
        globalns,
        localns,
    )
    
    And if this fix could solve the "double ForwardRef" issue mentionned in https://bugs.python.org/issue38605, it would also resolve https://bugs.python.org/issue35834 in the same time, raising NameError in case of unknown ForwardRef with postponed annotation.

    @wyfo wyfo mannequin added type-bug An unexpected behavior, bug, or error labels Jul 19, 2020
    @wyfo
    Copy link
    Mannequin Author

    wyfo mannequin commented Jul 20, 2020

    Ok, I admit that I did not think about recursive type when proposing this "fix".
    I've tried an implementation that just stop when recursion is encountered in a PR.

    @gvanrossum
    Copy link
    Member

    New changeset 653f420 by wyfo in branch 'master':
    bpo-41341: Recursive evaluation of ForwardRef in get_type_hints (bpo-21553)
    653f420

    @gvanrossum
    Copy link
    Member

    Łukasz, can I please have a decision on whether to backport this bugfix to 3.9?

    See my comment about that:
    #21553 (review)

    @ambv
    Copy link
    Contributor

    ambv commented Jul 26, 2020

    Given the previous behavior was clearly a bug and after looking at the PR, I think it should go into 3.9.0rc1.

    @miss-islington
    Copy link
    Contributor

    New changeset 41d1c04 by Miss Islington (bot) in branch '3.9':
    bpo-41341: Recursive evaluation of ForwardRef in get_type_hints (GH-21553)
    41d1c04

    @gvanrossum
    Copy link
    Member

    Thank you Joseph Perez! Looking forward to more of your contributions.

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

    No branches or pull requests

    3 participants