microsoft / pyright

Static Type Checker for Python
Other
13.12k stars 1.4k forks source link

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

Closed danpascu closed 1 month ago

danpascu commented 1 month ago

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.

erictraut commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

danpascu commented 1 month ago

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 commented 1 month ago

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.