Object of type `type[P]` is not assignable to attribute `cls` of type `<class 'P'>`: ?

[smurfix]

Question

What does this error even mean?
I have to admit that I understand neither the "invalid assignment" nor the "implicit shadowing".

class P:
    pass

class Test:
    cls = P

    def __init__(self, prefix: P | None):
        if isinstance(prefix, P):
            self.cls = type(prefix)

$ ty check /tmp/t.py
error[invalid-assignment]: Object of type `type[P]` is not assignable to attribute `cls` of type `<class 'P'>`
 --> /tmp/t.py:9:13
  |
7 |     def __init__(self, prefix: P | None):
8 |         if isinstance(prefix, P):
9 |             self.cls = type(prefix)
  |             ^^^^^^^^
  |
info: Implicit shadowing of class `P`. Add an annotation to make it explicit if this is intentional

Version

0.0.31

[AlexWaygood]

<class 'P'> is a type that includes only one possible runtime value: the class object P itself. It's not currently spellable in the type system, but it's a type that ty infers in some situations.

type[P] is a larger type that holds an infinite set of possible runtime values: the class object P, and any possible subclass of P. This type is spellable in the type system.

ty has here inferred the type of cls in the body of Test as being of type <class 'P'>, but infers the narrowed type of prefix inside the isinstance() branch as being of type type[P] (this less precise type is because the type P does not mean "any instance of exactly P; it means "any instance of P, or any instance of any subclass of P).

That's a partial explanation, but I'm not totally sure our behaviour here is desirable. We wouldn't usually complain about assignments like this unless you've explicitly annotated an instance variable to say what type you'd like us to enforce. You haven't done that here, so it seems undesirable for us to emit this diagnostic. My guess is that we're getting confused between "variable bindings that are created using the class statement" (which in our model constitute "implicit type declarations" as well as variable bindings) with "variable bindings where the bound variable has a class-type".

[carljm]

Thanks for the report!

I think there are two bugs here. One is that we should just get rid of that special "implicit shadowing" message; it fires in too many cases that are not the scenario it is supposed to catch.

The other is that in type promotion we should widen class-literal types to subclass-of types.

(If we do the second, then the first won't be a problem in this specific case anymore, but it's still an issue in other situations.)

[AlexWaygood]

The other is that in type promotion we should widen class-literal types to subclass-of types.

Hmm, that's quite a different analysis to the one I posted above. You really think that for something like this:

class Foo:
    class Bar: ...

reveal_type(Foo().Bar)

the revealed type there should be type[Foo.Bar] rather than <class 'Foo.Bar'>?

What do you think of my argument above that this attribute should not really have been considered by ty as having a "declared type" at all? If we fixed that, that would also get rid of the diagnostic here, I think

[carljm]

It's not totally clear to me that the example you posted and the OP example (nested class definition vs assignment statement where the RHS happens to be a class object) should be handled in the same way. But I do definitely think that cls = P should be treated the same as any other class-body assignment statement without an annotation, which is that we infer an attribute type from the RHS and promote singletons to more useful attribute types.

What do you think of my argument above that this attribute should not really have been considered by ty as having a "declared type" at all?

The only way I can interpret this as a sound proposal, would result in the revealed type in your example becoming Unknown, which seems clearly worse than having it be type[Foo.Bar] (which matches every other type checker). Whatever "declared type" we infer for the attribute becomes both the upper bound for assignments to that attribute, and (for soundness) must also be the type we infer for accesses of the attribute (since accesses must account for possible assignments.)

So it seems to me that if we want your example to reveal <class 'Foo.Bar'>, that implies that we treat nested class statements as effectively immutable attributes. That is, the OP example would still fail if cls = P were replaced with a nested class definition. I think that differentiation between cls = P and an actual nested class definition is probably defensible? But I don't feel super strongly about it.

[smurfix]

You really think that … the revealed type there should be type[Foo.Bar] rather than <class 'Foo.Bar'>?

One might assume that doing this by default might cause interesting (i.e. unwanted) effects WRT covariant vs. contravariant typing rules down the line. (Disclaimer: I'm not at all well-versed in type theory.)

I'm thus mostly-OK with keeping the error (but please improve the error message, e.g. by mentioning that inferring a type for class-values attributes is nontrivial, and recommending to explicitly type it, in this case with cls: type[P]).

What do you think of my argument above that this attribute should not really have been considered by ty as having a "declared type" at all?

cls has been assigned to, so it probably should have some inferred type, even if it's just type. I mean we wouldn't be OK with cls=42down the line, would we?

[AlexWaygood]

The only way I can interpret this as a sound proposal, would result in the revealed type in your example becoming Unknown, which seems clearly worse than having it be type[Foo.Bar] (which matches every other type checker). Whatever "declared type" we infer for the attribute becomes both the upper bound for assignments to that attribute, and (for soundness) must also be the type we infer for accesses of the attribute (since accesses must account for possible assignments.)

It turns out I misunderstood exactly what our current behaviour for instance-attributes-with-class-level-defaults was. It looks like class-literal variables are currently handled consistently with how we handle other instance-attribute variables with class-body defaults. But I don't like how we handle instance attributes with class-body defaults!

My proposal is that since we currently have this behaviour:

class A:
    def __init__(self, x: bool):
        self.x = x

    def method(self):
        self.x = 42

reveal_type(A(True).x)  # revealed: int

we should therefore also have this behaviour, since A.x and B.x are both instance variables (the only difference is that one class has a default value in the class body for the instance attribute and the other does not):

class B:
    x = True

    def __init__(self, x: int | None):
        if x is not None:
            self.x = x

reveal_type(B(42).x)  # revealed: int

This would fully match pyright's behaviour (multiplay gist: 93066476ef91c405816626f618f6cd8a).

[carljm]

Interesting! I think those examples do reveal a bug that was previously hidden by our Unknown unioning. It seems like our intended (and actual) behavior is that when the class body has bindings but no declarations, we also union in inside-method assignments into the attribute type. But somehow when we are checking those very same assignments, we seem to check them against the class-body-assignments-only inferred type, not against the full unioned type? That seems very weird, and like the main thing to fix here, before we consider promoting class literal types.

[AlexWaygood]

That seems very weird, and like the main thing to fix here, before we consider promoting class literal types.

Agreed. It would fix this issue (I think). And remember that I previously experimented with promoting class-literal types, without much success (astral-sh/ruff#23872 (comment)) -- though that was before the recent change to remove | Unknown widening for attributes.