mirror of https://github.com/astral-sh/ruff
[`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601)
## Summary Adds `class-as-data-structure` rule (`B903`). Also compare pylint's `too-few-public-methods` (`PLR0903`). Took some creative liberty with this by allowing the class to have any decorators or base classes. There are years-old issues on pylint that don't approve of the strictness when it comes to these things. Especially considering that dataclass is a decorator and namedtuple _can be_ a base class. I feel ignoring those explicitly is redundant all things considered, but it's not a hill I'm willing to die on! See: #970 ## Test Plan `cargo test` --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: dylwil3 <dylwil3@gmail.com>
This commit is contained in:
parent
e7248ee43e
commit
78e26cec02
98
crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py
vendored
Normal file
98
crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py
vendored
Normal file
|
|
@ -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
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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"))]
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
||||
|
|
@ -1571,7 +1571,7 @@ mod tests {
|
|||
const PREVIEW_RULES: &[Rule] = &[
|
||||
Rule::ReimplementedStarmap,
|
||||
Rule::SliceCopy,
|
||||
Rule::TooManyPublicMethods,
|
||||
Rule::ClassAsDataStructure,
|
||||
Rule::TooManyPublicMethods,
|
||||
Rule::UnnecessaryEnumerate,
|
||||
Rule::MathConstant,
|
||||
|
|
|
|||
|
|
@ -2906,6 +2906,7 @@
|
|||
"B9",
|
||||
"B90",
|
||||
"B901",
|
||||
"B903",
|
||||
"B904",
|
||||
"B905",
|
||||
"B909",
|
||||
|
|
|
|||
Loading…
Reference in New Issue