Deploying pydantic-docs with    Cloudflare Pages

View logs

CodSpeed Performance Report

Merging #10911 will not alter performance

_{Comparing fix-type-checking-issue-model-fields (60522af) with main (b985365)}

Summary

✅ 46 untouched benchmarks

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
main.py
Project Total

This report was generated by python-coverage-comment-action

We are hitting undefined behavior between pyright and mypy here, when a property is defined both on the metaclass and the class itself. I think it's a bit unfortunate to have model_fields typed as a class variable, because it won't emit deprecation warnings here. Here is an alternative approach:

• Until V3, we use the following descriptor defined on BaseModel (not the metaclass):

Code sample in pyright playground

import warnings
from typing import Callable, Generic, TypeVar, overload

from typing_extensions import deprecated

from pydantic.warnings import PydanticDeprecatedSince211

ModelT = TypeVar('ModelT', bound='BaseModel')
R = TypeVar('R')

class classonlyproperty(Generic[ModelT, R]):
    def __init__(self, fget: Callable[[type[ModelT]], R], /) -> None:
        self.fget = fget

    @overload
    def __get__(self, instance: None, owner: type[ModelT]) -> R: ...
    @overload
    @deprecated(
        "Accessing this attribute on the instance is deprecated, and will be removed in Pydantic V3.",
        category=None,
    )
    def __get__(self, instance: ModelT, owner: type[ModelT]) -> R: ...
    def __get__(self, instance: ModelT | None, owner: type[ModelT]) -> R:
        if instance is not None:
            warnings.warn(
                "Accessing this attribute on the instance is deprecated, and will be removed in Pydantic V3.",
                category=PydanticDeprecatedSince211,
            )
        return self.fget(owner)


class BaseModel:
    @classonlyproperty
    @classmethod
    def model_fields(cls) -> dict[str, str]:
        ...


BaseModel.model_fields
BaseModel().model_fields  # type checker error

pyright will emit an error, and mypy will probably be able to do so soon implements it in 1.14.

• In V3, we remove the classonlyproperty helper and the model_fields from BaseModel to move it back to the model metaclass, as it is currently.

WDYT?

@Viicos,

Interesting idea. 2 reservations I have about this:

• The reason we included the model_fields property on BaseModel in the first place was for backwards compatibility - folks sometimes access model_fields on a BaseModel instance. Thus, I think decorating with the classonlyproperty is a bit misleading.

I sort of poorly explained this in a comment:

# TODO: V3 - remove `model_fields` and `model_computed_fields` properties from the `BaseModel` class - they should only
# be accessible on the model class, not on instances. We have these purely for backwards compatibility with Pydantic <v2.10.

• Class properties aren't supported by Python, so I think this might be doing a bit more type hacking / jumping through hoops than we really should be doing?

• Thus, I think decorating with the classonlyproperty is a bit misleading.

The naming of the decorator can be improved, but with this approach, accessing the property does work at runtime. However, it will emit a runtime deprecation warning and a type checker error/warning

• Class properties aren't supported by Python, so I think this might be doing a bit more type hacking / jumping through hoops than we really should be doing?

Well the usage of the descriptor can feel a bit too much for such a small use case, but descriptor support is implemented by all major type checkers and this seems to be the only viable approach to emit a deprecation warning. I'm a bit worried that in V3 this will come as a surprise, because we currently don't emit anything (as an example, SQLModel relies on this)

Yeah, fair point.

What if:

• For v2.10, we go with the approach on this PR
• For v2.11, we go with your approach (I don't want to introduce a deprecation warning in a patch release 😓)

Sounds good, could you please add some tests in the typechecking folder? Probably in base_model.py.

Oh so odd, I think I have them locally but didn't push...