python / cpython

The Python programming language
https://www.python.org
Other
62.87k stars 30.12k forks source link

singledispatchmethod raises an error when relying on a forward declaration #86153

Open b62e5afe-fdc6-42de-985a-faeb74e5c5a6 opened 4 years ago

b62e5afe-fdc6-42de-985a-faeb74e5c5a6 commented 4 years ago
BPO 41987
Nosy @gvanrossum, @glyph, @ambv, @ethanhs, @joernheissler, @isidentical, @mental32, @ryansobol, @AlexWaygood
PRs
  • python/cpython#23216
  • 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: ```python assignee = None closed_at = None created_at = labels = ['type-bug', 'library', '3.9', '3.10'] title = 'singledispatchmethod raises an error when relying on a forward declaration' updated_at = user = 'https://github.com/glyph' ``` bugs.python.org fields: ```python activity = actor = 'AlexWaygood' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'glyph' dependencies = [] files = [] hgrepos = [] issue_num = 41987 keywords = ['patch'] message_count = 16.0 messages = ['378346', '378350', '378353', '378361', '379896', '379911', '380797', '380799', '380800', '380803', '380832', '380843', '380845', '380852', '409572', '410269'] nosy_count = 9.0 nosy_names = ['gvanrossum', 'glyph', 'lukasz.langa', 'ethan smith', 'joernheissler', 'BTaskaya', 'mental', 'ryansobol', 'AlexWaygood'] pr_nums = ['23216'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue41987' versions = ['Python 3.9', 'Python 3.10'] ```

    b62e5afe-fdc6-42de-985a-faeb74e5c5a6 commented 4 years ago

    This example:

    from __future__ import annotations
    from functools import singledispatchmethod
    
    class Comparable:
        @singledispatchmethod
        def compare(self, arg: object):
            raise NotImplementedError("what")
    
        @compare.register
        def _(self, arg: Comparable):
            return "somewhat similar"

    print(Comparable().compare(Comparable()))

    Produces this result:

    File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/typing.py", line 518, in _evaluate eval(self.__forward_code__, globalns, localns), File "\<string>", line 1, in \<module> NameError: name 'Comparable' is not defined

    It seems like perhaps singledispatchmethod should defer its type evaluation to its first invocation?

    isidentical commented 4 years ago

    AFAIK the normal way of registering types (dispatcher.register(\<type>)) also requires that registered type be defined at the execution type. I guess, if we are going to support such a thing, we might end up with supporting passing strings into the .register() as the initial argument of matching type to be consistent. Anyways, this would be a feature request for 3.10+, so changing the version info.

    gvanrossum commented 4 years ago

    This behavior (only relevant with from __future__ import annotations) has been around since @singledispatchmethod was introduced in 3.8, so I agree we should treat it as a feature request.

    In the meantime, maybe a workaround is to move the register call out of the class? It looks a little bit ugly but probably works.

    (Disclaimer: I'm not familiar with singledispatchmethod.)

    b62e5afe-fdc6-42de-985a-faeb74e5c5a6 commented 4 years ago

    The behavior is the same with a traditional quoted forward declaration, so it’s not specific to the __future__ import; I just phrased the example that way to show how it’s going to look in the future and to illustrate how it might crop up in a way which is maximally confusing to users less familiar with the internals of type annotations.

    63dff802-1043-45b8-a784-38127289711f commented 3 years ago

    It's worth pointing out that a similar error is produced for a forward-referenced return type of a registered method, but only for python3.9. For example:

    from __future__ import annotations
    from functools import singledispatchmethod
    
    class Integer:
        def __init__(self, value: int):
            self.value = value
    
        def __str__(self) -> str:
            return str(self.value)
    
        @singledispatchmethod
        def add(self, other: object) -> Integer:
            raise NotImplementedError(f"Unsupported type {type(other)}")
    
        @add.register
        def _(self, other: int) -> "Integer":
            return Integer(self.value + other)

    print(Integer(2).add(40))

    This code runs without error in python3.8, and I am using this technique in code running in a production environment.

    $ python3.8 --version
    Python 3.8.6
    $ python3.8 integer.py
    42

    However, this code throws a NameError in python3.9.

    $ python3.9 --version
    Python 3.9.0
    $ python3.9 integer.py
    Traceback (most recent call last):
      File "/Users/ryansobol/Downloads/integer.py", line 5, in <module>
        class Integer:
      File "/Users/ryansobol/Downloads/integer.py", line 17, in Integer
        def _(self, other: int) -> "Integer":
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/functools.py", line 909, in register
        return self.dispatcher.register(cls, func=method)
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/functools.py", line 860, in register
        argname, cls = next(iter(get_type_hints(func).items()))
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py", line 1386, in get_type_hints
        value = _eval_type(value, globalns, localns)
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py", line 254, in _eval_type
        return t._evaluate(globalns, localns, recursive_guard)
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py", line 497, in _evaluate
        self.__forward_value__ = _eval_type(
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py", line 254, in _eval_type
        return t._evaluate(globalns, localns, recursive_guard)
      File "/usr/local/Cellar/python@3.9/3.9.0_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/typing.py", line 493, in _evaluate
        eval(self.__forward_code__, globalns, localns),
      File "<string>", line 1, in <module>
    NameError: name 'Integer' is not defined

    I know that some may see this issue as a feature request for 3.10+. However, for me, it is a bug preventing my code from migrating to 3.9.

    gvanrossum commented 3 years ago

    I'm not an expert on singledispatch. It seems the get_type_hints() call is present in 3.8 as well.

    Could you investigate and find a root cause? Then maybe we can fix it. (If you come up with a PR that would be very much appreciated.)

    gvanrossum commented 3 years ago

    I spent some time debugging this looking for the root cause.

    I think it looks like the recursion check in ForwardRef._evaluate() fails to trigger. At some point recursiveguard is a frozen set containing "'Integer'" (i.e. a string whose first and last character are single quotes, while self.\_forward_arg__ is 'Integer' (i.e. a string that does not contain quotes).

    I'm running out of time for the rest of the investigation, so feel free to confirm this and go down the rabbit hole from there...

    e0bb9de3-d923-49c1-9bb3-91ce496ad4a0 commented 3 years ago

    Interesting! thanks for the hint guido I'll try to investigate further.

    This sounds like a regression (to me at least), in that case should a separate issue & patch PR be opened on the bpo or should this issue be used instead?

    gvanrossum commented 3 years ago

    Keep this issue.

    gvanrossum commented 3 years ago

    FWIW here's a minimal demo:

    from __future__ import annotations
    from typing import get_type_hints
    
    class C:
        def func(self, a: "C"):
            pass
    
        print(get_type_hints(func))

    In 3.8 this prints

    {'a': ForwardRef('C')}

    while in 3.9 it raises NameError:
    Traceback (most recent call last):
      File "C:\Users\gvanrossum\cpython\t.py", line 4, in <module>
        class C:
      File "C:\Users\gvanrossum\cpython\t.py", line 8, in C
        print(get_type_hints(func))
      File "C:\Python39\lib\typing.py", line 1386, in get_type_hints
        value = _eval_type(value, globalns, localns)
      File "C:\Python39\lib\typing.py", line 254, in _eval_type
        return t._evaluate(globalns, localns, recursive_guard)
      File "C:\Python39\lib\typing.py", line 497, in _evaluate
        self.__forward_value__ = _eval_type(
      File "C:\Python39\lib\typing.py", line 254, in _eval_type
        return t._evaluate(globalns, localns, recursive_guard)
      File "C:\Python39\lib\typing.py", line 493, in _evaluate
        eval(self.__forward_code__, globalns, localns),
      File "<string>", line 1, in <module>
    NameError: name 'C' is not defined
    gvanrossum commented 3 years ago

    So the biggest difference I see is that ForwardRef._evaluate() has grown a recursive_guard argument in 3.9. This makes me think that in 3.8, only one level of evaluation was happening, and in 3.8, we keep evaluating until we don't see a string or ForwardRef.

    The specific examples all happen at a point where the forward ref "C" cannot be resolved at all yet (since they're happening *in the class body*).

    Possibly the best way out is to treat unresolved references differently, and just return the ForwardRef to the caller -- after all this is what the example does in 3.8.

    63dff802-1043-45b8-a784-38127289711f commented 3 years ago

    Does anyone know why the treatment of unresolved references was changed in 3.9?

    63dff802-1043-45b8-a784-38127289711f commented 3 years ago

    Also, I'm a bit puzzled about something from the previously mentioned Integer class and its use of __future__.annotations.

    Why is it possible to declare an Integer return type for the add() method, but only possible to declare an "Integer" forward reference for the _() method?

    gvanrossum commented 3 years ago

    Does anyone know why the treatment of unresolved references was changed in 3.9?

    Probably to prepare for 3.10, where from _future__ import annotations is the default.

    Also, I'm a bit puzzled about something from the previously mentioned Integer class and its use of __future__.annotations.

    Why is it possible to declare an Integer return type for the add() method, but only possible to declare an "Integer" forward reference for the _() method?

    I don't know -- you might want to look through the source code of singledispatch. Maybe the flow through the initial decorator is different than the flow through register().

    11ae746e-d782-4f33-a097-758c9f315fb2 commented 2 years ago

    Hello!

    git-bisect points at https://bugs.python.org/issue41341 https://github.com/python/cpython/pull/21553

    It breaks both the examples from https://bugs.python.org/issue41987#msg379896 and https://bugs.python.org/issue41987#msg380803

    gvanrossum commented 2 years ago

    Thanks for the bisection. It's not surprising that that's the culprit, and in other situations that's the right thing to do. I'm not sure how to address this without breaking other stuff -- maybe leave the ForwardRef if evaluating it doesn't work? But that's likely to have other subtle side effects -- we still want simple typos (or other reasons why a reference is legitimately broken) to go unchecked. Maybe singledispatch can catch the error and fall back on looking at bare __annotations__?