From 1c496e9b3a772c0ac65ee7099e8292ec2a3d01e6 Mon Sep 17 00:00:00 2001 From: Adam Tuft <73994535+adamtuft@users.noreply.github.com> Date: Thu, 6 Jun 2024 10:30:16 +0100 Subject: [PATCH] Add a new ``declare-non-slot`` error code (#9564) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --------- Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas --- doc/data/messages/d/declare-non-slot/bad.py | 5 + doc/data/messages/d/declare-non-slot/good.py | 5 + doc/user_guide/checkers/features.rst | 3 + doc/user_guide/messages/messages_overview.rst | 1 + doc/whatsnew/fragments/9499.feature | 3 + pylint/checkers/classes/class_checker.py | 99 +++++++++++++++++-- .../r/regression_02/regression_5479.py | 2 +- tests/functional/s/slots_checks.py | 59 +++++++++++ tests/functional/s/slots_checks.txt | 2 + 9 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 doc/data/messages/d/declare-non-slot/bad.py create mode 100644 doc/data/messages/d/declare-non-slot/good.py create mode 100644 doc/whatsnew/fragments/9499.feature diff --git a/doc/data/messages/d/declare-non-slot/bad.py b/doc/data/messages/d/declare-non-slot/bad.py new file mode 100644 index 0000000000..5e39d47953 --- /dev/null +++ b/doc/data/messages/d/declare-non-slot/bad.py @@ -0,0 +1,5 @@ +class Student: + __slots__ = ("name",) + + name: str + surname: str # [declare-non-slot] diff --git a/doc/data/messages/d/declare-non-slot/good.py b/doc/data/messages/d/declare-non-slot/good.py new file mode 100644 index 0000000000..1ca1de19c1 --- /dev/null +++ b/doc/data/messages/d/declare-non-slot/good.py @@ -0,0 +1,5 @@ +class Student: + __slots__ = ("name", "surname") + + name: str + surname: str diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 786495754f..5e3a43d9c6 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -282,6 +282,9 @@ Classes checker Messages Used when a method has an attribute different the "self" as first argument. This is considered as an error since this is a so common convention that you shouldn't break it! +:declare-non-slot (E0245): *No such name %r in __slots__* + Raised when a type annotation on a class is absent from the list of names in + __slots__, and __slots__ does not contain a __dict__ entry. :unexpected-special-method-signature (E0302): *The special method %r expects %s param(s), %d %s given* Emitted when a special method was defined with an invalid number of parameters. If it has too few or too many, it might not work at all. diff --git a/doc/user_guide/messages/messages_overview.rst b/doc/user_guide/messages/messages_overview.rst index 99cf238480..4913ccd29f 100644 --- a/doc/user_guide/messages/messages_overview.rst +++ b/doc/user_guide/messages/messages_overview.rst @@ -69,6 +69,7 @@ All messages in the error category: error/catching-non-exception error/class-variable-slots-conflict error/continue-in-finally + error/declare-non-slot error/dict-iter-missing-items error/duplicate-argument-name error/duplicate-bases diff --git a/doc/whatsnew/fragments/9499.feature b/doc/whatsnew/fragments/9499.feature new file mode 100644 index 0000000000..e60a7c4f73 --- /dev/null +++ b/doc/whatsnew/fragments/9499.feature @@ -0,0 +1,3 @@ +Add new `declare-non-slot` error which reports when a class has a `__slots__` member and a type hint on the class is not present in `__slots__`. + +Refs #9499 diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index ffe47ab156..9758d2450c 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -702,6 +702,12 @@ def _has_same_layout_slots( "Used when a class tries to extend an inherited Enum class. " "Doing so will raise a TypeError at runtime.", ), + "E0245": ( + "No such name %r in __slots__", + "declare-non-slot", + "Raised when a type annotation on a class is absent from the list of names in __slots__, " + "and __slots__ does not contain a __dict__ entry.", + ), "R0202": ( "Consider using a decorator instead of calling classmethod", "no-classmethod-decorator", @@ -870,6 +876,7 @@ def _dummy_rgx(self) -> Pattern[str]: "invalid-enum-extension", "subclassed-final-class", "implicit-flag-alias", + "declare-non-slot", ) def visit_classdef(self, node: nodes.ClassDef) -> None: """Init visit variable _accessed.""" @@ -878,6 +885,50 @@ def visit_classdef(self, node: nodes.ClassDef) -> None: self._check_proper_bases(node) self._check_typing_final(node) self._check_consistent_mro(node) + self._check_declare_non_slot(node) + + def _check_declare_non_slot(self, node: nodes.ClassDef) -> None: + if not self._has_valid_slots(node): + return + + slot_names = self._get_classdef_slots_names(node) + + # Stop if empty __slots__ in the class body, this likely indicates that + # this class takes part in multiple inheritance with other slotted classes. + if not slot_names: + return + + # Stop if we find __dict__, since this means attributes can be set + # dynamically + if "__dict__" in slot_names: + return + + for base in node.bases: + ancestor = safe_infer(base) + if not isinstance(ancestor, nodes.ClassDef): + continue + # if any base doesn't have __slots__, attributes can be set dynamically, so stop + if not self._has_valid_slots(ancestor): + return + for slot_name in self._get_classdef_slots_names(ancestor): + if slot_name == "__dict__": + return + slot_names.append(slot_name) + + # Every class in bases has __slots__, our __slots__ is non-empty and there is no __dict__ + + for child in node.body: + if isinstance(child, nodes.AnnAssign): + if child.value is not None: + continue + if isinstance(child.target, nodes.AssignName): + if child.target.name not in slot_names: + self.add_message( + "declare-non-slot", + args=child.target.name, + node=child.target, + confidence=INFERENCE, + ) def _check_consistent_mro(self, node: nodes.ClassDef) -> None: """Detect that a class has a consistent mro or duplicate bases.""" @@ -1482,6 +1533,24 @@ def _check_functools_or_not(self, decorator: nodes.Attribute) -> bool: return "functools" in dict(import_node.names) + def _has_valid_slots(self, node: nodes.ClassDef) -> bool: + if "__slots__" not in node.locals: + return False + + for slots in node.ilookup("__slots__"): + # check if __slots__ is a valid type + if isinstance(slots, util.UninferableBase): + return False + if not is_iterable(slots) and not is_comprehension(slots): + return False + if isinstance(slots, nodes.Const): + return False + if not hasattr(slots, "itered"): + # we can't obtain the values, maybe a .deque? + return False + + return True + def _check_slots(self, node: nodes.ClassDef) -> None: if "__slots__" not in node.locals: return @@ -1515,13 +1584,19 @@ def _check_slots(self, node: nodes.ClassDef) -> None: continue self._check_redefined_slots(node, slots, values) - def _check_redefined_slots( - self, - node: nodes.ClassDef, - slots_node: nodes.NodeNG, - slots_list: list[nodes.NodeNG], - ) -> None: - """Check if `node` redefines a slot which is defined in an ancestor class.""" + def _get_classdef_slots_names(self, node: nodes.ClassDef) -> list[str]: + + slots_names = [] + for slots in node.ilookup("__slots__"): + if isinstance(slots, nodes.Dict): + values = [item[0] for item in slots.items] + else: + values = slots.itered() + slots_names.extend(self._get_slots_names(values)) + + return slots_names + + def _get_slots_names(self, slots_list: list[nodes.NodeNG]) -> list[str]: slots_names: list[str] = [] for slot in slots_list: if isinstance(slot, nodes.Const): @@ -1531,6 +1606,16 @@ def _check_redefined_slots( inferred_slot_value = getattr(inferred_slot, "value", None) if isinstance(inferred_slot_value, str): slots_names.append(inferred_slot_value) + return slots_names + + def _check_redefined_slots( + self, + node: nodes.ClassDef, + slots_node: nodes.NodeNG, + slots_list: list[nodes.NodeNG], + ) -> None: + """Check if `node` redefines a slot which is defined in an ancestor class.""" + slots_names: list[str] = self._get_slots_names(slots_list) # Slots of all parent classes ancestors_slots_names = { diff --git a/tests/functional/r/regression_02/regression_5479.py b/tests/functional/r/regression_02/regression_5479.py index 9955ea6804..9b493f1929 100644 --- a/tests/functional/r/regression_02/regression_5479.py +++ b/tests/functional/r/regression_02/regression_5479.py @@ -1,7 +1,7 @@ """Test for a regression on slots and annotated assignments. Reported in https://github.com/pylint-dev/pylint/issues/5479 """ -# pylint: disable=too-few-public-methods, unused-private-member, missing-class-docstring, missing-function-docstring +# pylint: disable=too-few-public-methods, unused-private-member, missing-class-docstring, missing-function-docstring, declare-non-slot from __future__ import annotations diff --git a/tests/functional/s/slots_checks.py b/tests/functional/s/slots_checks.py index 2c22e968ed..5a0bc0630c 100644 --- a/tests/functional/s/slots_checks.py +++ b/tests/functional/s/slots_checks.py @@ -128,3 +128,62 @@ class Parent: class ChildNotAffectedByValueInSlot(Parent): __slots__ = ('first', ) + + +class ClassTypeHintNotInSlotsWithoutDict: + __slots__ = ("a", "b") + + a: int + b: str + c: bool # [declare-non-slot] + + +class ClassTypeHintNotInSlotsWithDict: + __slots__ = ("a", "b", "__dict__") + + a: int + b: str + c: bool + + +class BaseNoSlots: + pass + + +class DerivedWithSlots(BaseNoSlots): + __slots__ = ("age",) + + price: int + + +class BaseWithSlots: + __slots__ = ("a", "b",) + + +class DerivedWithMoreSlots(BaseWithSlots): + __slots__ = ("c",) + + # Is in base __slots__ + a: int + + # Not in any base __slots__ + d: int # [declare-non-slot] + e: str= "AnnAssign.value is not None" + + +class BaseWithSlotsDict: + __slots__ = ("__dict__", ) + +class DerivedTypeHintNotInSlots(BaseWithSlotsDict): + __slots__ = ("other", ) + + a: int + def __init__(self) -> None: + super().__init__() + self.a = 42 + + +class ClassWithEmptySlotsAndAnnotation: + __slots__ = () + + a: int diff --git a/tests/functional/s/slots_checks.txt b/tests/functional/s/slots_checks.txt index d63ad25173..127cb465fc 100644 --- a/tests/functional/s/slots_checks.txt +++ b/tests/functional/s/slots_checks.txt @@ -14,3 +14,5 @@ invalid-slots:81:0:81:16:TwelfthBad:Invalid __slots__ object:UNDEFINED class-variable-slots-conflict:114:17:114:24:ValueInSlotConflict:Value 'first' in slots conflicts with class variable:UNDEFINED class-variable-slots-conflict:114:45:114:53:ValueInSlotConflict:Value 'fourth' in slots conflicts with class variable:UNDEFINED class-variable-slots-conflict:114:36:114:43:ValueInSlotConflict:Value 'third' in slots conflicts with class variable:UNDEFINED +declare-non-slot:138:4:138:5:ClassTypeHintNotInSlotsWithoutDict:No such name 'c' in __slots__:INFERENCE +declare-non-slot:170:4:170:5:DerivedWithMoreSlots:No such name 'd' in __slots__:INFERENCE