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.

classification
Title: Accept Final as indicating ClassVar for dataclass
Type: behavior Stage:
Components: Library (Lib) Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: GBeauregard, carljm, eric.smith, med2277, msully4321, saaketp
Priority: normal Keywords:

Created on 2021-10-05 23:00 by GBeauregard, last changed 2022-04-11 14:59 by admin.

Messages (12)
msg403277 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-05 23:00
PEP 591 for the Final Attribute states "Type checkers should infer a final attribute that is initialized in a class body as being a class variable. Variables should not be annotated with both ClassVar and Final."

This is a bit of a typing conflict for dataclasses, where ClassVar is used to indicate a desired library behavior, but one may want to indicate Final.

I propose accepting the Final attribute as an indicator of a ClassVar in dataclasses class bodies in order to be better compatible with the Final PEP.

There is at least one edge case that would need to be handled where someone might want to explicitly mark a dataclass field Final, which could be allowed as a field:
a: Final[int] = dataclasses.field(init=False, default=10)
msg403552 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-09 20:00
Hi Eric,

I've been shopping this idea around on the mailing list and haven't received any objections. Do you have any concerns? Can we handle Final with the same checks as ClassVar?

Regarding potentially merging a change, I'm not sure where this falls between a bug fix and a feature. On one hand the PEP 591 instruction to typecheckers on how to treat Final is relatively absolute and the dataclasses behavior could be considered a buggy interaction with it. On the other hand this is sorta a new behavior. Do you have any opinions? Should I not worry about it when working on patch and let core devs figure out if it would need backported?

I've read through the dataclasses code and I think I can implement this myself and submit a PR, but I may need a bit of a heavy handed code review since I've only recently started getting serious with Python.

Thanks for your time and work on dataclassess.
msg403553 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2021-10-09 20:40
I was waiting for someone smarter than me to chime in on one of the discussions.

I wouldn't worry about whether it's a bug or feature, at this point. Assuming buy-in from type checkers, I'd probably call it a bug, but I can be reasoned with.

One thing I don't understand is what you mean by:

"""
There is at least one edge case that would need to be handled where someone might want to explicitly mark a dataclass field Final, which could be allowed as a field:
a: Final[int] = dataclasses.field(init=False, default=10)
"""

I assume we'd want to treat it like a ClassVar, whatever that does. What's your thought? Are you saying ClassVar works incorrectly in this instance.
msg403555 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-09 21:48
When I originally submitted the issue I hadn't finished going through all of the dataclasses code and it hadn't even occurred to me that it could be valid to use ClassVar with field(). I (wrongly) assumed this would always raise and that field() is only valid for things intended to be instance vars.

I do find this behavior a little surprising, but on reflection I don't think it's explicitly wrong as long we raise for default_factory like it currently does. I think it's then appropriate to just do the exact same behavior for Final as ClassVar.

I'm going to start working on a PR, thanks for your feedback.
msg403560 - (view) Author: Saaket Prakash (saaketp) Date: 2021-10-10 00:02
Treating Final as ClassVar by default may be fine,
but it should not throw when using default_factory like ClassVar does.

There are valid uses of Final with instance variable when one would want the value to be unchanged after the `__init__` runs
but different instances can be initialized with different values that are generated by a default_factory.

A quick search on github for this pattern gives this
https://github.com/166MMX/hiro-python-library/blob/fb29e3247a8fe1b0f7dc4e68141cf7340a8dd0a5/src/arago/hiro/model/ws.py#L120
which will break if Final throws when using default_factory.

PEP 591 says:
Type checkers should infer a final attribute _that is initialized in a class body_ as being a class variable.
When using default_factory the attribute is not initialized inside the class body but when the instance is initialized.
So allowing instance level Final with default_factory will not be going against the PEP.
msg403565 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-10 00:38
Yeah, I was just discussing this with someone in IRC, and I appreciate an example of it in the wild.

I agree there's some wiggle room with what "initialized in a class body" means when it comes to dataclasses.

I see several interpretations there, and I would strongly prefer feedback from typing folks, particularly since they would be responsible for implementing any Final default_factory exceptions.

On the implementation side this does complicate things a bit depending on specifics. Are Final default_factory fields real fields or pseudo-fields? (i.e. are they returned by dataclasses.fields()?) Depending on how this works dataclasses might need a bit more refactoring than I'd be the right person for, but I'm still willing to give it a shot.
msg403615 - (view) Author: Carl Meyer (carljm) * Date: 2021-10-11 04:19
> Are Final default_factory fields real fields or pseudo-fields? (i.e. are they returned by dataclasses.fields()?)

They are real fields, returned by `dataclasses.fields()`.

In my opinion, the behavior change proposed in this bug is a bad idea all around, and should not be made, and the inconsistency with PEP 591 should rather be resolved by explicitly specifying the interaction with dataclasses in a modification to the PEP.

Currently the meaning of:

```
@dataclass
class C:
    x: Final[int] = 3
```

is well-defined, intuitive, and implemented consistently both in the runtime and in type checkers. It specifies a dataclass field of type `int`, with a default value of `3` for new instances, which can be overridden with an init arg, but cannot be modified (per type checker; runtime doesn't enforce Final) after the instance is initialized.

Changing the meaning of the above code to be "a dataclass with no fields, but one final class attribute of value 3" is a backwards-incompatible change to a less useful and less intuitive behavior.

I argue the current behavior is intuitive because in general the type annotation on a dataclass attribute applies to the eventual instance attribute, not to the immediate RHS -- this is made very clear by the fact that typecheckers happily accept `x: int = dataclasses.field(...)` which in a non-dataclass context would be a type error. Therefore the Final should similarly be taken to apply to the eventual instance attribute, not to the immediate assignment. And therefore it should not (in the case of dataclasses) imply ClassVar.

I realize that this means that if we want to allow final class attributes on dataclasses, it would require wrapping an explicit ClassVar around Final, which violates the current text of PEP 591. I would suggest this is simply because that PEP did not consider the specific case of dataclasses, and the PEP should be amended to carve out dataclasses specifically.
msg403616 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-11 04:48
Thanks for the feedback Carl.

Your proposed nesting PEP change is not possible: ClassVar[Final[int]] isn't valid because Final[int] isn't a type. As far as I know this would need type intersections to be possible.

I'm going to try sending a one-off email to the PEP authors since probably whatever happens the PEP needs a clarification anyway.
msg403617 - (view) Author: Carl Meyer (carljm) * Date: 2021-10-11 05:06
Good idea to check with the PEP authors. 

I don’t think allowing both ClassVar and Final in dataclasses requires general intersection types. Neither ClassVar nor Final are real types; they aren’t part of the type of the value.  They are more like special annotations on a name, which are wrapped around a type as syntactic convenience. You’re right that it would require more than just amendment to the PEP text, though; it might require changes to type checkers, and it would also require changes to the runtime behavior of the `typing` module to special-case allowing `ClassVar[Final[…]]`. And the downside of this change is that it couldn’t be context sensitive to only be allowed in dataclasses. But I think this isn’t a big problem; type checkers could still error on that wrapping in non dataclass contexts if they want to. 

But even if that change can’t be made, I think backwards compatibility still precludes changing the interpretation of `x: Final[int] = 3` on a dataclass, and it is more valuable to be able to specify Final instance attributes (fields) than final class attributes on dataclasses.
msg403890 - (view) Author: Michael Sullivan (msully4321) Date: 2021-10-14 07:55
I tend to agree with Carl that making Final imply ClassVar for dataclass would make things worse.

For better or worse (mostly better, but there are downsides like this!), dataclass class bodies are essentially written in their own domain specific language, and allowances need to be made in how typing things are interpreted in such a case, and I think that Carl is right that the right interpretation in the case of dataclasses is that the annotation describes the eventual instance attribute.

At least, I feel this way 80% of the time. I can definitely see the argument for wanting consistency in the interpretation.

From a more directly practical perspective, though, the change would also be a breaking one (though /arguably/ only of broken code), and so maybe not worth it even if we would prefer the changed behavior.

I think the right approach is probably to just append the PEP and then maybe also support ClassVar[Final[Whatever]]. It shouldn't need intersection types or anything; if it's a pain to implement it would be for annoying reasons and not deep ones.
msg403931 - (view) Author: Gregory Beauregard (GBeauregard) * Date: 2021-10-14 18:36
Hi Michael,

Thanks for taking the time to look into this.

I don't feel strongly enough about following the existing PEP wording to desire creating a situation where instance vars can't be marked Final if we can instead make a workaround with ClassVar[Final[...]]. Plus, I can appreciate the argument that dataclasses are their own thing that should be treated specially in the PEP.

If you know what's involved in formally proposing and enacting a PEP amendment I can get behind that.
msg411067 - (view) Author: Mehdi2277 (med2277) Date: 2022-01-21 01:21
I recently hit this issue working on a config/parsing runtime type checking library (similar in spirit to pydantic).

The one other special typeform I was using these with that led me to discover this issue was Annotated. I use Annotated a fair amount to do some runtime analysis and I was used to `Annotated[typeform]` always works. But ClassVar and Final are special and `Annotated[ClassVar[...]] `and `Annotated[Final[...]]` both fail. I find `Annotated` interaction also weird. I ended up working around it by doing `ClassVar[Annotated[...]]` and stripping the classvar/final to look for the annotation metadata.

I think all 3 of annotated/final/classvar should be order compatible as they all serve to add information on the type they contain. 

If we ignore Annotated, I would say ClassVar/Final should be order compatible and a rule that Final[ClassVar[...]] works but not ClassVar[Final[...]] or vice versa would be weird.
History
Date User Action Args
2022-04-11 14:59:50adminsetgithub: 89547
2022-01-21 01:21:16med2277setnosy: + med2277
messages: + msg411067
2021-10-14 18:36:38GBeauregardsetmessages: + msg403931
2021-10-14 07:55:21msully4321setnosy: + msully4321
messages: + msg403890
2021-10-11 05:06:23carljmsetmessages: + msg403617
2021-10-11 04:48:31GBeauregardsetmessages: + msg403616
2021-10-11 04:19:28carljmsetnosy: + carljm
messages: + msg403615
2021-10-10 00:38:15GBeauregardsetmessages: + msg403565
2021-10-10 00:02:51saaketpsetnosy: + saaketp
messages: + msg403560
2021-10-09 21:48:46GBeauregardsetmessages: + msg403555
2021-10-09 20:40:31eric.smithsetmessages: + msg403553
2021-10-09 20:00:16GBeauregardsetmessages: + msg403552
2021-10-05 23:04:21eric.smithsetnosy: + eric.smith
2021-10-05 23:02:27GBeauregardsettype: enhancement -> behavior
2021-10-05 23:00:02GBeauregardcreate