Implement ClassVar semantics (fixes #2771)#2873
Conversation
mypy/checkexpr.py
Outdated
|
|
||
| def narrow_type_from_binder(self, expr: Expression, known_type: Type) -> Type: | ||
| if isinstance(known_type, ClassVarType): | ||
| known_type = known_type.item |
There was a problem hiding this comment.
This indeed looks strange, and probably would be not necessary if class variables would be implemented as flags for types or Var nodes.
mypy/subtypes.py
Outdated
| for item in right.items) | ||
| elif isinstance(right, ClassVarType): | ||
| return is_subtype(left, right.item, type_parameter_checker, | ||
| ignore_pos_arg_names=ignore_pos_arg_names) |
There was a problem hiding this comment.
I don't think ClassVar represents actually a new type, so that it should not appear here, otherwise you would need to implement also meet_types, join_types, etc. ClassVar is rather an "access modifier" for a class member.
mypy/checker.py
Outdated
| """ | ||
| if isinstance(instance_type, Instance) and isinstance(attribute_type, ClassVarType): | ||
| self.msg.fail("Illegal assignment to class variable", context) | ||
|
|
There was a problem hiding this comment.
I believe this code should rather appear in analyze_member_access (there is also a special flag is_lvalue to indicate that an assignment to a member is being analysed rather that reading from it).
| item = items[0] | ||
| return TypeType(item, line=t.line) | ||
| elif fullname == 'typing.ClassVar': | ||
| if len(t.args) != 1: |
There was a problem hiding this comment.
I think a bare ClassVar should be also allowed meaning ClassVar[Any]
mypy/types.py
Outdated
| return TypeType(Type.deserialize(data['item'])) | ||
|
|
||
|
|
||
| class ClassVarType(Type): |
There was a problem hiding this comment.
As I already mentioned, I think this should be rather a flag in Type base class or in the Var symbol node (maybe is_classvar flag in some analogy to is_classmethod).
| y = A.x | ||
| reveal_type(y) | ||
| [out] | ||
| main:5: error: Revealed type is 'builtins.int' |
There was a problem hiding this comment.
I think more tests are needed here, some ideas:
- More tests on accessing class vars (through instances, subclasses, and instances of subclasses)
- Class vars with complex types: user defined types, generic collections
- Test behaviour of class vars with type variables in user defined generic classes
- Check correct access and inference for the above point
- Check more overriding options: overriding by an instance variable in subclass, and by assigning to an instance of subclass (I believe both should be errors)
- Test behaviour on attempts to define and/or override class vars inside method bodies.
- Maybe multiple inheritance
- Add also few tests that combine more than one of the above ideas
|
Thank you for working on this! Here are some comments. I think the two major are:
|
|
Indeed, |
It should be done in There are also some other places you should pay attention, in particular EDIT: corrected file name |
|
Great, thanks! This is the approach I tried first, but somehow got convinced the *checking* should be done in typechecker (despite that ClassVar is not really a type) and ended up with ClassVarType and hacks around it.
|
|
Theoretically, ClassVar in a class A should be a field in the metaclass of A (which is never the declared metaclass but a "ghost" metaclass created for A which inherits from the declared/analyzed metaclass). Similarly this should be the case for In other words, if we'll get Type[T] for classes T refer to dedicated TypeInfo for the ghost metaclass, this feature should (hopefully) be trivial to implement: add a variable to the TypeInfo. |
|
@ilevkivskyi, removed |
|
What's not done (and I'd rather postpone to another PR):
class A:
x, y = 1, 2 # type: ClassVar[int], ClassVar[int]
[a, b, *c] = ['a', 'b'] # type: List[ClassVar]
I think basic cases of ClassVars in signatures could be easily checked in |
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! This looks better, I still have some comments. In addition, there are three important things that should be done in this PR:
- Prohibit definition of class vars inside methods, i.e.
x: ClassVar[int]should fail inside methods (probably using.is_func_scope()) - Prohibit class vars in function signatures (as you mentioned)
- In general
ClassVarshould be allowed only as an "outermost" type for simple assignments, things likeUnion[ClassVar[T], int]are invalid (and will actually fail at runtime). This probably should be done intypeanal.py
Everything else probably could be done later in a separate PR
mypy/checkmember.py
Outdated
| # TODO allow setting attributes in subclass (although it is probably an error) | ||
| msg.read_only_property(name, info, node) | ||
| if is_lvalue and var.is_classvar: | ||
| msg.cant_assign_to_classvar(node) |
There was a problem hiding this comment.
This looks surprisingly simple, but maybe more corner cases will be necessary for more tests, see below
There was a problem hiding this comment.
It does, but I think it works. There are tests with subclasses, callables, Type type and everything passes.
mypy/semanal.py
Outdated
| return False | ||
| if self.is_class_scope() or not isinstance(lvalue.node, Var): | ||
| node = cast(Var, lvalue.node) | ||
| node.is_classvar = True |
There was a problem hiding this comment.
This looks wrong to me: in this branch lvalue.node is not an instance of Var, why do you set the .is_classvar flag on something that could be not a Var?
There was a problem hiding this comment.
Indeed, that was wrong. I had if statements written a bit different at first and didn't fully apply De Morgan's law there. And because mypy was complaining there (and I didn't see anything wrong in it...), I've added cast.
mypy/semanal.py
Outdated
| return False | ||
|
|
||
| def check_classvar_override(self, node: Var, is_classvar: bool) -> None: | ||
| name = node.name() |
There was a problem hiding this comment.
I think this should be deferred to a later stage and checked in checker.py (probably in visit_assignment_stmt)
mypy/typeanal.py
Outdated
| if len(t.args) == 0: | ||
| return AnyType(line=t.line) | ||
| if len(t.args) != 1: | ||
| self.fail('ClassVar[...] must have exactly one type argument', t) |
There was a problem hiding this comment.
The error message could be misleading, change it to "... at most one type argument"
| from typing import ClassVar | ||
| class A: | ||
| x = 1 # type: ClassVar[int] | ||
| A().x |
There was a problem hiding this comment.
Use reveal_type here to show that the type is correctly read (i.e. int in this case).
| pass | ||
| class B: | ||
| x = A() # type: ClassVar[A] | ||
|
|
There was a problem hiding this comment.
Add tests where type inside ClassVar is more complex, like List[int], Union[int, str] etc. And check:
class C:
x: ClassVar[List[int]]
C()[0] = 1 # This should probably succeed
C()[0] = 's' # This failsThere was a problem hiding this comment.
I have resorted to using list.append, because indexed assignment didn't work in unit tests (even without ClassVar).
| x = None # type: ClassVar[A] | ||
| C.x = B() | ||
| [out] | ||
| main:8: error: Incompatible types in assignment (expression has type "B", variable has type "A") |
There was a problem hiding this comment.
Check that this succeeds if B is a subclass of A, also check that you can override one classvar with another compatible in a subclass,:
class A:
pass
class B(A):
pass
class C:
x = None # type: ClassVar[A]
class D(C):
x = None # type: ClassVar[B] # This should be allowed| x = 1 # type: int | ||
| class B(A): | ||
| x = 2 # type: ClassVar[int] | ||
| [out] |
There was a problem hiding this comment.
This test looks identical to the previous one.
There was a problem hiding this comment.
Fixed. :) I've meant to test the "opposite" case, where normal attribute is overridden with ClassVar.
test-data/unit/semanal-classvar.test
Outdated
| class A: | ||
| x = None # type: ClassVar[T] | ||
| [out] | ||
| main:4: error: Invalid type "__main__.T" |
There was a problem hiding this comment.
Tests involving generic classes are still missing, for example like these:
T = TypeVar('T')
class A(Generic[T]):
x = None # type: ClassVar[T]
reveal_type(A[int].x) # this should be int
class B(Generic[T]):
x = None # type: ClassVar[List[T]]
B[int].x[0] = 'abc' # This should failetc.
|
Again, added more tests. :) I will move overriding checks to type checker and test cases for
This is already done; ClassVar can be defined only in class scope. There are tests for I'll fix it and add test case for this.
Could you guide me a bit with this then? Like I said, this could be done in
I can totally agree about is valid Python and produces could also work, but of course we should recommend writing |
| x = 1 # type: ClassVar[int] | ||
| class B(A): | ||
| x = 2 # type: int | ||
| [out] |
There was a problem hiding this comment.
Now this tests repeats testOverrideWithNormalAttribute
|
Thank you for working on this! Here is some response to your questions:
You don't need to, if you will only allow
I propose not to allow the syntax in last line now. We don't even know yet how people will use the
I think this should not be allowed. First of all, "there should be only one way to do it" :-), second, we should not allow something that may make people think
I am looking forward for this and for fix for |
test-data/unit/check-classvar.test
Outdated
| [out] | ||
| main:5: error: Revealed type is 'builtins.list[builtins.int*]' | ||
| main:6: error: Argument 1 to "append" of "list" has incompatible type "str"; expected "T" | ||
|
|
There was a problem hiding this comment.
Hm... This error message looks suspicious, it should say "int" instead of "T".
I checked, this is the same without ClassVar, but since we are introducing this feature (class variables), it would be nice to fix this. I guess it is something to do with add_class_tvars in checkmember.py only considering Callable and Overloaded (presumably for @classmethods), but now that we are going to have arbitrary class variables, we should also consider other options (Instance, UnionType, TypeType, etc.) I think this is important because it is something to do with the "core" of new feature.
And could you please add a test with correct assignment, i.e. A[int].x.append(42).
There was a problem hiding this comment.
@miedzinski Jukka proposes, and I agree with him, that generic class variables should be prohibited. So that things like x = None # type: ClassVar[List[T]] should be flagged as errors. It is quite easy to check, there is a function in typeanal.py, get_type_var_names that returns all type variables in an analyzed type (even deeply nested ones). You should make an error if this function returns a non-empty list for something wrapped in ClassVar
|
Sorry, one last thing, could you please also add tests involving import between modules (accessing class var on a class defined in another module), also in |
|
I've made TODO:
Regarding generics: I can do this, but in another PR. It's not directly related to T = TypeVar('T')
class A(Generic[T]):
x = None # type: List[T]
A[int].x.append(0)Right now this fails with |
|
@ilevkivskyi, done. |
ilevkivskyi
left a comment
There was a problem hiding this comment.
OK, thank you, this is almost ready! Just few more minor comments.
| self.builtin_type('builtins.tuple'), t.line) | ||
| items = [self.anal_type(item, True) | ||
| for item in t.items] | ||
| return TupleType(items, self.builtin_type('builtins.tuple'), t.line) |
There was a problem hiding this comment.
Is this change really necessary? I am asking for two reasons:
- This looks unrelated to class variables
- The transformation you made is actually not equivalent, this code returns more precise type on failure (
TupleTypewith corresponding number ofAnys as items, instead of just plainAnyType()intypeanal.pycode)
There was a problem hiding this comment.
My bad. I've restored old behaviour, although in TypeAnalyser. This is needed if we want to fail on
class A:
x, y = None, None # type: ClassVar, ClassVar
Since I've removed the test for this we could allow this, but these variables wouldn't get is_classvar flag. I would keep it as is.
mypy/semanal.py
Outdated
|
|
||
| def check_classvar(self, s: AssignmentStmt) -> None: | ||
| lvalue = s.lvalues[0] | ||
| if len(s.lvalues) != 1 or not isinstance(lvalue, (NameExpr, MemberExpr)): |
There was a problem hiding this comment.
You can just use RefExpr instead of (NameExpr, MemberExpr)
mypy/typeanal.py
Outdated
| if len(t.args) != 1: | ||
| self.fail('ClassVar[...] must have at most one type argument', t) | ||
| return AnyType() | ||
| items = self.anal_array(t.args) |
There was a problem hiding this comment.
I think it would be cleaner if you just return self.anal_nested(t.args[0])
test-data/unit/check-classvar.test
Outdated
| x = 1 # type: ClassVar[int] | ||
| A().x = 2 | ||
| [out] | ||
| main:4: error: Illegal assignment to class variable |
There was a problem hiding this comment.
You use too general error messages that might be not very helpful for a user trying to figure out what is wrong. You could use: 'Can not assign to class variable "x" via instance'.
test-data/unit/check-classvar.test
Outdated
| class B(A): | ||
| x = 2 # type: int | ||
| [out] | ||
| main:5: error: Invalid class attribute definition (previously declared on base class "A") |
There was a problem hiding this comment.
This error message is also too generic. I would propose two separate messages:
- 'Cannot override class variable (previously declared on base class "A") with instance variable'
- 'Cannot override instance variable (previously declared on base class "A") with class variable'
test-data/unit/semanal-classvar.test
Outdated
| class A: | ||
| x, y = None, None # type: ClassVar, ClassVar | ||
| [out] | ||
| main:3: error: Invalid ClassVar definition |
There was a problem hiding this comment.
Although this is not allowed now, I don't think we need to explicitly test this, maybe we will allow this in the future.
test-data/unit/semanal-classvar.test
Outdated
| from typing import ClassVar | ||
| def f(x: str, y: ClassVar) -> None: pass | ||
| [out] | ||
| main:2: error: Invalid ClassVar definition |
There was a problem hiding this comment.
I would like to see one more test with a method, like this
class C:
def meth(x: ClassVar[int]) -> None: ... # Erroralso we need to prohibit ClassVar in for and with statements inside class, like this:
class D:
for i in range(10): # type: ClassVar[int] # Error here
test-data/unit/semanal-classvar.test
Outdated
| class B: | ||
| x = None # type: A[ClassVar] | ||
| [out] | ||
| main:5: error: Invalid ClassVar definition |
There was a problem hiding this comment.
I would like to also have two separate error messages instead of "Invalid ClassVar definition":
- "ClassVar could be only used for assignments in class body"
- "Invalid type: ClassVar nested inside other type"
|
@ilevkivskyi, done. Thanks for suggesting new error messages - I wasn't especially proud of them, but it definitely isn't my strong side. :) |
test-data/unit/semanal-classvar.test
Outdated
| from typing import ClassVar | ||
| def f() -> ClassVar: pass | ||
| [out] | ||
| main:2: error: Invalid type: ClassVar nested inside other type |
There was a problem hiding this comment.
I have one last comment, I would really prefer to see the other error "ClassVar can only be used for assignments in class body" in these three tests. I understand why it behaves like this, but maybe you could play a bit with visit_func_def and typeanal.py to tweak this?
|
@miedzinski Also I would recommend you to update the PR description and add there a brief summary of what you implement here, and how you do this (the current description is too much outdated). |
|
@ilevkivskyi, done. In case of |
Thanks! This is exactly what I wanted. The PR now LGTM. |
|
@ilevkivskyi, done. I have added another helper - I have updated the PR description. I suppose this is not enough to close #2878, right? |
I think you could follow this idea #2878 (comment) and use your function |
|
@ilevkivskyi, done. PR updated so it will close #2878. I hope it's OK. |
|
It looks good to me. |
| [out2] | ||
| main:4: error: Cannot assign to class variable "x" via instance | ||
|
|
||
| [case testIncrementalClassVarGone] |
There was a problem hiding this comment.
Can you add another incremental test that caches a class with a ClassVar (this module is not changed)? Another file then introduces in the .next file an assignment to the class variable via instance, which should be flagged.
|
I checked that this doesn't conflict with Dropbox code bases. I only did a quick pass but this looks good. Thanks for writing such extensive tests! I had one idea for an additional test. Additionally, there a few simple merge conflicts. This should be almost ready to merge, unless @gvanrossum has some comments. |
|
🎉 |
Support ClassVar annotations
Implements ClassVar introduced in PEP 526. Resolves #2771, #2879.
ClassVar[...].ClassVarannotations outside class bodies (regular variable annotations, function and method signatures).self).ClassVars nested inside other types.is_classvarflag toVar.nesting_levelattribute toTypeAnalyserand use it inside helper methodsanal_typeandanal_array. Move analyzing implicitTupleTypes fromSemanticAnalyzertoTypeAnalyser.ClassVar[T]asTand bareClassVarasAny.ClassVars and accessing generic instance variables via class (see Generic class atributes aren't fully instantiated #2878 comments). Addtypes.get_type_varshelper, which returns all type variables present in analyzed type, similar toTypeAnalyser.get_type_var_names.