Issue46571
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2022-01-29 14:12 by sobolevn, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 31042 | merged | sobolevn, 2022-02-01 08:29 |
Messages (14) | |||
---|---|---|---|
msg412073 - (view) | Author: Nikita Sobolev (sobolevn) * | Date: 2022-01-29 14:12 | |
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: https://github.com/python/cpython/blob/8b1b27f1939cc4060531d198fdb09242f247ca7c/Lib/typing.py#L1952-L1975 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? 0. Nothing, it works correctly (I disagree) 1. Do not cover nested classes at all with `@no_type_check`, only cover methods 2. Only cover types that are **defined** in this class, like my `Inner` class example 3. 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 :) |
|||
msg412074 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-29 14:24 | |
...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! |
|||
msg412079 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-01-29 15:49 | |
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`. |
|||
msg412083 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-29 16:53 | |
@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? |
|||
msg412130 - (view) | Author: Nikita Sobolev (sobolevn) * | Date: 2022-01-30 07:31 | |
## 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: https://github.com/HypothesisWorks/hypothesis/issues/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. 3. `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 4. 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? :) |
|||
msg412151 - (view) | Author: Ken Jin (kj) * | Date: 2022-01-30 14:49 | |
I agree with Jelle, let's go with (2). It feels strange to have a decorator modify types that it doesn't own. |
|||
msg412153 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-30 14:55 | |
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. |
|||
msg412158 - (view) | Author: Nikita Sobolev (sobolevn) * | Date: 2022-01-30 16:06 | |
Ken Jin, Jelle, can you please share your ideas why `(2)` is better than `(1)`? |
|||
msg412159 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-01-30 16:21 | |
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. |
|||
msg412160 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-30 16:28 | |
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? |
|||
msg412162 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-01-30 16:47 | |
I think we could do it by looking at __qualname__. |
|||
msg412178 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-30 22:59 | |
Okay, somebody can submit a PR! |
|||
msg413523 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-02-19 01:53 | |
New changeset 395029b0bd343648b4da8044c7509672ea768775 by Nikita Sobolev in branch 'main': bpo-46571: improve `typing.no_type_check` to skip foreign objects (GH-31042) https://github.com/python/cpython/commit/395029b0bd343648b4da8044c7509672ea768775 |
|||
msg413525 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-02-19 01:54 | |
Thanks for the fix Nikita! |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:55 | admin | set | github: 90729 |
2022-02-19 01:54:58 | JelleZijlstra | set | status: open -> closed resolution: fixed messages: + msg413525 stage: patch review -> resolved |
2022-02-19 01:53:43 | JelleZijlstra | set | messages: + msg413523 |
2022-02-01 08:29:29 | sobolevn | set | keywords:
+ patch stage: patch review pull_requests: + pull_request29224 |
2022-01-30 22:59:20 | gvanrossum | set | messages: + msg412178 |
2022-01-30 16:47:50 | JelleZijlstra | set | messages: + msg412162 |
2022-01-30 16:28:29 | gvanrossum | set | messages: + msg412160 |
2022-01-30 16:21:05 | JelleZijlstra | set | messages: + msg412159 |
2022-01-30 16:06:44 | sobolevn | set | messages: + msg412158 |
2022-01-30 14:55:30 | AlexWaygood | set | messages: + msg412153 |
2022-01-30 14:49:02 | kj | set | messages: + msg412151 |
2022-01-30 07:31:26 | sobolevn | set | messages: + msg412130 |
2022-01-29 16:53:37 | gvanrossum | set | messages: + msg412083 |
2022-01-29 15:49:46 | JelleZijlstra | set | messages: + msg412079 |
2022-01-29 14:24:02 | AlexWaygood | set | messages: + msg412074 |
2022-01-29 14:12:12 | sobolevn | set | type: behavior |
2022-01-29 14:12:02 | sobolevn | create |