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

Allow both Final[ClassVar[T]] and ClassVar[Final[T]] in dataclass context #8676

Closed
danpascu opened this issue Aug 6, 2024 · 5 comments
Closed
Labels
as designed Not a bug, working as intended enhancement request New feature or request

Comments

@danpascu
Copy link

danpascu commented Aug 6, 2024

Is your feature request related to a problem? Please describe.

I added some size: Final[ClassVar[int]] = 256 within a dataclass and pyright gave an error that ClassVar is not allowed in this context, which was unclear why it failed, considering that this was recently allowed by the typing spec and was implemented in pyright not long ago.

After some googling and reading some discussions on the subject I found that people have mentioned both forms Final[ClassVar[T]] and ClassVar[Final[T]] during the discussions about dataclasses and class variables with final values, so I tried the other form and it worked.

Describe the solution you’d like

I would like to suggest that both forms be accepted. The runtime accepts both and the order in which they are written is just a matter of personal preference. I myself find Final[ClassVar[T]] to be more readable, that's why I wrote it like that in the first place. Accepting both would definitely reduce the friction and confusion of why it doesn't work for people that write Final[ClassVar[T]] because it sound more readable to them.

@danpascu danpascu added the enhancement request New feature or request label Aug 6, 2024
erictraut added a commit that referenced this issue Aug 6, 2024
… a dataclass. Previously, the `Final` qualifier needed to be the outermost. This addresses #8676.
erictraut added a commit that referenced this issue Aug 6, 2024
… a dataclass. Previously, the `Final` qualifier needed to be the outermost. This addresses #8676. (#8678)
@erictraut erictraut added the addressed in next version Issue is fixed and will appear in next published version label Aug 6, 2024
@erictraut
Copy link
Collaborator

Yeah, I don't see anything in the typing spec that indicates ClassVar must be nested inside Final and not the other way around. Mypy enforces that Final must be outermost, but mypy also doesn't (yet) support Final ClassVars for dataclasses.

This will be included in the next release.

@danpascu
Copy link
Author

danpascu commented Aug 6, 2024

I just realized now that while it makes sense from a typing point of view to allow them to combine in both directions, since both Final and ClassVar apply to the attribute not to each other, this currently doesn't work as expected at runtime because dataclasses when they see Final[ClassVar[T]] do not interpret that as a class attribute and generate an instance attribute instead. I consider this to be a bug in dataclass, due probably to the fact that combining Final and ClassVar in this way was a recent change in the typing spec, so dataclass weren't coded to deal with it yet.

It's up to you if you still consider that this is worth including in the next release. IMO this change in pyright is the right one and a bug report should be opened against dataclasses to consider Final[ClassVar[T]] as a class attribute.

What is your opinion on this?

@erictraut
Copy link
Collaborator

Ah, then I will revert the change. Pyright should match the runtime behavior.

@erictraut erictraut added as designed Not a bug, working as intended and removed addressed in next version Issue is fixed and will appear in next published version labels Aug 6, 2024
@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Aug 6, 2024
@danpascu
Copy link
Author

danpascu commented Aug 6, 2024

Would it be at least possible to give a better error message that suggests what is wrong and what to do about it (i.e. reverse the order of the two) so people don't go hunting on google for answers? I know I wasted a lot of time looking for this because it's not intuitive that they need to be in a specific order, nor is this mandated by the typing spec.

Also do you still think it's worth opening a bug report with dataclasses and coming back to this fix when they address it (if they do)?

@danpascu
Copy link
Author

danpascu commented Aug 6, 2024

I found some discussion about this and someone already mentioned this there, so they seem to be aware of it:
JWCS comment on issue 89547 and carljm comment in issue 89547

So I won't open an issue for the time being, see what that discussion concludes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants