Clarify ClassVar protocol conformance expectations - #2340
Conversation
b297a5f to
7315f15
Compare
|
I agree that the current conformance suite is overstepping what the spec actually says, and the relaxed checks in this PR should not be errors. I would prefer to clarify the spec text and assert that those two lines should not be errors, rather than use |
7315f15 to
7ab5fee
Compare
|
class attributes can (usually) also be accessed on class A:
a: ClassVar[int] = 1
b: int = 1
def get_a(self) -> int:
return self.a # valid
def get_b(self) -> int:
return self.b # validSo in that sense, a I also agree with Carl that we should specify this in the SPEC first. Then, after we've agreed on semantics, we can adjust the conformance tests accordingly. |
yes, I say this in my PR description, and it's covered by the new assertions I added
I don't think this is fully true: a
I fully agree that we should have a conversation about what the spec should say! This, however, is a more involved process that requires discussion on DPO and consensus from the community more broadly. In the meantime, ty has behaviour that I'm unhappy with, and improving that behaviour is not possible without regressing our conformance score. It's precisely because this is not specified at all right now that I wanted to relax the assertions and instead only assert the minimum behaviour that is compatible with all reasonable interpretations of the spec that I'm currently aware of. Completely removing the |
Hmm yea that's a good point. It's too bad we have no good way of specifying whether the So yea, maybe accepting these changes, as a temporary patch of sorts, is a good idea. |
This is PEP 767 which unfortunately stalled over some concerns unrelated to protocols. |
Summary
Relax two conformance-suite assertions that require an explicit
ClassVardeclaration when matching protocol members, even though the typing specification does not clearly impose that requirement. Add coverage for cases that must be rejected under either reasonable interpretation of the specification.Motivation
astral-sh/ruff#27530 changed ty's protocol matching to satisfy two existing conformance-suite assertions. Before that change, ty treated
ClassVarprotocol members structurally: given a protocol memberx: ClassVar[T], an implementing class needed to provide an attribute that was readable on instances and both readable and writable on the class object.For example:
Under a structural interpretation,
Implementationprovides everything the protocol requires: itsvalueattribute is readable and writable on the class object and readable on instances. The fact that the implementing attribute permits additional instance-level operations does not prevent callers using the protocol type from respecting the protocol's restrictions.After astral-sh/ruff#27530, ty rejects this implementation solely because
Implementation.valuewas not explicitly declared withClassVar. That makes protocol matching depend on how an attribute was nominally declared, rather than on the interface the implementing class actually exposes.I would prefer to revert the nominal declaration check added in astral-sh/ruff#27530, since I feel that protocol subtyping and assignability should always be structural, never nominal. Unfortunately, doing so currently causes ty to fail conformance assertions that go beyond what the specification actually says.
What the specification requires
The protocol-member section says:
It doesn't, however, say anything about how a
ClassVarqualifier on protocols should be interpreted. Other sections also do not go into this in much detail:Two interpretations therefore seem possible:
ClassVarprotocol member requires an implementing attribute to be explicitly declared withClassVar.ClassVarprotocol member imposes only structural requirements: the attribute must be readable and writable on the class object and readable on instances.My preference is the second interpretation because it preserves structural protocol matching.
Conformance-suite changes
Mark the existing disputed assertions in
protocols_definition.pyandprotocols_class_objects.pywith# E?rather than# E. This permits checkers to accept or reject implementations whose only disputed property is the absence of an explicit matchingClassVardeclaration.Also add required-error assertions for implementations that should (in my view) fail under either interpretation:
InstanceOnly.valueis available only on instances, so it cannot satisfy the class-object requirements.MetaclassOnly.valueis available on the class object through its metaclass, but it is not available on instances ofMetaclassOnly.The first new assertion also exposes an existing pycroscope bug: pycroscope incorrectly accepts an instance-only attribute as satisfying a
ClassVarprotocol member. This PR therefore updates its recorded conformance result toPartial.