Skip to content

Commit

Permalink
[pylint] Implement assigning-non-slot (E0237) (#9623)
Browse files Browse the repository at this point in the history
## Summary

Implement [assigning-non-slot /
E0237](https://pylint.readthedocs.io/en/latest/user_guide/messages/error/assigning-non-slot.html)

related #970

## Test Plan

`cargo test`
  • Loading branch information
tsugumi-sys authored Jan 24, 2024
1 parent eab1a68 commit 57313d9
Show file tree
Hide file tree
Showing 8 changed files with 338 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
class StudentA:
__slots__ = ("name",)

def __init__(self, name, surname):
self.name = name
self.surname = surname # [assigning-non-slot]
self.setup()

def setup(self):
pass


class StudentB:
__slots__ = ("name", "surname")

def __init__(self, name, middle_name):
self.name = name
self.middle_name = middle_name # [assigning-non-slot]
self.setup()

def setup(self):
pass


class StudentC:
__slots__ = ("name", "surname")

def __init__(self, name, surname):
self.name = name
self.surname = surname # OK
self.setup()

def setup(self):
pass


class StudentD(object):
__slots__ = ("name", "surname")

def __init__(self, name, middle_name):
self.name = name
self.middle_name = middle_name # [assigning-non-slot]
self.setup()

def setup(self):
pass


class StudentE(StudentD):
def __init__(self, name, middle_name):
self.name = name
self.middle_name = middle_name # OK
self.setup()

def setup(self):
pass
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::NoSlotsInNamedtupleSubclass) {
flake8_slots::rules::no_slots_in_namedtuple_subclass(checker, stmt, class_def);
}
if checker.enabled(Rule::NonSlotAssignment) {
pylint::rules::non_slot_assignment(checker, class_def);
}
if checker.enabled(Rule::SingleStringSlots) {
pylint::rules::single_string_slots(checker, class_def);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "E0116") => (RuleGroup::Stable, rules::pylint::rules::ContinueInFinally),
(Pylint, "E0117") => (RuleGroup::Stable, rules::pylint::rules::NonlocalWithoutBinding),
(Pylint, "E0118") => (RuleGroup::Stable, rules::pylint::rules::LoadBeforeGlobalDeclaration),
(Pylint, "E0237") => (RuleGroup::Stable, rules::pylint::rules::NonSlotAssignment),
(Pylint, "E0241") => (RuleGroup::Stable, rules::pylint::rules::DuplicateBases),
(Pylint, "E0302") => (RuleGroup::Stable, rules::pylint::rules::UnexpectedSpecialMethodSignature),
(Pylint, "E0307") => (RuleGroup::Stable, rules::pylint::rules::InvalidStrReturnType),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ mod tests {
Path::new("named_expr_without_context.py")
)]
#[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"))]
#[test_case(Rule::NonSlotAssignment, Path::new("non_slot_assignment.py"))]
#[test_case(Rule::PropertyWithParameters, Path::new("property_with_parameters.py"))]
#[test_case(
Rule::RedefinedArgumentFromLocal,
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub(crate) use no_method_decorator::*;
pub(crate) use no_self_use::*;
pub(crate) use non_ascii_module_import::*;
pub(crate) use non_ascii_name::*;
pub(crate) use non_slot_assignment::*;
pub(crate) use nonlocal_without_binding::*;
pub(crate) use potential_index_error::*;
pub(crate) use property_with_parameters::*;
Expand Down Expand Up @@ -124,6 +125,7 @@ mod no_method_decorator;
mod no_self_use;
mod non_ascii_module_import;
mod non_ascii_name;
mod non_slot_assignment;
mod nonlocal_without_binding;
mod potential_index_error;
mod property_with_parameters;
Expand Down
242 changes: 242 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
use rustc_hash::FxHashSet;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, Stmt};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for assignments to attributes that are not defined in `__slots__`.
///
/// ## Why is this bad?
/// When using `__slots__`, only the specified attributes are allowed.
/// Attempting to assign to an attribute that is not defined in `__slots__`
/// will result in an `AttributeError` at runtime.
///
/// ## Known problems
/// This rule can't detect `__slots__` implementations in superclasses, and
/// so limits its analysis to classes that inherit from (at most) `object`.
///
/// ## Example
/// ```python
/// class Student:
/// __slots__ = ("name",)
///
/// def __init__(self, name, surname):
/// self.name = name
/// self.surname = surname # [assigning-non-slot]
/// self.setup()
///
/// def setup(self):
/// pass
/// ```
///
/// Use instead:
/// ```python
/// class Student:
/// __slots__ = ("name", "surname")
///
/// def __init__(self, name, surname):
/// self.name = name
/// self.surname = surname
/// self.setup()
///
/// def setup(self):
/// pass
/// ```
#[violation]
pub struct NonSlotAssignment {
name: String,
}

impl Violation for NonSlotAssignment {
#[derive_message_formats]
fn message(&self) -> String {
let NonSlotAssignment { name } = self;
format!("Attribute `{name}` is not defined in class's `__slots__`")
}
}

/// E0237
pub(crate) fn non_slot_assignment(checker: &mut Checker, class_def: &ast::StmtClassDef) {
// If the class inherits from another class (aside from `object`), then it's possible that
// the parent class defines the relevant `__slots__`.
if !class_def.bases().iter().all(|base| {
checker
.semantic()
.resolve_call_path(base)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["", "object"]))
}) {
return;
}

for attribute in is_attributes_not_in_slots(&class_def.body) {
checker.diagnostics.push(Diagnostic::new(
NonSlotAssignment {
name: attribute.name.to_string(),
},
attribute.range(),
));
}
}

#[derive(Debug)]
struct AttributeAssignment<'a> {
/// The name of the attribute that is assigned to.
name: &'a str,
/// The range of the attribute that is assigned to.
range: TextRange,
}

impl Ranged for AttributeAssignment<'_> {
fn range(&self) -> TextRange {
self.range
}
}

/// Return a list of attributes that are assigned to but not included in `__slots__`.
fn is_attributes_not_in_slots(body: &[Stmt]) -> Vec<AttributeAssignment> {
// First, collect all the attributes that are assigned to `__slots__`.
let mut slots = FxHashSet::default();
for statement in body {
match statement {
// Ex) `__slots__ = ("name",)`
Stmt::Assign(ast::StmtAssign { targets, value, .. }) => {
let [Expr::Name(ast::ExprName { id, .. })] = targets.as_slice() else {
continue;
};

if id == "__slots__" {
slots.extend(slots_attributes(value));
}
}

// Ex) `__slots__: Tuple[str, ...] = ("name",)`
Stmt::AnnAssign(ast::StmtAnnAssign {
target,
value: Some(value),
..
}) => {
let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() else {
continue;
};

if id == "__slots__" {
slots.extend(slots_attributes(value));
}
}

// Ex) `__slots__ += ("name",)`
Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => {
let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() else {
continue;
};

if id == "__slots__" {
slots.extend(slots_attributes(value));
}
}
_ => {}
}
}

if slots.is_empty() {
return vec![];
}

// Second, find any assignments that aren't included in `__slots__`.
let mut assignments = vec![];
for statement in body {
let Stmt::FunctionDef(ast::StmtFunctionDef { name, body, .. }) = statement else {
continue;
};

if name == "__init__" {
for statement in body {
match statement {
// Ex) `self.name = name`
Stmt::Assign(ast::StmtAssign { targets, .. }) => {
let [Expr::Attribute(attribute)] = targets.as_slice() else {
continue;
};
let Expr::Name(ast::ExprName { id, .. }) = attribute.value.as_ref() else {
continue;
};
if id == "self" && !slots.contains(attribute.attr.as_str()) {
assignments.push(AttributeAssignment {
name: &attribute.attr,
range: attribute.range(),
});
}
}

// Ex) `self.name: str = name`
Stmt::AnnAssign(ast::StmtAnnAssign { target, .. }) => {
let Expr::Attribute(attribute) = target.as_ref() else {
continue;
};
let Expr::Name(ast::ExprName { id, .. }) = attribute.value.as_ref() else {
continue;
};
if id == "self" && !slots.contains(attribute.attr.as_str()) {
assignments.push(AttributeAssignment {
name: &attribute.attr,
range: attribute.range(),
});
}
}

// Ex) `self.name += name`
Stmt::AugAssign(ast::StmtAugAssign { target, .. }) => {
let Expr::Attribute(attribute) = target.as_ref() else {
continue;
};
let Expr::Name(ast::ExprName { id, .. }) = attribute.value.as_ref() else {
continue;
};
if id == "self" && !slots.contains(attribute.attr.as_str()) {
assignments.push(AttributeAssignment {
name: &attribute.attr,
range: attribute.range(),
});
}
}

_ => {}
}
}
}
}

assignments
}

/// Return an iterator over the attributes enumerated in the given `__slots__` value.
fn slots_attributes(expr: &Expr) -> impl Iterator<Item = &str> {
// Ex) `__slots__ = ("name",)`
let elts_iter = match expr {
Expr::Tuple(ast::ExprTuple { elts, .. })
| Expr::List(ast::ExprList { elts, .. })
| Expr::Set(ast::ExprSet { elts, .. }) => Some(elts.iter().filter_map(|elt| match elt {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => Some(value.to_str()),
_ => None,
})),
_ => None,
};

// Ex) `__slots__ = {"name": ...}`
let keys_iter = match expr {
Expr::Dict(ast::ExprDict { keys, .. }) => Some(keys.iter().filter_map(|key| match key {
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => Some(value.to_str()),
_ => None,
})),
_ => None,
};

elts_iter
.into_iter()
.flatten()
.chain(keys_iter.into_iter().flatten())
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
non_slot_assignment.py:6:9: PLE0237 Attribute `surname` is not defined in class's `__slots__`
|
4 | def __init__(self, name, surname):
5 | self.name = name
6 | self.surname = surname # [assigning-non-slot]
| ^^^^^^^^^^^^ PLE0237
7 | self.setup()
|

non_slot_assignment.py:18:9: PLE0237 Attribute `middle_name` is not defined in class's `__slots__`
|
16 | def __init__(self, name, middle_name):
17 | self.name = name
18 | self.middle_name = middle_name # [assigning-non-slot]
| ^^^^^^^^^^^^^^^^ PLE0237
19 | self.setup()
|

non_slot_assignment.py:42:9: PLE0237 Attribute `middle_name` is not defined in class's `__slots__`
|
40 | def __init__(self, name, middle_name):
41 | self.name = name
42 | self.middle_name = middle_name # [assigning-non-slot]
| ^^^^^^^^^^^^^^^^ PLE0237
43 | self.setup()
|


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 57313d9

Please sign in to comment.