diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py new file mode 100644 index 0000000000000..fadc6af24c94a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py @@ -0,0 +1,98 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +class Point: # B903 + def __init__(self, x: float, y: float) -> None: + self.x = x + self.y = y + + +class Rectangle: # OK + def __init__(self, top_left: Point, bottom_right: Point) -> None: + ... + + def area(self) -> float: + ... + + +@dataclass +class Circle: # OK + center: Point + radius: float + + def area(self) -> float: + ... + + +class CustomException(Exception): # OK + ... + + +class A: # OK + class B: + ... + + def __init__(self): + ... + +class C: # B903 + c: int + def __init__(self,d:list): + self.d = d + +class D: # B903 + """This class has a docstring.""" + # this next method is an init + def __init__(self,e:dict): + self.e = e + +# <--- begin flake8-bugbear tests below +# (we have modified them to have type annotations, +# since our implementation only triggers in that +# stricter setting.) +class NoWarningsMoreMethods: + def __init__(self, foo:int, bar:list): + self.foo = foo + self.bar = bar + + def other_function(self): ... + + +class NoWarningsClassAttributes: + spam = "ham" + + def __init__(self, foo:int, bar:list): + self.foo = foo + self.bar = bar + + +class NoWarningsComplicatedAssignment: + def __init__(self, foo:int, bar:list): + self.foo = foo + self.bar = bar + self.spam = " - ".join([foo, bar]) + + +class NoWarningsMoreStatements: + def __init__(self, foo:int, bar:list): + foo = " - ".join([foo, bar]) + self.foo = foo + self.bar = bar + + +class Warnings: + def __init__(self, foo:int, bar:list): + self.foo = foo + self.bar = bar + + +class WarningsWithDocstring: + """A docstring should not be an impediment to a warning""" + + def __init__(self, foo:int, bar:list): + self.foo = foo + self.bar = bar + +# <-- end flake8-bugbear tests diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 950b79a504195..02810fba0d81b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -432,6 +432,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::EqWithoutHash) { pylint::rules::object_without_hash_method(checker, class_def); } + if checker.enabled(Rule::ClassAsDataStructure) { + flake8_bugbear::rules::class_as_data_structure(checker, class_def); + } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( checker, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 8723708aca190..155f5e9b8649b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -357,6 +357,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension), (Flake8Bugbear, "039") => (RuleGroup::Stable, rules::flake8_bugbear::rules::MutableContextvarDefault), (Flake8Bugbear, "901") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ReturnInGenerator), + (Flake8Bugbear, "903") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ClassAsDataStructure), (Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept), (Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict), (Flake8Bugbear, "909") => (RuleGroup::Preview, rules::flake8_bugbear::rules::LoopIteratorMutation), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index b254847c38937..27510d7e9282d 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -21,6 +21,7 @@ mod tests { #[test_case(Rule::AssertRaisesException, Path::new("B017.py"))] #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))] #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))] + #[test_case(Rule::ClassAsDataStructure, Path::new("class_as_data_structure.py"))] #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))] #[test_case(Rule::DuplicateTryBlockException, Path::new("B025.py"))] #[test_case(Rule::DuplicateValue, Path::new("B033.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs new file mode 100644 index 0000000000000..035ab4bcbcc83 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -0,0 +1,127 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::{self as ast}; +use ruff_python_semantic::analyze::visibility::{self, Visibility::Public}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for classes that only have a public `__init__` method, +/// without base classes and decorators. +/// +/// ## Why is this bad? +/// Classes with just an `__init__` are possibly better off +/// being a dataclass or a namedtuple, which have less boilerplate. +/// +/// ## Example +/// ```python +/// class Point: +/// def __init__(self, x: float, y: float): +/// self.x = x +/// self.y = y +/// ``` +/// +/// Use instead: +/// ```python +/// from dataclasses import dataclass +/// +/// +/// @dataclass +/// class Point: +/// x: float +/// y: float +/// ``` +#[derive(ViolationMetadata)] +pub(crate) struct ClassAsDataStructure; + +impl Violation for ClassAsDataStructure { + #[derive_message_formats] + fn message(&self) -> String { + "Class could be dataclass or namedtuple".to_string() + } +} + +/// B903 +pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::StmtClassDef) { + // skip stub files + if checker.source_type.is_stub() { + return; + } + + // allow decorated classes + if !class_def.decorator_list.is_empty() { + return; + } + + // allow classes with base classes + if class_def.arguments.is_some() { + return; + } + + let mut public_methods = 0; + let mut has_dunder_init = false; + + for stmt in &class_def.body { + if public_methods > 1 && has_dunder_init { + // we're good to break here + break; + } + match stmt { + ast::Stmt::FunctionDef(func_def) => { + if !has_dunder_init + && func_def.name.to_string() == "__init__" + && func_def + .parameters + .iter() + // skip `self` + .skip(1) + .all(|param| param.annotation().is_some() && !param.is_variadic()) + // `__init__` should not have complicated logic in it + // only assignments + && func_def + .body + .iter() + .all(is_simple_assignment_to_attribute) + { + has_dunder_init = true; + } + if matches!(visibility::method_visibility(func_def), Public) { + public_methods += 1; + } + } + // Ignore class variables + ast::Stmt::Assign(_) | ast::Stmt::AnnAssign(_) | + // and expressions (e.g. string literals) + ast::Stmt::Expr(_) => {} + _ => { + // Bail for anything else - e.g. nested classes + // or conditional methods. + return; + } + } + } + + if has_dunder_init && public_methods == 1 { + checker + .diagnostics + .push(Diagnostic::new(ClassAsDataStructure, class_def.range())); + } +} + +// Checks whether a statement is a, possibly augmented, +// assignment of a name to an attribute. +fn is_simple_assignment_to_attribute(stmt: &ast::Stmt) -> bool { + match stmt { + ast::Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { + let [target] = targets.as_slice() else { + return false; + }; + target.is_attribute_expr() && value.is_name_expr() + } + ast::Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + target.is_attribute_expr() && value.as_ref().is_some_and(|val| val.is_name_expr()) + } + _ => false, + } +} diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs index 8fcca6883ee3f..dcf29d0fc1552 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs @@ -4,6 +4,7 @@ pub(crate) use assert_raises_exception::*; pub(crate) use assignment_to_os_environ::*; pub(crate) use batched_without_explicit_strict::*; pub(crate) use cached_instance_method::*; +pub(crate) use class_as_data_structure::*; pub(crate) use duplicate_exceptions::*; pub(crate) use duplicate_value::*; pub(crate) use except_with_empty_tuple::*; @@ -43,6 +44,7 @@ mod assert_raises_exception; mod assignment_to_os_environ; mod batched_without_explicit_strict; mod cached_instance_method; +mod class_as_data_structure; mod duplicate_exceptions; mod duplicate_value; mod except_with_empty_tuple; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap new file mode 100644 index 0000000000000..eea9b90dd5bfe --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap @@ -0,0 +1,71 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +class_as_data_structure.py:6:1: B903 Class could be dataclass or namedtuple + | +6 | / class Point: # B903 +7 | | def __init__(self, x: float, y: float) -> None: +8 | | self.x = x +9 | | self.y = y + | |__________________^ B903 + | + +class_as_data_structure.py:40:1: B903 Class could be dataclass or namedtuple + | +38 | ... +39 | +40 | / class C: # B903 +41 | | c: int +42 | | def __init__(self,d:list): +43 | | self.d = d + | |__________________^ B903 +44 | +45 | class D: # B903 + | + +class_as_data_structure.py:45:1: B903 Class could be dataclass or namedtuple + | +43 | self.d = d +44 | +45 | / class D: # B903 +46 | | """This class has a docstring.""" +47 | | # this next method is an init +48 | | def __init__(self,e:dict): +49 | | self.e = e + | |__________________^ B903 +50 | +51 | # <--- begin flake8-bugbear tests below + | + +class_as_data_structure.py:63:1: B903 Class could be dataclass or namedtuple + | +63 | / class NoWarningsClassAttributes: +64 | | spam = "ham" +65 | | +66 | | def __init__(self, foo:int, bar:list): +67 | | self.foo = foo +68 | | self.bar = bar + | |______________________^ B903 + | + +class_as_data_structure.py:85:1: B903 Class could be dataclass or namedtuple + | +85 | / class Warnings: +86 | | def __init__(self, foo:int, bar:list): +87 | | self.foo = foo +88 | | self.bar = bar + | |______________________^ B903 + | + +class_as_data_structure.py:91:1: B903 Class could be dataclass or namedtuple + | +91 | / class WarningsWithDocstring: +92 | | """A docstring should not be an impediment to a warning""" +93 | | +94 | | def __init__(self, foo:int, bar:list): +95 | | self.foo = foo +96 | | self.bar = bar + | |______________________^ B903 +97 | +98 | # <-- end flake8-bugbear tests + | diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 56b8f479dfb35..22436db9c92ae 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1571,7 +1571,7 @@ mod tests { const PREVIEW_RULES: &[Rule] = &[ Rule::ReimplementedStarmap, Rule::SliceCopy, - Rule::TooManyPublicMethods, + Rule::ClassAsDataStructure, Rule::TooManyPublicMethods, Rule::UnnecessaryEnumerate, Rule::MathConstant, diff --git a/ruff.schema.json b/ruff.schema.json index f82acd2435383..5b44257bfb391 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2906,6 +2906,7 @@ "B9", "B90", "B901", + "B903", "B904", "B905", "B909",