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

Strange @typing.no_type_check behavior for class variables #90729

Closed
sobolevn opened this issue Jan 29, 2022 · 14 comments
Closed

Strange @typing.no_type_check behavior for class variables #90729

sobolevn opened this issue Jan 29, 2022 · 14 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

@sobolevn
Copy link
Member

BPO 46571
Nosy @gvanrossum, @JelleZijlstra, @sobolevn, @Fidget-Spinner, @AlexWaygood
PRs
  • bpo-46571: improve typing.no_type_check to skip foreign objects #31042
  • 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 2022-02-19.01:54:58.041>
    created_at = <Date 2022-01-29.14:12:02.478>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'Strange `@typing.no_type_check` behavior for class variables'
    updated_at = <Date 2022-02-19.01:54:58.040>
    user = 'https://github.com/sobolevn'

    bugs.python.org fields:

    activity = <Date 2022-02-19.01:54:58.040>
    actor = 'JelleZijlstra'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-02-19.01:54:58.041>
    closer = 'JelleZijlstra'
    components = ['Library (Lib)']
    creation = <Date 2022-01-29.14:12:02.478>
    creator = 'sobolevn'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46571
    keywords = ['patch']
    message_count = 14.0
    messages = ['412073', '412074', '412079', '412083', '412130', '412151', '412153', '412158', '412159', '412160', '412162', '412178', '413523', '413525']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'JelleZijlstra', 'sobolevn', 'kj', 'AlexWaygood']
    pr_nums = ['31042']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46571'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @sobolevn
    Copy link
    Member Author

    I was working on improving coverage and test quality of typing.py, when I saw that @no_type_check is quite strange.

    Let's dive into this!

    ## Everything is correct

    We will start with a basic example, that illustrates that everything works fine:

    from typing import no_type_check, get_type_hints
    
    class A:
        x: int = 1
    
    class B:
        y: str = 'a'
    
    print(get_type_hints(A))  # ok: {'x': <class 'int'>}
    print(get_type_hints(B))  # ok: {'y': <class 'str'>}
    

    Ok, let's move on.

    Adding @no_type_check

    Now, adding @no_type_check to B will make the result of get_type_hints() call on it - an empty dict:

    from typing import no_type_check, get_type_hints
    
    class A:
        x: int = 1
    
    @no_type_check
    class B:
        y: str = 'a'
    
    print(get_type_hints(A))  # ok: {'x': <class 'int'>}
    print(get_type_hints(B))  # ok: {}
    

    This is still ok.

    ## Broken?

    And now we can add some class-level constant to B, like delegate to show how it breaks A:

    from typing import no_type_check, get_type_hints
    
    class A:
        x: int = 1
    
    @no_type_check
    class B:
        y: str = 'a'
        delegate = A  # adding this line will make `A` to have `__no_type_check__` as well
    
    print(get_type_hints(A))  # {}, wait, what?
    print(get_type_hints(B))  # {}, ok
    

    Why is that important?

    It introduces an unfortunate side-effect that can make some totally unrelated (!) class completely ignore get_type_hints() and break things like pydantic, beartype, `attrs, etc that rely on type hints.

    By adding a class-level assignment to a class that has @no_type_check or other no_type_check_decorator.

    Why does this happen?

    It happens because no_type_check has this logic:

        if isinstance(arg, type):
            arg_attrs = arg.__dict__.copy()
            for attr, val in arg.__dict__.items():
                if val in arg.__bases__ + (arg,):
                    arg_attrs.pop(attr)
            for obj in arg_attrs.values():
                if isinstance(obj, types.FunctionType):
                    obj.__no_type_check__ = True
                if isinstance(obj, type):
                    no_type_check(obj)
    

    Source:

    cpython/Lib/typing.py

    Lines 1952 to 1975 in 8b1b27f

    def no_type_check(arg):
    """Decorator to indicate that annotations are not type hints.
    The argument must be a class or function; if it is a class, it
    applies recursively to all methods and classes defined in that class
    (but not to methods defined in its superclasses or subclasses).
    This mutates the function(s) or class(es) in place.
    """
    if isinstance(arg, type):
    arg_attrs = arg.__dict__.copy()
    for attr, val in arg.__dict__.items():
    if val in arg.__bases__ + (arg,):
    arg_attrs.pop(attr)
    for obj in arg_attrs.values():
    if isinstance(obj, types.FunctionType):
    obj.__no_type_check__ = True
    if isinstance(obj, type):
    no_type_check(obj)
    try:
    arg.__no_type_check__ = True
    except TypeError: # built-in classes
    pass
    return arg

    As you can see above, we traverse all __dict__ values of the given class and for some reason recurse into all nested types.

    I think that the original goal was to handle cases like:

    @no_type_check
    class Outer:
        class Inner: ...
    

    And now it also affects regular assignments.

    So, what can we do?

    1. Nothing, it works correctly (I disagree)
    2. Do not cover nested classes at all with @no_type_check, only cover methods
    3. Only cover types that are defined in this class, like my Inner class example
    4. Something else?

    I think that (2) is more inline with the currect implementation, so my vote is for it.

    I would like to implement this when we will have the final agreement :)

    @sobolevn sobolevn added 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 labels Jan 29, 2022
    @AlexWaygood
    Copy link
    Member

    ...Option 3). Deprecate @no_type_check?

    Maybe we should gather some stats on how many people are using @no_type_check? My feeling is that it's never achieved widespread adoption, so maybe it's just not that useful a thing to have in the stdlib.

    Anyway, the behaviour you've pointed out is nuts, so I agree that something needs to change here!

    @JelleZijlstra
    Copy link
    Member

    Let's not jump into deprecating it.

    I think (2) makes the most sense. If we can get that working reliably (using __qualname__ I assume), it would be my preference; otherwise we should do (1).

    This problem can also affect function objects, right? class B: meth = some_function.

    @gvanrossum
    Copy link
    Member

    @no_type_check (introduced by PEP-484) is intended for static checkers, to signal to them that they shouldn't check the given function or class.

    I don't think PEP-484 specifies its runtime effect -- arguably get_type_hints() could just ignore it. Or raise an exception when called on such a class. We could even argue that @no_type_check shouldn't have a runtime effect.

    But before we change anything we should be guided by:

    1. What is documented?
    2. What does it do now?
    3. How is that used by runtime type inspectors?
    4. What would be most useful to runtime type inspectors?

    @sobolevn
    Copy link
    Member Author

    ## 1. What is documented?

    The docs makes this even more weird!

    @typing.no_type_check
    Decorator to indicate that annotations are not type hints.
    This works as class or function decorator. With a class, it applies recursively to all methods defined in that class (but not to methods defined in its superclasses or subclasses).
    This mutates the function(s) in place.

    https://docs.python.org/3/library/typing.html#typing.no_type_check

    Docs do not mention modifing nested classes at all! So, it looks like the (1) solution.

    ## 2. What does it do now?

    It modifies nested types, even ones used in assignments.

    ## 3. How is that used by runtime type inspectors?

    I've made a little research:

    1. Hypothesis (I help with maintaining its typing API) does not support @no_type_check at all: Improve error message when st.builds() is missing required uninferrable arguments HypothesisWorks/hypothesis#3225

    2. Pydantic, looks like @no_type_check does not change anything at all.

    from pydantic import BaseModel
    from typing import no_type_check
    
    @no_type_check  # the same with and without this decorator
    class MyModel(BaseModel):
        a: int
    
    print(MyModel(a=1))  # ok
    print(MyModel(a='1a'))  # ok
    # pydantic.error_wrappers.ValidationError: 1 validation error for MyModel
    # a: value is not a valid integer (type=type_error.integer)
    

    So, it always tries to coerce types. Docs: https://pydantic-docs.helpmanual.io/usage/types/

    They also use it inside their own code-base: https://github.com/samuelcolvin/pydantic/search?q=no_type_check Probably for mypy to be happy.

    1. dataclasses and attrs - nothing changes. Both do not use neither @no_type_check nor get_type_hints inside.

    Attrs: https://github.com/python-attrs/attrs/search?q=get_type_hints

    1. Beartype: https://github.com/beartype/beartype

    We've verified that @beartype reduces to the identity decorator when decorating unannotated callables. That's but the tip of the iceberg, though. @beartype unconditionally reduces to a noop when:
    The decorated callable is itself decorated by the PEP-484-compliant @typing.no_type_check decorator.

    So, as far as I understand, they only have @no_type_check support for callables. Related test: https://github.com/beartype/beartype/blob/50b213f315ecf97ea6a42674defe474b8f5d7203/beartype_test/a00_unit/a00_util/func/pep/test_utilpep484func.py

    So, to conclude, some project might still rely on current behavior that nested types are also implicitly marked as @no_type_check, but I cannot find any solid proof for it.

    The noticable thing is that this never came up before in ~6 years while this logic exists: https://github.com/python/cpython/blame/8fb36494501aad5b0c1d34311c9743c60bb9926c/Lib/typing.py#L1969-L1970

    It helps to prove my point: probably, no one uses it.

    ## 3. How is that used by runtime type inspectors?
    ## 4. What would be most useful to runtime type inspectors?

    With all the information I gathered, I've changed my opinion :)
    Now I think that we should drop the part with modifing nested types at all ((1) solution). Only a type with @no_type_check should be modified.

    This is what docs say. This will also solve the original problem.
    Even if someone relies on current behavior: the fix is not hard, just add @no_type_check to nested types as well.

    So, do others have any objections / comments / feedback? :)

    @Fidget-Spinner
    Copy link
    Member

    I agree with Jelle, let's go with (2). It feels strange to have a decorator modify types that it doesn't own.

    @AlexWaygood
    Copy link
    Member

    I fully withdraw my suggestion of deprecating the decorator; it's evidently used more than I realised. I don't have any strong opinion on whether (1) or (2) would be a better solution.

    @sobolevn
    Copy link
    Member Author

    Ken Jin, Jelle, can you please share your ideas why (2) is better than (1)?

    @JelleZijlstra
    Copy link
    Member

    1. Less change in behavior

    2. From the purpose of the decorator, it makes sense for it to apply to nested classes as well as methods, even if the docs don't say so explicitly.

    @gvanrossum
    Copy link
    Member

    I agree that Jelle's proposal (Nikita's #2) looks best *if* we can implement it.

    How do we ensure that

    class A:
      ...
    
    @no_type_check
    class B:
      AA = A
      class C:
        ...
      ...

    suppresses annotations in B and C but not in A?

    @JelleZijlstra
    Copy link
    Member

    I think we could do it by looking at __qualname__.

    @gvanrossum
    Copy link
    Member

    Okay, somebody can submit a PR!

    @JelleZijlstra
    Copy link
    Member

    New changeset 395029b by Nikita Sobolev in branch 'main':
    bpo-46571: improve typing.no_type_check to skip foreign objects (GH-31042)
    395029b

    @JelleZijlstra
    Copy link
    Member

    Thanks for the fix Nikita!

    @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.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

    5 participants