-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Implement ClassVar semantics (fixes #2771) #2873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks surprisingly simple, but maybe more corner cases will be necessary for more tests, see below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks identical to the previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.