gh-144618: Don't track ClassVar dataclass members as defaults

[johnslavik]

I haven't added code for cleaning up ClassVar[Field] = dataclasses.field(default=MISSING) from class members, because this is an incorrect use of the API.
Perhaps it should even fail, but that's out of scope of the issue.

• Issue: Dataclasses trigger __get__ on unused ClassVar values #144618

[sobolevn]

What do you think: which parts of 3rd party libs this can potentially affect? Like pydantic validation / etc.

[ericvsmith]

I've asked the pydantic devs to comment.

[johnslavik]

This will subtly change Pydantic by forcing classvar defaults to be dataclasses.MISSING but still keep my repro failing:

https://github.com/pydantic/pydantic/blob/f42171c760d43b9522fde513ae6e209790f7fefb/pydantic/dataclasses.py#L228-L235

I'd tried to run Pydantic test suite against my change but haven't gotten the time to get it to work and closely verify it.

The issue and the change are related to GH-88609.

[Viicos]

The Pydantic test suite passes with this change, although the descriptor's __get__() is still accessed and raises if using Pydantic dataclasses (so even if gh-144618 is fixed here, it will still reproduce, but presumably we can mimic this fix in our dataclass logic).

However, I feel like changing the defaults of class variables is going to cause issues for other users.

As of now, the result of __get__() is used as a default:

from dataclasses import dataclass
from typing import ClassVar

class Kaboom:
    def __get__(self, inst, owner):
        return 1


@dataclass
class Foo:
    kaboom: ClassVar[Kaboom] = Kaboom()

# On main:
Foo.__dataclass_fields__['kaboom'].default
#> 1
# With this PR:
#> MISSING

What about keeping the existing behavior, and if __get__() raises then we keep the Kaboom() instance as a default? (Triggering __get__() -- even if it raises -- might have side effects, but it was already triggered up until now).

[johnslavik]

Thanks a lot for your chiming in @Viicos!

What about keeping the existing behavior, and if __get__() raises then we keep the Kaboom() instance as a default?

It seems unclear whether we would catch BaseException or Exception, and the overall flow I think could become hard to explain. -0 on that.

I feel like changing the defaults of class variables is going to cause issues for other users.

How strongly?

On balance, I'm OK with closing this as too risky. This is an edge case.
My issue is just an instance of GH-88609 with a scope that is safest to change, because classvars aren't exposed as fields publicly.

[Viicos]

My issue is just an instance of GH-88609 with a scope that is safest to change, because classvars aren't exposed as fields publicly.

Ah so given that there's no public way to get the field info for class var pseudo-fields, this might be safe enough then!

[johnslavik]

Hm, looking at the code, this PR also affects a case where Field instance comes from a __get__:

from dataclasses import dataclass, field

class F:
    def __get__(self, instance, owner):
        return field(default=1)
    
@dataclass
class C:
    x: int = F()

print(C())

This used to work and C.x was 1 by default.

I'm not sure this is something we want to support, but I'll fix it anyway.
I'm not going to add this to tests, because I think this is an unintentional consequence of GH-88609.
You are free to revert this fix. cc @ericvsmith

[johnslavik]

I'm not sure if I like how complicated this gets, but it is a step toward GH-88609 -- unless it is too late now and both GH-144618 and GH-88609 should be closed as "not planned". I'm fine with either outcome.