python / cpython

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

Discrepancy between `__args__` of `typing.Callable` and `collections.abc.Callable` #103452

Open AlexWaygood opened 1 year ago

AlexWaygood commented 1 year ago

On both Python 3.11 and main @ b57105ae33e1f61e6bdf0eec45c4135d067b9b22:

>>> import typing, collections.abc
>>> Ts = typing.TypeVarTuple("Ts")
>>> typing.Callable[[*Ts], None].__args__
(*Ts, <class 'NoneType'>)
>>> collections.abc.Callable[[*Ts], None].__args__
(*Ts, None)

According to this assertion in the test suite for typing.py, the behaviour of typing.Callable is correct here, whereas the behaviour of collections.abc.Callable is incorrect: https://github.com/python/cpython/blob/b57105ae33e1f61e6bdf0eec45c4135d067b9b22/Lib/test/test_typing.py#L1290-L1292

However, the test is currently only run with typing.Callable, whereas it should be run with collections.abc.Callable as well.

Cc. @mrahtz, who wrote this test. This may also be of interest to @sobolevn :)

sobolevn commented 1 year ago

Oh no 🔥 🚒 😆

sobolevn commented 1 year ago

This is not about Callable, the same happens with other types.GenericAlias objects:

>>> import typing

>>> list[None].__args__
(None,)
>>> typing.List[None].__args__
(<class 'NoneType'>,)

>>> set[None]
set[None]
>>> typing.Set[None]
typing.Set[NoneType]
Eclips4 commented 1 year ago

Problem is here: https://github.com/python/cpython/blob/96663875b2ea55c65e83551cdb741bbcdcaa7f21/Lib/typing.py#L160-L166

Eclips4 commented 1 year ago

So, we can roughly delete this two lines from _type_convert and some other places in typing.py. Of course, test_typing.py need some fixes (replace in many places type(None) to None). But, let's see what others say. With this fixes, example from Alex works fine:

>>> import typing, collections.abc
>>> Ts = typing.TypeVarTuple("Ts")
>>> typing.Callable[[*Ts], None].__args__
(*Ts, None)
>>> collections.abc.Callable[[*Ts], None].__args__
(*Ts, None)
sobolevn commented 1 year ago

No, it is not a problem :)

The problem is in types.GenericAlias, but we are not sure (yet) that it is a bug. typing has this behaviour for a long time.

AlexWaygood commented 1 year ago

@sobolevn is correct. The objects in typing.py are much older than types.GenericAlias, so if we need to change one of the two, it should be types.GenericAlias rather than the objects in typing.py.

But the fact that all types.GenericAlias objects behave the same way here makes me think that this was probably a deliberate decision.

sunmy2019 commented 1 year ago

The behavior is specified here:

https://peps.python.org/pep-0484/#using-none

When used in a type hint, the expression None is considered equivalent to type(None).

https://peps.python.org/pep-0483/#pragmatics

Where a type is expected, None can be substituted for type(None); e.g. Union[t1, None] == Union[t1, type(None)].

https://peps.python.org/pep-0483/#fundamental-building-blocks

Optional[t1]. Alias for Union[t1, None], i.e. Union[t1, type(None)].

But I do not find any wording about None in the PEP of Type Hinting Generics.

This sounds like a bug to me. I think the correct behavior here is to make

collections.abc.Callable[[*Ts], None] == collections.abc.Callable[[*Ts], type(None)]

evaluates to True, no matter how the underlying works.

Perhaps a PEP is needed to resolve this?

mrahtz commented 1 year ago

Fwiw as the one who wrote the original test, I don't have a strong opinion about this - I don't think there's anything about the way that TypeVarTuples interacts with Callable which makes it matter whether it's None vs NoneType.

My personal vote is that it should be None, because it seems simpler and more intuitive: if you put Foo in the args to anything, including Callable, you should expect to see Foo in the __args__. In everyday life I just use None everywhere and it works fine, so I'd expect it to just be fine here too with no complications.

I do agree with @sunmy2019 that it seems correct for this to be the case:

collections.abc.Callable[[*Ts], None] == collections.abc.Callable[[*Ts], type(None)]

But I'd expect that to be true because None == type(None); I'm a bit surprised to find that it isn't:

>>> None is type(None)
False
>>> None == type(None)
False

This seems super weird to me considering that those PEPs @sunmy2019 is quoting explicitly says they should be equivalent. How difficult would this be to change?

sobolevn commented 1 year ago

It is not hard at all, but this is breaking change. We have types.GenericAlias with this behavior since 3.9:

Python 3.9.15 (main, Nov 22 2022, 17:18:20) 
[Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> list[None].__args__[0]
>>> import typing
>>> typing.List[None].__args__[0]
<class 'NoneType'>

I am pretty sure many libraries rely on this. I've tried pydantic:

>>> from pydantic import BaseModel
>>> class My(BaseModel):
...    x: list[None]
... 
>>> import types
>>> class Other(BaseModel):
...    y: list[types.NoneType]
... 
>>> My(x=[None])
My(x=[None])
>>> My(x=[type(None)])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for My
x -> 0
  value is not None (type=type_error.not_none)
>>> Other(y=[None])
Other(y=[None])
>>> Other(y=[type(None)])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Other
y -> 0
  value is not None (type=type_error.not_none)

Looks like this exact lib handles list[type(None)] correctly.

sunmy2019 commented 1 year ago

Looks like this exact lib handles list[type(None)] correctly.

Found this in their code

    elif obj is None or obj is _typing_extra.NoneType:
        return core_schema.NoneSchema(type='none')

https://github.com/pydantic/pydantic/blob/c443799023a205a605d747be212a3c3618e6c494/pydantic/_internal/_generate_schema.py#L332-L333

sunmy2019 commented 1 year ago

This seems super weird to me, considering that those PEPs @sunmy2019 is quoting explicitly says they should be equivalent. How difficult would this be to change?

type(None) should not is None, otherwise None will be callable, causing many disasters!

I am a little bit in favor of NoneType since it's a type hint, and None is not a type. Meanwhile, I even did not realize why I can write None for a type before I saw this issue. And it looks like many linters or formatters have adopted this discrepancy. (of course, otherwise, users will complain)

So, I think either way is acceptable.

hmc-cs-mdrissi commented 1 year ago

Funny timing as I recently had some runtime type manipulation code run into this None vs NoneType with annotations past week. I'm fairly neutral as to whether get_type_hints converts None -> NoneType or not. It is one special case rules that's very easy to miss and debug much later. I suspect a fair number of runtime type libraries have some special case for NoneType (or should).

I do not want to backport any change related to this though. If 3.12/13 wants to change None/NoneType special casing sure. I don't want to worry about sys.version on patch version and which behavior is right feels very unclear to me.

eltoder commented 1 year ago

This seems like an obvious omission/oversight in types.GenericAlias not unlike https://github.com/python/cpython/issues/101859. Generic arguments are supposed to be types and None is supposed to be a shortcut to write type(None). At least when using public API like typing.get_args() we should get NoneType out, otherwise people are forced to handle None as a special case everywhere. Currently this is not the case:

>>> typing.get_args(dict[str, None])
(<class 'str'>, None)

Moreover, the equality is broken as well

>>> dict[str, None] == dict[str, type(None)]
False
Prometheus3375 commented 10 months ago

I agree that types.GenericAlias should be changed to convert None to NoneType. I do not think there should be PEP for this change, because, in my opinion, this behavior was introduced due to an oversight, rather than being intended; thus, it is just a bug.

It is usually expected to find NoneType inside __args__. Prior 3.9 there were few places where None could be met in a "wild": annotation which consists only from None or inside args of Literal[None, ...]. The former is resolved by typing.get_type_hints converting None to NoneType, the latter makes total sense as it is Literal.

Version 3.9 introduced types.GenericAlias used by all of built-in generic classes. This is really weird that instances of types.GenericAlias do not convert None to NoneType unlike any instance of typing._BaseGenericAlias. Moreover, types.UnionType introduced in 3.10 actually does it.

This behavior of types.GenericAlias causes issues with some libraries which uses annotations. For example, prior to my report, dataclasses-avroschema was throwing error upon serialization of a dataclass which has dict[str, None] as a field. This was fixed by specifically converting None to NoneType.


However, not only types.GenericAlias does not convert None to NoneType. For example, inspect.signature also does not do this, i.e., inspect.signature(foo_with_no_return).return_annotation returns None for a function with no return. To be fair, inspect.signature does not resolve annotations via strings unless kwarg eval_str is set to True.

The same goes for dataclasses: dataclasses.Field does not convert None to NoneType and does not resolve annotations via strings.

So even if types.GenericAlias will be changed, any library which works with dataclasses or ispect.signature still should support both None and NoneType. Given that, the point of the change to types.GenericAlias is to make its behavior consistent with typing._BaseGenericAlias.


If there is an intent to unify the behavior of converting None to NoneType across all python modules which somehow work with annotations OR there is an intent to remove such conversion, then there definitely should be a PEP about this.

JelleZijlstra commented 10 months ago

I feel strongly that we should not make types.GenericAlias do any conversions like turning None into NoneType. The standard library's introspection APIs should generally return exactly what the user wrote, and leave it to third-party tools to do any further processing. That keeps the standard library simple, and leaves things more flexible for the external tools. If we change something, we throw away information, as tools can no longer tell whether the user wrote X[None] or X[type(None)].

I know we do processing like None -> NoneType in a few places in typing.py, but in my view that has been a mistake that we should not repeat in any other APIs. If possible, we should get rid of such processing, but backwards compatibility makes that difficult.

AlexWaygood commented 10 months ago

I concur with @JelleZijlstra. The discrepancy here is unfortunate, but the behaviour of types.GenericAlias is probably preferable here, really, despite the behaviour of typing._GenericAlias being more longstanding.

I'm not sure what, if anything, is really to be done here.