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.

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

Add a new declare-non-slot error code #9564

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 84 additions & 7 deletions pylint/checkers/classes/class_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,12 @@ def _has_same_layout_slots(
"Used when we detect that a property also has parameters, which are useless, "
"given that properties cannot be called with additional arguments.",
),
"E0245": (
adamtuft marked this conversation as resolved.
Show resolved Hide resolved
"No such name '%r' in __slots__",
"declare-non-slot",
"Used when a type annotation on a class is absent from the list of names in __slots__, "
"and __slots__ does not contain a __dict__ entry.",
adamtuft marked this conversation as resolved.
Show resolved Hide resolved
),
}


Expand Down Expand Up @@ -872,6 +878,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."""
Expand All @@ -880,6 +887,39 @@ 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)
adamtuft marked this conversation as resolved.
Show resolved Hide resolved

for base in node.bases:
ancestor = safe_infer(base)
if not isinstance(ancestor, nodes.ClassDef):
continue
if not self._has_valid_slots(ancestor):
return
else:
slot_names.extend(self._get_classdef_slots_names(ancestor))
adamtuft marked this conversation as resolved.
Show resolved Hide resolved
adamtuft marked this conversation as resolved.
Show resolved Hide resolved

# Every class in bases has __slots__

if "__dict__" in slot_names:
return

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,
adamtuft marked this conversation as resolved.
Show resolved Hide resolved
)

def _check_consistent_mro(self, node: nodes.ClassDef) -> None:
"""Detect that a class has a consistent mro or duplicate bases."""
Expand Down Expand Up @@ -1485,6 +1525,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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on mobile but this seems very similar to _check_slots. I'm a bit worried that we're duplicating checks and run the risk of code drift.

Have you considered refactoring the other method to serve both purposes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that _check_slots and _has_valid_slots are similar and that it would be best not to duplicate these checks. Refactoring is complicated by the fact that _check_slots is doing a few jobs at the same time:

  • checking whether __slots__ is valid
  • if not, add 1 of 2 possible messages based on the reason why it isn't valid
  • If __slots__ looks valid, get the slot items and apply 2 further checks.

The difference between _check_slots and _has_valid_slots are that _check_slots cares about the reason why __slots__ isn't valid, whereas _has_valid_slots only detects whether __slots__ is valid. It might be wise to introduce a method that serves both purposes e.g. _validate_slots that returns an Enum that either reports a valid __slots__ or says why it is invalid. This would allow you to separate the multiple concerns of the _check_slots method a little more clearly.

The question is whether refactoring _check_slots is within the scope of this MR. To me that feels like quite a big refactor that should be its own MR. I would propose that if the present MR is accepted an issue should then be opened to highlight the need to refactor these slot checks to reduce the duplication. I'd be happy to work on that issue as a separate MR.

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
Expand Down Expand Up @@ -1518,13 +1576,22 @@ 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):

if not self._has_valid_slots(node):
return []

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]):
slots_names: list[str] = []
for slot in slots_list:
if isinstance(slot, nodes.Const):
Expand All @@ -1534,6 +1601,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 = {
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/r/regression_02/regression_5479.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down
53 changes: 53 additions & 0 deletions tests/functional/s/slots_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,56 @@ 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]


class BaseWithSlotsDict:
__slots__ = ("__dict__", )

class DerivedTypeHintNotInSlots(BaseWithSlotsDict):
__slots__ = ("other", )

a: int

def __init__(self) -> None:
super().__init__()
self.a = 42
2 changes: 2 additions & 0 deletions tests/functional/s/slots_checks.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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__:UNDEFINED
declare-non-slot:170:4:170:5:DerivedWithMoreSlots:No such name 'd' in __slots__:UNDEFINED