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

Add ability to use Final in a field type annotation #2768

Merged
merged 9 commits into from
Aug 5, 2022

Conversation

uriyyo
Copy link
Contributor

@uriyyo uriyyo commented May 9, 2021

Add ability to use Final in a field type annotation

Implementation is based on a PEP 591:

Type checkers should infer a final attribute that is initialized in a class body as being a class variable.

A final attribute declared in a class body without an initializer must be initialized in the init method

So in this case field name will be treated as a ClassVar:

from typing import Final
from pydantic import BaseModel

class Model(BaseModel):
    name: Final[str] = 'Yurii'

assert 'name' in Model.__class_vars__
assert 'name' not in Model.__fields__

But in a case when a default value is not present than field will be treated as an init-only required field:

from typing import Final
from pydantic import BaseModel

class Model(BaseModel):
    name: Final[str]

assert 'name' not in Model.__class_vars__
assert 'name' in Model.__fields__

_ = Model()  # will fail because name is missed

obj = Model(name='Yurri')
obj.name = 'New Value'  # will fail because final field has been already inited

Related issue number

#2766

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@uriyyo uriyyo marked this pull request as draft May 9, 2021 17:14
@uriyyo
Copy link
Contributor Author

uriyyo commented May 9, 2021

@samuelcolvin @PrettyWood Could you please review this PR?

I want to hear your opinion regarding this feature.

@nuno-andre
Copy link
Contributor

IMHO, this implementation adds an unexpected behavior from the pydanticish perspective (a default value turns what would otherwise be an instance field into a class var) without a clear benefit.

from pydantic import BaseModel

class Model(BaseModel):
    var: Final[str] = Field(max_length=3)

I presume that in a case like this no one would expect to have an immutable class var of the type FieldInfo.

The aim of a final name is not to be reassigned after initialization, but there's no such initialization in a Model declaration. As with dataclass, it's needed to annotate the attribute as a ClassVar to be treated as such and excluded from fields/__fields__.

from pydantic import BaseModel

class A:
    var = 1

class B(BaseModel):
    var = 1
>>> A.var
1
>>> B.var
...
AttributeError: type object 'B' has no attribute 'var'

I think Final could be more useful as an allow_mutation = False synonym that would also prevent the attribute redefinition in subclasses. The problem here would be that Final and ClassVar should not be together...

The behavior of Final for cases such as dataclass or TypedDict is not defined in the PEP. And, in fact, mypy doesn't seem to know how to deal with it.

from dataclasses import dataclass
from typing import Final, TypedDict

@dataclass
class A:
    var: Final[str]
#> error: Final name must be initialized with a value

class B(TypedDict):
    var: Final[str]
#> error: Final can be only used as an outermost qualifier in a variable annotation

# this also raises a TypeError
#> TypeError: typing.Final[str] is not valid as type argument

But mypy's Var AST node has a final_set_in_init attribute that seems a fine approach for Pydantic to mimic in a case like this.

@PrettyWood
Copy link
Member

PrettyWood commented May 9, 2021

Honestly I'm against it. I feel like it's really confusing and the different behaviour with or without a default value goes against pydantic one (required vs optional more or less).
I feel like people should stick with plain ClassVar or directly the type, which is explicit

Edit: I like the "synonym for immutable" (to me this is what seems the most reasonable) hence setting allow_mutation = False behind the scenes but what about class vars if it goes against the PEP?

@uriyyo
Copy link
Contributor Author

uriyyo commented May 9, 2021

@nuno-andre @PrettyWood Thanks for your comments.

@nuno-andre Thanks for pointing out case when the field declared as a Field call. I have updated the implementation and now it will be resolved correctly.

Regarding the mypy. Currently, pydantic mypy plugin handles a lot of cases and for instance, it generates __init__ method with the correct type decl for a model. From my perspective, we can update plugin to handle special cases for Final.

@PrettyWood Currently pydantic has a lot of ways to decl fields:

class Model(BaseModel):
    a: int  # required
    b: int = 10  # optional
    c: int = Field()  # required
    d: int = Field(default=10)  # optional
    e: int = Field(default_factory=lambda: 10)  # optional

To simplify things we can always use Final type annotation as for instance fields.

class Model(BaseModel):
    a: Final[int] = 10 # instance field with default value 10 same as Field(default=10)

Another feature of this PR is that we can have init-only fields.
So we can disable mutation for a specific set of fields.

class Model(BaseModel):
    a: Final[int]
    b: int


obj = Model(a=10, b=20)
obj.a = 100  # will fail because final field can't be changed outside of init method
obj.b = 100  # totally okay

As far as implementation follows PEP 591 it's pretty easy to predict the behavior of this feature.

@PrettyWood
Copy link
Member

@uriyyo My point was just about class var / field. By setting a default value, we change a field in a class var, which is what the PEP says but feels anti-pydantic to me.
The use of a: Final[int] to make a value immutable (since it's only settable once in the init) feels good.
Discard: It's just my opinion ;)

@uriyyo
Copy link
Contributor Author

uriyyo commented May 9, 2021

@PrettyWood Thanks for your opinion, it's really important for me🙂

I totally agree with you that we can use Final to make a value immutable and it will be more explicit rather than have a behavior when Final makes field as a class var.

@uriyyo uriyyo marked this pull request as ready for review June 10, 2021 13:35
@PrettyWood
Copy link
Member

I think we should get more feedback on this before adding anything or reviewing.
@samuelcolvin what's your opinion on the rationale? others?

@uriyyo
Copy link
Contributor Author

uriyyo commented Apr 3, 2022

@PrettyWood @samuelcolvin Any chance to review this one? It would be great to hear your opinion. I would like to use this feature in my projects)

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM.

pydantic/main.py Outdated
@@ -183,12 +184,17 @@ def __new__(mcs, name, bases, namespace, **kwargs): # noqa C901
def is_untouched(v: Any) -> bool:
return isinstance(v, untouched_types) or v.__class__.__name__ == 'cython_function_or_method'

def is_finalvar_with_default_val(type_: Type[Any], val: Any) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved into the module scope of fields.py.

@@ -347,6 +353,10 @@ def __setattr__(self, name, value): # noqa: C901 (ignore complexity)
raise ValueError(f'"{self.__class__.__name__}" object has no field "{name}"')
elif not self.__config__.allow_mutation or self.__config__.frozen:
raise TypeError(f'"{self.__class__.__name__}" is immutable and does not support item assignment')
elif name in self.__fields__ and self.__fields__[name].final:
raise TypeError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -386,10 +387,21 @@ def _check_classvar(v: Optional[Type[Any]]) -> bool:
return v.__class__ == ClassVar.__class__ and getattr(v, '_name', None) == 'ClassVar'


def _check_finalvar(v: Optional[Type[Any]]) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a docstring explaining what this is doing.

assert 'a' not in Model.__class_vars__
assert 'a' in Model.__fields__

assert Model.__fields__['a'].final
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we we add a check somewhere else that final is normally false.

@samuelcolvin
Copy link
Member

please update.

@samuelcolvin
Copy link
Member

you'll also need to rebase/merge master to get tests passing.

@uriyyo
Copy link
Contributor Author

uriyyo commented Aug 5, 2022

please review

@samuelcolvin
Copy link
Member

thanks so much.

@uriyyo uriyyo deleted the fix-issue-2766 branch August 5, 2022 16:42
@bzoracler
Copy link

A PR has just been opened to change the Python typing specifications that will propagate to any use of dataclass-likes in the Python ecosystem, such as with @dataclass_transform.

python/typing#1669

The above typing PR specifies behaviour which is incompatible with the Final = <value> (in Pydantic , treated as a ClassVar in BaseModel subclasses based on a consistent reading of PEP 591) made in this Pydantic PR. The relevant behaviour still exists in Pydantic here:

if _is_finalvar_with_default_val(ann_type, getattr(cls, ann_name, PydanticUndefined)):
class_vars.add(ann_name)
continue

The typing PR will mean that Final = <value> become instance fields with a default value of <value> instead, in dataclass-like class bodies.


As @dataclass_transform was created with Pydantic in mind, I think it'd be valuable if anyone would like to weigh in with explicit support or rejection of this change to the typing spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants