Class method can't get class fields

[ndmitchell]

Describe the Bug

from typing import reveal_type

class Foo:
    x: str

    @classmethod
    def f(cls) -> None:
        reveal_type(cls.x) # E: Instance-only attribute `x` of class `Foo` is not visible on the class

Both Mypy and Pyright think this should be a str, not an error.

Sandbox Link

https://pyrefly.org/sandbox/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMogCmAboQIYA2A+vAoQFD0DGFZAzm1AGJhgBc9KEKgAPPlDYwQjYVAACLdmwiEYACzAATQcM2FgUYAAoWbAJRQAtAD4oAOTApCA2bKKlKNRIRMU2AOhEzHVcoIA

(Only applicable for extension issues) IDE Information

No response

[stroxler]

I'm not sure what we should do here.

Pyrefly is modeling the runtime conservatively - the name x is undefined on Foo (we can add an invocation of Foo.f() to the example, our sandbox clearly shows this).

I'm not entirely convinced that the fact other type checkers get this wrong relative to the runtime is a reason we should be unable to catch the problem. That said, there are plenty of patterns where this might succeed, because some other code might have initialized x in a way that static analysis cannot track.

As a result, maybe we should isolate this case to a specific error kind, but I'm not convinced we should eliminate it entirely

---

CC @rchen152, @grievejia, @yangdanny97 I'm curious your opinions on this one.

I actually thought Pyright didn't allow this, I wonder if it changed recently or my memory is wrong?

[ndmitchell]

https://github.com/pytorch/pytorch/blob/981c99fdffd9a4f510e4b89245d16aa427ee0978/torch/_decomp/decompositions_for_rng.py#L134 as an instance in Pytorch, if that's useful.

[stroxler]

Okay, so here the class is using non-type-level logic for why the access is safe.

First, the line number you linked is setting rather than reading the variable, which is pretty much always runtime-safe since you can define arbitrary new names without an error (and maybe we should allow that).

But more importantly, some classmethods are reading variables that at the type level aren't necessarily defined.

The reason it works, I think, is that they assume (again, not using type level reasoning) that these methods would only ever be invoked inside a context block, and the context manager helper __enter__ initializes all the class fields.

I'm still not entirely convinced this is a compelling use case - the whole point of a type checker is to reason conservatively, and the reasons this work aren't type-level. This particular class works, but I don't think I would turn off safety checks for all other classes to make this unsafe example happy, especially since just initializing the fields in the class body would eliminate the type errors here.

The fact that we diverge from mypy and pyright does make me think we need a dedicated error code at the very least, so that it's easy to opt out.

[samwgoldman]

We can fix the issue by annotating these attributes as ClassVar in PyTorch, yes?

https://github.com/pytorch/pytorch/blob/981c99fdffd9a4f510e4b89245d16aa427ee0978/torch/_decomp/decompositions_for_rng.py#L121-L123

[stroxler]

Whether ClassVar fixes the problem depends how we want the semantics to work

We could probably choose to silence this error whenever ClassVar is used (maybe we already do, TBH I'm not sure), but that isn't fixing the fact that this kind of code is actually unsafe - this name is not defined when the body finishes executing, regardless of how it is annotated, unless it actually gets assigned

This is what Pyrefly is really warning us about, so ClassVar is only partially relevant in that it potentially is a reasonable way for users to tell us they don't want safety

---

If we do a good job implementing the class field logic, using ClassVar would at the very least give us one nice thing, which is that we could complain about the lack of initialization only on the actual declaration rather than the 20-odd downstream uses.

That would definitely be an improvement regardless of what else we do

[stroxler]

Was just reviewing a diff touching relevant code and noted that indeed we already treat ClassVar as magically initialized

So yes, I think we could just annotate these as ClassVar. That trick will work for any case where we hit this issue and the code is never trying to also shadow the same field on instances