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

inspect not capturing type annotations created by __class_getitem__ #89601

Closed
Tracked by #89828
rhettinger opened this issue Oct 11, 2021 · 22 comments
Closed
Tracked by #89828

inspect not capturing type annotations created by __class_getitem__ #89601

rhettinger opened this issue Oct 11, 2021 · 22 comments
Labels
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

@rhettinger
Copy link
Contributor

BPO 45438
Nosy @gvanrossum, @rhettinger, @ambv, @serhiy-storchaka, @ilevkivskyi, @miss-islington, @Fidget-Spinner, @martinitus
PRs
  • bpo-45438: format of inspect.Signature with generic builtins #29212
  • [3.10] bpo-45438: format of inspect.Signature with generic builtins (GH-29212) #29253
  • [3.9] bpo-45438: format of inspect.Signature with generic builtins (GH-29212) #29254
  • 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-10-29.08:43:15.006>
    created_at = <Date 2021-10-11.22:05:54.548>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'inspect not capturing type annotations created by __class_getitem__'
    updated_at = <Date 2021-10-29.08:43:15.004>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2021-10-29.08:43:15.004>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-10-29.08:43:15.006>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2021-10-11.22:05:54.548>
    creator = 'rhettinger'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45438
    keywords = ['patch']
    message_count = 21.0
    messages = ['403692', '403697', '403759', '403760', '403826', '404961', '404966', '404967', '404970', '404974', '404976', '404993', '405034', '405128', '405132', '405133', '405138', '405161', '405163', '405283', '405292']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'rhettinger', 'lukasz.langa', 'serhiy.storchaka', 'levkivskyi', 'miss-islington', 'kj', 'martinitus']
    pr_nums = ['29212', '29253', '29254']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45438'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @rhettinger
    Copy link
    Contributor Author

    In the example below, __annotations__ is correct but not the corresponding Signature object.

    -----------------------------------------------------------------------

    from typing import List
    
    def f(s: List[float]) -> None: pass
    
    def g(s: list[float]) -> None: pass
    >>> inspect.signature(f)
    <Signature (s: List[float]) -> None>
    
    >>> inspect.signature(g)
    <Signature (s: list) -> None>

    g.__annotations__
    {'s': list[float], 'return': None}

    @rhettinger rhettinger added 3.10 only security fixes stdlib Python modules in the Lib dir labels Oct 11, 2021
    @gvanrossum
    Copy link
    Member

    Raymond, the bug must be in the Python code in inspect.py. Could you dig a little deeper there? I don't know much about it. Specifically I think the problem may just be in the repr() of class Parameter:

    >>> inspect.signature(g).parameters['s']             
    <Parameter "s: list">
    >>> inspect.signature(g).parameters['s'].annotation
    list[float]
    >>>

    @rhettinger
    Copy link
    Contributor Author

    It looks like the error is in inspect.formatannotation().

    For instances of type, that function incorrectly returns only annotation.__qualname__.

    Instead, it should return repr(annotation) which would include the args.

    =================================================================

    def formatannotation(annotation, base_module=None):
        if getattr(annotation, '__module__', None) == 'typing':
            return repr(annotation).replace('typing.', '')
        if isinstance(annotation, type):                # <== Erroneous case
            if annotation.__module__ in ('builtins', base_module):
                return annotation.__qualname__
            return annotation.__module__+'.'+annotation.__qualname__
        return repr(annotation)

    @gvanrossum
    Copy link
    Member

    Sure. Waiting for your PR. :-)

    @serhiy-storchaka
    Copy link
    Member

    The cause is that isinstance(list[int], type) returns True. It can cause bugs in other parts of the code which test for instance of type. For example:

    >>> types.resolve_bases((typing.List[int],))
    (<class 'list'>, <class 'typing.Generic'>)
    >>> types.resolve_bases((list[int],))
    (list[int],)
    
    >>> types.prepare_class('A', (int,), {'metaclass': typing.Type[int]})
    (typing.Type[int], {}, {})
    >>> types.prepare_class('A', (int,), {'metaclass': type[int]})
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/serhiy/py/cpython/Lib/types.py", line 125, in prepare_class
        meta = _calculate_meta(meta, bases)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/serhiy/py/cpython/Lib/types.py", line 139, in _calculate_meta
        if issubclass(base_meta, winner):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    TypeError: issubclass() argument 2 cannot be a parameterized generic
    
    >>> @functools.singledispatch
    ... def g(a): pass
    ... 
    >>> @g.register
    ... def g2(a: typing.List[int]): pass
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
      File "/home/serhiy/py/cpython/Lib/functools.py", line 863, in register
        raise TypeError(
        ^^^^^^^^^^^^^^^^
    TypeError: Invalid annotation for 'a'. typing.List[int] is not a class.
    >>> @g.register(list[int])
    ... def g2(a): pass
    ... 
    >>> @g.register
    ... def g3(a: typing.List[int]): pass
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
      File "/home/serhiy/py/cpython/Lib/functools.py", line 863, in register
        raise TypeError(
        ^^^^^^^^^^^^^^^^
    TypeError: Invalid annotation for 'a'. typing.List[int] is not a class.
    >>> @g.register
    ... def g3(a: list[int]): pass
    ... 

    And many other examples, too many to show here.

    Was it mistake to make isinstance(list[int], type) returning True?

    @martinitus
    Copy link
    Mannequin

    martinitus mannequin commented Oct 25, 2021

    Can confirm for 3.9.7 as well.

    @martinitus
    Copy link
    Mannequin

    martinitus mannequin commented Oct 25, 2021

    I just created a PR and signed the contributor agreement. Waiting for it to update :-) Comments welcome!

    @serhiy-storchaka
    Copy link
    Member

    There are two ways to fix the larger issue.

    1. Make issubclass(types.GenericAlias, type) returning True, and also make isinstance(typing.List[int], type) returning True and issubclass(typing._GenericAlias, type) returning True, and analyze every place in the interpreter and the stdlib which calls isinstance(..., type) or issubclass(..., type) and ensure that they work with types.GenericAlias and typing._GenericAlias and their instances as well as with ordinary types, fix them and add tests for them. And perhaps do the same for types.UnionType, typing._UnionGenericAlias, typing.TypeVar, typing.NewType, etc, etc.

    2. Make isinstance(list[int], type) returning False. It would be nice to ad also tests for the same cases as in option 1, but it is not so urgent, and I expect that in most cases the current behavior which matches the status quo is expected.

    I tried to implement option 1, but it is just too much places, so it would take a large amount of time which I do not have right now. First than invest my time in this I want to make sure which option is desirable in long term.

    Guido, Ivan, Ken Jin, what would you say?

    @ilevkivskyi
    Copy link
    Member

    Was it mistake to make isinstance(list[int], type) returning True?

    What was the motivation for this? At first glance returning True looks wrong.

    @gvanrossum
    Copy link
    Member

    I really don't recall if we even seriously considered what isinstance(list[int], type) should return. PEP-585 doesn't mention it. I presume it falls out of the way it's being tested and the way list[int] passes most attribute requests on to the origin (i.e., to list).

    Given that this has returned True for two releases now I'm very reluctant to changing this now. So our best option is pursuing Serhiy's option 1, at least partially. The fix for the OP should be simple enough, right?

    @serhiy-storchaka
    Copy link
    Member

    Two years is not so long for a bug. We fixed 8-year and 12-year bugs.

    The issue is that this feature is internally inconsistent (isinstance() is not consistent with issubclass()), the C implementation of list[int] is not consistent with the Python implementation of List[int], and a lot of code was not tested with this feature, so we perhaps have a lot of bugs in the stdlib and in the third-party libraries. Are we going to backport changes for making code working with GenericAlias as a type to 3.9 and 3.10? If not, we will actually add new features in 3.11 and keep 3.9 and 3.10 broken. If yes, these changes can have larger effect than just making isinstance(list[int], type) returning False, and they can break more code.

    Note also that isinstance(List[int], type) was True before 3.7, and we intentionally made it False in 3.7 (it was required significant rewriting of the typing module and introducing __mro_entries__). Do we want to revert this decision?

    @gvanrossum
    Copy link
    Member

    The issue is that this feature is internally inconsistent (isinstance() is not consistent with issubclass()),

    issubclass(x, list[int]) rejects the second argument for reasons explained in the PEP. I don't think from that reasoning you can infer that list[int] is not a type. Rather the contrary -- if it's not a type the PEP could just have said "it's not a type" but it doesn't, it says "it's unacceptable as second arg to issubclass()". In fact, the PEP uses language ("instances of parameterized collections") that makes me think in the PEP author's mind list[int] *is* a type.

    the C implementation of list[int] is not consistent with the Python implementation of List[int],

    Consistency with typing was not a goal of the PEP.

    and a lot of code was not tested with this feature,

    What other places are there that are broken because of this?

    so we perhaps have a lot of bugs in the stdlib and in the third-party libraries.

    Without more evidence this sounds like speculation. *With* evidence you might well convince me.

    Are we going to backport changes for making code working with GenericAlias as a type to 3.9 and 3.10?

    I'm fine with backporting the fix in the PR, #73398.

    If not, we will actually add new features in 3.11 and keep 3.9 and 3.10 broken. If yes, these changes can have larger effect than just making isinstance(list[int], type) returning False, and they can break more code.

    Are there other fixes you already know about besides #73398? That PR seems 100% beneficial.

    Note also that isinstance(List[int], type) was True before 3.7, and we intentionally made it False in 3.7 (it was required significant rewriting of the typing module and introducing __mro_entries__). Do we want to revert this decision?

    I don't recall why we changed that, I thought it was a side effect of making the implementation faster, not because of a decision that we didn't want these to be treated as types. I looked at inspect.py in 3.6, and it seems its formatannotation() has a special case for annotations that come from the typing module (like List[int]). I guess nobody thought to have a test for that, so the bug in inspect.py slipped by when it was introduced in 3.9 -- we're relying on Raymond's keen eye here. But if we had had such a test, and it had alerted us to the problem in 3.9 when types.GenericAlias was introduced, I expect that we would have fixed it just like martinitus does in his PR.

    Anyway, I am willing to be swayed by evidence, but this bug in inspect.py isn't enough.

    @martinitus
    Copy link
    Mannequin

    martinitus mannequin commented Oct 26, 2021

    Just my two cents as a new contributor but long time user:

    • isinstance(list[int], type) returning False seems incredibly un-intuitive to me. I always see generics (e.g. list without type parameter) as higher kinded types, where passing a type argument via [] turns the hkt into a concrete type.

    • Backporting to 3.9 and 3.10 should be no issue

    • I am not deep enough in pythons type system to judge whether my PR is consistent/good. I really just tried out what works and "feels consistent".

    @gvanrossum
    Copy link
    Member

    New changeset d02ffd1 by Martin Rueckl in branch 'main':
    bpo-45438: format of inspect.Signature with generic builtins (bpo-29212)
    d02ffd1

    @miss-islington
    Copy link
    Contributor

    New changeset ce7a6af by Miss Islington (bot) in branch '3.10':
    bpo-45438: format of inspect.Signature with generic builtins (GH-29212)
    ce7a6af

    @miss-islington
    Copy link
    Contributor

    New changeset 21150c6 by Miss Islington (bot) in branch '3.9':
    bpo-45438: format of inspect.Signature with generic builtins (GH-29212)
    21150c6

    @gvanrossum
    Copy link
    Member

    Can we close this?

    @serhiy-storchaka
    Copy link
    Member

    issubclass(x, list[int]) rejects the second argument for reasons explained in the PEP.

    1. One problem is that isinstance(x, type) != issubclass(type(x), type)
      if x is list[int]. It is unprecedented, I cannot recall any other case
      in which isinstance() and issubclas() are inconsistent. Ant it breaks
      code because these two expressions are often considered equivalent and
      interchangeable in refactoring.

    2. Other problem is that isinstance(x, type) is used as a guard before
      using x as a type. isinstance(obj, type) and issubclass(obj, SomeClass) is a common idiom, because issubclass() raises an exception
      if its first argument is not a type. It is now broken for list[int].

    What other places are there that are broken because of this?
    $ find Lib -name '*.py' \! -path '*/test*' -exec egrep 'isinstance.*,
    type\)' '{}' + | wc -l
    55

    In msg403826 I showed few examples from just two files, but there are
    tens more potential examples. I'll show them all if I have enough of
    spare time.

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-40296.

    @gvanrossum
    Copy link
    Member

    OK, OK, I see your point. I'm curious what Łukasz thinks.

    @serhiy-storchaka
    Copy link
    Member

    I have opened separate issues for different cases and a meta-issue45665 for general discussion.

    @serhiy-storchaka serhiy-storchaka added 3.9 only security fixes 3.11 only security fixes labels Oct 29, 2021
    @serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error 3.9 only security fixes labels Oct 29, 2021
    @serhiy-storchaka serhiy-storchaka added the 3.11 only security fixes label Oct 29, 2021
    @serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Oct 29, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @dss010101
    Copy link

    is there a recommended workaround until this is fixed?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    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

    6 participants