-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: prevent ClassVar
override with instance property
#18853
base: master
Are you sure you want to change the base?
fix: prevent ClassVar
override with instance property
#18853
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
c47da59
to
1486fad
Compare
1486fad
to
f2f8a98
Compare
self.fail( | ||
message_registry.CANNOT_OVERRIDE_CLASS_VAR.format(base.name), | ||
decorator_func, | ||
) |
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.
wondering if the error code OVERRIDE
should be attached to this error message? left it as misc
to keep behaviour consistent with existing error message:
Line 3722 in f2f8a98
self.fail(message_registry.CANNOT_OVERRIDE_CLASS_VAR.format(base.name), node) |
Diff from mypy_primer, showing the effect of this PR on open source code: django-stubs (https://github.com/typeddjango/django-stubs)
+ django-stubs/db/migrations/operations/special.pyi:38: error: Cannot override class variable (previously declared on base class "Operation") with instance variable [misc]
+ django-stubs/db/migrations/operations/special.pyi:38: note: Error code "misc" not covered by "type: ignore" comment
+ django-stubs/db/migrations/operations/special.pyi:59: error: Cannot override class variable (previously declared on base class "Operation") with instance variable [misc]
+ django-stubs/db/migrations/operations/special.pyi:59: note: Error code "misc" not covered by "type: ignore" comment
|
from typing import ClassVar | ||
|
||
class A: | ||
x: ClassVar[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.
Could you add a testcase with ClassVar[property]
? I suspect that isn't supported even without this PR (#18504) and don't know if it is important enough, but better be explicit about this.
class Parent:
foo: ClassVar[property]
class Child(Parent):
@property
def foo(self) -> None: ...
@foo.setter
def foo(self, _: None) -> None: ...
(this should ideally be accepted, but probably isn't yet)
Fixes #18790
This PR adds a new override check to prevent instance properties (defined using
@property
) from overriding inherited class variables.