-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Clarify meaning of Final in dataclasses #89547
Comments
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: |
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. |
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: """ 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. |
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. |
Treating Final as ClassVar by default may be fine, There are valid uses of Final with instance variable when one would want the value to be unchanged after the A quick search on github for this pattern gives this PEP-591 says: |
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. |
They are real fields, returned by 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:
is well-defined, intuitive, and implemented consistently both in the runtime and in type checkers. It specifies a dataclass field of type 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 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. |
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. |
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 But even if that change can’t be made, I think backwards compatibility still precludes changing the interpretation of |
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. |
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. |
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 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. |
Any update on this? |
I have been reading this thread with much interest after realizing
does not do what one would think. (Writing After reading through the replies, I agree that there is merit to leaving the current There seems to be a lot of argument over whether |
My reading of this thread is that there is basically agreement on what should happen (though there are some details that may need further discussion), it's just that the specific next steps are not clear, because we are in this gray area where the issue seems too small for a new PEP, but amending existing finalized PEPs is not common. Concretely I think what should happen is this:
Secondly, in order to allow final classvars on dataclasses, we could also do these:
And there are a few open questions:
The question of I think if we were only doing the first thing listed above, it could be done as an amendment clarification to PEP 591. But if we want to allow
I don't see much reason to do this. |
I think the correct order is Given that, the order should be restricted, so only But in my opinion it will be better to introduce dataclass-specific annotation Pros:
Cons:
Why I think this is the best solution:
|
For those who want to define final class variable now, there is a workaround: from dataclasses import dataclass
from typing import Final
class Base:
default_f1: Final = 1 # or Final[int]
@dataclass
class MyClass(Base):
f1: int = Base.default_f1 Also, remember that not annotated attributes are not processed by from dataclasses import dataclass
from typing import ClassVar
@dataclass
class MyClass:
default_f1 = 1 # not added to __dataclass_fields__
f1: int = default_f1
class_field: ClassVar[int] = 1 # added to __dataclass_fields__, but is not returned by fields Such attributes are not final and can be overwritten in subclasses. |
I agree with @carljm's post above (#89547 (comment)), and the good news is that now we have a clear path to make it happen: the Typing Council can change the spec. The procedure is laid out at https://github.com/python/typing-council (basically, first open a discussion on discuss.python.org, then propose a concrete change to the spec and ask the Council for a decision). Separately, the runtime should be changed to allow both of these:
I would accept a patch to CPython to allow both of these variants, regardless of whether the typing spec is changed, under the general principle that the runtime should be more permissive to allow experiments with extending the type system. |
I'd be interested in working on this. This feels like an issue I understand and could use as my first time working on typing.py directly. |
Sounds great, feel free to open a PR and I'll take a look. (I haven't checked recently but I think there's a place in |
Thanks for taking on the runtime change, @hmc-cs-mdrissi ! I can do the typing spec PR: https://discuss.python.org/t/treatment-of-final-attributes-in-dataclass-likes/47154/4 |
I've gotten,
both to work. But a different test case fails that expected
I'm also leaning to include test cases for Annotated interaction and ensure that class ACF:
x: Annotated[ClassVar[Final[int]], "a decoration"] and similar variations are well tested. |
I think it's OK to allow Type checkers in general should allow |
Coming from the runtime type-checking side, now that #116096 is merged, I noticed that dataclass will handle |
I've submitted a pull request to the typing spec: python/typing#1669 The typing spec applies to typecheckers, not to the runtime, so I don't think a dataclasses behavior change to unwrap type qualifiers looking for |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: