diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/non_slot_assignment.py b/crates/ruff_linter/resources/test/fixtures/pylint/non_slot_assignment.py new file mode 100644 index 0000000000..c1ced1bbb8 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/non_slot_assignment.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 33ca106673..a613625abd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index de80c5dfc0..f7c296a3ec 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 5786e857f9..acae7be19e 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -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, diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 22be5657bd..2845943106 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -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::*; @@ -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; diff --git a/crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs new file mode 100644 index 0000000000..db74a473bc --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs @@ -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 { + // 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 { + // 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()) +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0237_non_slot_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0237_non_slot_assignment.py.snap new file mode 100644 index 0000000000..6b0fc3d4a2 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0237_non_slot_assignment.py.snap @@ -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() + | + + diff --git a/ruff.schema.json b/ruff.schema.json index d2a0bea4ea..65a83ce31c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3119,6 +3119,8 @@ "PLE0117", "PLE0118", "PLE02", + "PLE023", + "PLE0237", "PLE024", "PLE0241", "PLE03",