Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: prevent ClassVar override with instance property #18853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
834e125
feat: check if `@property` overrides parent class variable
shenyih0ng Mar 29, 2025
0060334
test: add `testOverrideWithPropertyDecorator` unit test
shenyih0ng Mar 29, 2025
f2f8a98
fix: prevent override when there exists a `setter`
shenyih0ng Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2280,6 +2280,25 @@ def check_method_override_for_base_with_name(
if inner is not None:
typ = inner
typ = get_property_type(typ)

if (
isinstance(original_node, Var)
and original_node.is_classvar
and defn.name == original_node.name
and (isinstance(defn, (Decorator, OverloadedFuncDef)))
):
decorator_func = None
if isinstance(defn, Decorator):
decorator_func = defn.func
elif isinstance(defn.items[0], Decorator):
decorator_func = defn.items[0].func

if decorator_func:
self.fail(
message_registry.CANNOT_OVERRIDE_CLASS_VAR.format(base.name),
decorator_func,
)
Comment on lines +2297 to +2300
Copy link
Contributor Author

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:

mypy/mypy/checker.py

Line 3722 in f2f8a98

self.fail(message_registry.CANNOT_OVERRIDE_CLASS_VAR.format(base.name), node)


if (
isinstance(original_node, Var)
and not original_node.is_final
Expand Down
15 changes: 15 additions & 0 deletions test-data/unit/check-classvar.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,21 @@ class A(metaclass=ABCMeta):
class B(A):
x = 0 # type: ClassVar[int]

[case testOverrideWithPropertyDecorator]
from typing import ClassVar

class A:
x: ClassVar[int]
Copy link
Collaborator

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)

class B(A):
@property
def x(self) -> int: ...

@x.setter
def x(self, value: int) -> None: ...
[builtins fixtures/property.pyi]
[out]
main:7: error: Cannot override class variable (previously declared on base class "A") with instance variable

[case testAcrossModules]
import m
reveal_type(m.A().x)
Expand Down