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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
5 changes: 5 additions & 0 deletions doc/data/messages/d/declare-non-slot/bad.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Student:
__slots__ = ("name",)

name: str
surname: str # [declare-non-slot]
5 changes: 5 additions & 0 deletions doc/data/messages/d/declare-non-slot/good.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Student:
__slots__ = ("name", "surname")

name: str
surname: str
3 changes: 3 additions & 0 deletions doc/user_guide/checkers/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/9499.feature
Original file line number Diff line number Diff line change
@@ -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
99 changes: 92 additions & 7 deletions pylint/checkers/classes/class_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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."""
Expand All @@ -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)
adamtuft marked this conversation as resolved.
Show resolved Hide resolved

# 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
Copy link
Member

Choose a reason for hiding this comment

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

We have self.a = 42 line 183 in the functional test, maybe getting coverage for this is as simple as adding a line with self.b : str = "AnnAssign.value is not None" too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added this, hopefully this will fix the code coverage complaint. Waiting for the CI checks to run now.

Copy link
Member

Choose a reason for hiding this comment

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

There might be specific condition to enter this condition, (a "b" value in slot maybe ?)

Copy link
Contributor Author

@adamtuft adamtuft Jun 5, 2024

Choose a reason for hiding this comment

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

The codecov test result seems to have changed now, it's claiming that a line isn't covered which (surely?) must be covered by the tests which generate the new message. I don't understand why the codecov test is failing, are you able to shed any light on this please?

Copy link
Member

Choose a reason for hiding this comment

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

I tried something with b: str being declared. I assume the diff between main and this branch got too big. Also fixed the doc generation.

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
confidence=INFERENCE,
)

def _check_consistent_mro(self, node: nodes.ClassDef) -> None:
"""Detect that a class has a consistent mro or duplicate bases."""
Expand Down Expand Up @@ -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:
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
Contributor 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 @@ -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):
Expand All @@ -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 = {
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
59 changes: 59 additions & 0 deletions tests/functional/s/slots_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

Added this line to cover everything. Do we agree it shouldn't raised @adamtuft ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I see now that I added that annotation to a test which doesn't report the message, so it wasn't covering the required line.

Yes, looks good, that doesn't raise the error 👍 thank you for your help!



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
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__:INFERENCE
declare-non-slot:170:4:170:5:DerivedWithMoreSlots:No such name 'd' in __slots__:INFERENCE
Loading