Update to pyright==1.1.345

[Viicos]

Required by #8237

Most of the changes are introduced by the new standard type checking mode.

One failure remains on:

pydantic/pydantic/type_adapter.py

Lines 109 to 153 in da468c4

	class TypeAdapter(Generic[T]):
	"""Type adapters provide a flexible way to perform validation and serialization based on a Python type.
	A `TypeAdapter` instance exposes some of the functionality from `BaseModel` instance methods
	for types that do not have such methods (such as dataclasses, primitive types, and more).
	**Note:** `TypeAdapter` instances are not types, and cannot be used as type annotations for fields.
	Attributes:
	core_schema: The core schema for the type.
	validator (SchemaValidator): The schema validator for the type.
	serializer: The schema serializer for the type.
	"""
	if TYPE_CHECKING:
	@overload
	def __new__(cls, __type: type[T], *, config: ConfigDict | None = ...) -> TypeAdapter[T]:
	...
	# this overload is for non-type things like Union[int, str]
	# Pyright currently handles this "correctly", but MyPy understands this as TypeAdapter[object]
	# so an explicit type cast is needed
	@overload
	def __new__(cls, __type: T, *, config: ConfigDict | None = ...) -> TypeAdapter[T]:
	...
	def __new__(cls, __type: Any, *, config: ConfigDict | None = None) -> TypeAdapter[T]:
	"""A class representing the type adapter."""
	raise NotImplementedError
	@overload
	def __init__(
	self, type: type[T], *, config: ConfigDict | None = None, _parent_depth: int = 2, module: str | None = None
	) -> None:
	...
	# this overload is for non-type things like Union[int, str]
	# Pyright currently handles this "correctly", but MyPy understands this as TypeAdapter[object]
	# so an explicit type cast is needed
	@overload
	def __init__(
	self, type: T, *, config: ConfigDict | None = None, _parent_depth: int = 2, module: str | None = None
	) -> None:
	...

As described by the comment, pyright handles it currently and doesn't need the extra overload, hence the emitted diagnostic:

error: Overload 2 for "__new__" will never be used because its parameters overlap overload 1 (reportOverlappingOverload)

This can be fixed by adding an if MYPY block, but first I think we can remove one of the overload set (either __init__ or __new__), I don't know if it makes much sense to have both defined (apart from IDE support, but in that case we can keep only the __init__ overload set defined).

Edit: went for __init__ and special cased mypy.

Selected Reviewer: @hramezani

[codspeed-hq]

CodSpeed Performance Report

Merging #8453 will not alter performance

_{Comparing Viicos:update-pyright-343 (bc9fe04) with main (da468c4)}

Summary

✅ 10 untouched benchmarks

[Viicos]

This can be fixed by adding an if MYPY block, but first I think we can remove one of the overload set (either __init__ or __new__), I don't know if it makes much sense to have both defined (apart from IDE support, but in that case we can keep only the __init__ overload set defined).

Edit: went for __init__ and special cased mypy.

Actually, an if MYPY block isn't special cased by pyright (a workaround is available here, but does not apply to library code).

I think the best thing would be to remove mypy support here. The following comment:

pydantic/pydantic/type_adapter.py

Lines 129 to 133 in 3f84e44

	# this overload is for non-type things like Union[int, str]
	# Pyright currently handles this "correctly", but MyPy understands this as TypeAdapter[object]
	# so an explicit type cast is needed
	@overload
	def __new__(cls, __type: T, *, config: ConfigDict | None = ...) -> TypeAdapter[T]:

seems outdated/incorrect, as the second overload also makes mypy understand it as TypeAdapter[object] (see playground).

Without the second overload (so only type[T] for the type argument), mypy raises a weird issue (playground).

I wonder if this could be clarified in the typing spec, as Union is a special form meant to be used in annotations, not as values (see also python/mypy#9773).

In the meanwhile, the last commit pushed removed support for mypy.

[Viicos]

Please review

[sydney-runkle]

Looks good overall, thank you!

Going to consult with the team re your questions for TypeAdapter

[sydney-runkle]

What's the problem here / what error is raised?

[Viicos]

Method "model_construct" overrides class "BaseModel" in an incompatible manner, due to the fact that reportIncompatibleMethodOverride was recently reported by default to be compliant with the typing spec.

[adriangb]

Don't we basically loose all typing for TypeAdapter(int) by doing this?

[adriangb]

Does this work now? What about passing in int | str with mypy as per comment above?

[sydney-runkle]

Following up, this doesn't work:

from pydantic import TypeAdapter

from typing import Union

adapter: TypeAdapter[Union[int, str]] = TypeAdapter(Union[int, str])

-->

test.py:5: error: Argument 1 to "TypeAdapter" has incompatible type "object"; expected "Union[Type[int], Type[str]]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

[sydney-runkle]

@Viicos,

Is there any downside of leaving the above overloads within the if TYPE_CHECKING block? I think it'd be preferred to leave those in place.

[Viicos]

Regarding TypeAdapter:

The current implementation is making use of __new__ and __init__ at the same time, which is currently unspecified in the typing spec and leads to different behaviors between pyright and mypy. Afaik pyright will make use of the __new__ return type annotation but will try to use the __init__ signature regarding arguments. mypy seems to discards the __new__ method in our case. The __new__ method also seems to be missing some arguments with respect to __init__.

Now regarding the overload for the type argument, the following comment:

pydantic/pydantic/type_adapter.py

Lines 129 to 133 in 54a1576

	# this overload is for non-type things like Union[int, str]
	# Pyright currently handles this "correctly", but MyPy understands this as TypeAdapter[object]
	# so an explicit type cast is needed
	@overload
	def __new__(cls, __type: T, *, config: ConfigDict | None = ...) -> TypeAdapter[T]:

seems incorrect: according to this comment, having only the first overload (with type: type[T]) will make mypy infer TypeAdapter(Union[int, str]) as TypeAdapter[object]. In reality, it doesn't infer anything, and throws a weird error (playground).

Having the second overload added is what actually makes mypy infer TypeAdapter(Union[str, int]) as TypeAdapter[object], which isn't ideal but still better than the previous error (playground). This also means that:

Following up, this doesn't work:

from pydantic import TypeAdapter

from typing import Union

adapter: TypeAdapter[Union[int, str]] = TypeAdapter(Union[int, str])

in fact never worked, even with the current implementation (in the last linked playground, you can try explicitly annotating a to TypeAdapter[Union[int, str]], you will get the same error).

However, I think the best solution would be:
I add back the overload with type: T, and ignore the pyright overlap issue. That way people can still do the following in mypy:

adapter: TypeAdapter[Union[int, str]] = TypeAdapter(Union[int, str])  # type: ignore[arg-type]
reveal_type(adapter)  # Revealed type is "__main__.TypeAdapter[Union[builtins.str, builtins.int]]"

---

Don't we basically loose all typing for TypeAdapter(int) by doing this?

There is actually no need to add __new__ overloads for our use case, type checkers will infer the type variable from the provided argument at instantiation. Not making use of __new__ also allows subclassing TypeAdapter and having the correct class inferred. You can see how it plays in pyright and mypy.

So long story short I think I just need to add an extra __init__ overload and it should be alright?

[adriangb]

Could you make a matrix of what does and doesn’t work with type adapter before and after this PR? We need to look at type checke and type/union.

[Viicos]

This is the current state with the last changes pushed:

Use case	Before	After	Notes
reveal_type(TypeAdapter(int))	mypy: TypeAdapter[int] ✅ pyright: TypeAdapter[int] ✅	mypy : TypeAdapter[int] ✅ pyright : TypeAdapter[int] ✅
reveal_type(TypeAdapter(Union[str, int]))	mypy: TypeAdapter[object] ❌ pyright: TypeAdapter[str | int] ✅	mypy : TypeAdapter[object] ❌ pyright : TypeAdapter[str | int] ✅	Explicitly annotating to TypeAdapter[Union[str, int]] can be used as a workaround for mypy, but you will need to add a # type: ignore comment
class Sub(TypeAdapter[T]): pass; reveal_type(Sub(int))	mypy: Sub[int] ✅ pyright: TypeAdapter[int] ❌	mypy : Sub[int] ✅ pyright : Sub[int] ✅

I've updated the docstring as well

[adriangb]

It looks like the only thing that is improved is the last use case, which imo we don’t want to accidentally support, so I would suggest making the class final if we move forward with this change.

[Viicos]

It looks like the only thing that is improved is the last use case

From a user perspective yes, and on the other hand this cleans up the usage of __new__ and __init__ which wasn't necessary.

which imo we don’t want to accidentally support, so I would suggest making the class final if we move forward with this change.

What are your concerns on this use case? I think people might want to create subclasses to change some method signature defaults for example.

A quick search shows only one result though, so annotating it as final probably won't break a lot of existing code

[adriangb]

Yes I’m not saying it’s a bad change if it doesn’t deteriorate an existing use case. I do want to be clear about what the advantages are.

After working on a project as widely used as Pydantic you realize that if someone can they will, and you end up having to support use cases you never imagined in the name of backwards compatibility. So for now I’d mark it as final since it wasn’t really safely subclassable before.

[Viicos]

After working on a project as widely used as Pydantic you realize that if someone can they will, and you end up having to support use cases you never imagined in the name of backwards compatibility. So for now I’d mark it as final since it wasn’t really safely subclassable before.

That's understandable! Anyway this will only be enforced at the typing level, and I feel it's a good way to "warn" users they are using an unsupported feature, and if they want to proceed they'll need to add a type: ignore comment

[sydney-runkle]

@Viicos, LGTM once you fix the ruff issues causing the tests to fail.

[Viicos]

LGTM once you fix the ruff issues causing the tests to fail.

Yeah I'm not sure what's happening here, I've added a test="skip" directive but it seems to be tested still. I'll fix the example, might be easier

[sydney-runkle]

🚀