diff --git a/README.md b/README.md index f0c72b0908..aaac4a5a86 100644 --- a/README.md +++ b/README.md @@ -529,8 +529,10 @@ For more, see [flake8-bugbear](https://pypi.org/project/flake8-bugbear/22.10.27/ | B019 | CachedInstanceMethod | Use of `functools.lru_cache` or `functools.cache` on methods can lead to memory leaks | | | B021 | FStringDocstring | f-string used as docstring. This will be interpreted by python as a joined string rather than a docstring. | | | B022 | UselessContextlibSuppress | No arguments passed to `contextlib.suppress`. No exceptions will be suppressed and therefore this context manager is redundant | | +| B024 | AbstractBaseClassWithoutAbstractMethod | `...` is an abstract base class, but it has no abstract methods | | | B025 | DuplicateTryBlockException | try-except block with duplicate exception `Exception` | | | B026 | StarArgUnpackingAfterKeywordArg | Star-arg unpacking after a keyword argument is strongly discouraged | | +| B027 | EmptyMethodWithoutAbstractDecorator | `...` is an empty method in an abstract base class, but has no abstract decorator | | ### flake8-builtins @@ -703,7 +705,7 @@ including: - [`flake8-annotations`](https://pypi.org/project/flake8-annotations/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) - [`flake8-bandit`](https://pypi.org/project/flake8-bandit/) (6/40) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (23/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (25/32) - [`flake8-2020`](https://pypi.org/project/flake8-2020/) - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (15/34) - [`autoflake`](https://pypi.org/project/autoflake/) (1/7) @@ -728,7 +730,7 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [`flake8-annotations`](https://pypi.org/project/flake8-annotations/) - [`flake8-bandit`](https://pypi.org/project/flake8-bandit/) (6/40) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (23/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (25/32) - [`flake8-2020`](https://pypi.org/project/flake8-2020/) Ruff can also replace [`isort`](https://pypi.org/project/isort/), [`yesqa`](https://github.com/asottile/yesqa), diff --git a/resources/test/fixtures/B024.py b/resources/test/fixtures/B024.py new file mode 100644 index 0000000000..e385874e9a --- /dev/null +++ b/resources/test/fixtures/B024.py @@ -0,0 +1,129 @@ +""" +Should emit: +B024 - on lines 17, 34, 52, 58, 69, 74, 84, 89 +""" + +import abc +import abc as notabc +from abc import ABC, ABCMeta +from abc import abstractmethod +from abc import abstractmethod as abstract +from abc import abstractmethod as abstractaoeuaoeuaoeu +from abc import abstractmethod as notabstract + +import foo + + +class Base_1(ABC): # error + def method(self): + foo() + + +class Base_2(ABC): + @abstractmethod + def method(self): + foo() + + +class Base_3(ABC): + @abc.abstractmethod + def method(self): + foo() + + +class Base_4(ABC): + @notabc.abstractmethod + def method(self): + foo() + + +class Base_5(ABC): + @abstract + def method(self): + foo() + + +class Base_6(ABC): + @abstractaoeuaoeuaoeu + def method(self): + foo() + + +class Base_7(ABC): # error + @notabstract + def method(self): + foo() + + +class MetaBase_1(metaclass=ABCMeta): # error + def method(self): + foo() + + +class MetaBase_2(metaclass=ABCMeta): + @abstractmethod + def method(self): + foo() + + +class abc_Base_1(abc.ABC): # error + def method(self): + foo() + + +class abc_Base_2(metaclass=abc.ABCMeta): # error + def method(self): + foo() + + +class notabc_Base_1(notabc.ABC): # safe + def method(self): + foo() + + +class multi_super_1(notabc.ABC, abc.ABCMeta): # safe + def method(self): + foo() + + +class multi_super_2(notabc.ABC, metaclass=abc.ABCMeta): # safe + def method(self): + foo() + + +class non_keyword_abcmeta_1(ABCMeta): # safe + def method(self): + foo() + + +class non_keyword_abcmeta_2(abc.ABCMeta): # safe + def method(self): + foo() + + +# very invalid code, but that's up to mypy et al to check +class keyword_abc_1(metaclass=ABC): # safe + def method(self): + foo() + + +class keyword_abc_2(metaclass=abc.ABC): # safe + def method(self): + foo() + + +class abc_set_class_variable_1(ABC): # safe + foo: int + + +class abc_set_class_variable_2(ABC): # safe + foo = 2 + + +class abc_set_class_variable_3(ABC): # safe + foo: int = 2 + + +# this doesn't actually declare a class variable, it's just an expression +class abc_set_class_variable_4(ABC): # error + foo diff --git a/resources/test/fixtures/B027.py b/resources/test/fixtures/B027.py new file mode 100644 index 0000000000..98c6b635f5 --- /dev/null +++ b/resources/test/fixtures/B027.py @@ -0,0 +1,89 @@ +""" +Should emit: +B027 - on lines 12, 15, 18, 22, 30 +""" + +import abc +from abc import ABC +from abc import abstractmethod +from abc import abstractmethod as notabstract + + +class AbstractClass(ABC): + def empty_1(self): # error + ... + + def empty_2(self): # error + pass + + def empty_3(self): # error + """docstring""" + ... + + def empty_4(self): # error + """multiple ellipsis/pass""" + ... + pass + ... + pass + + @notabstract + def empty_5(self): # error + ... + + @abstractmethod + def abstract_1(self): + ... + + @abstractmethod + def abstract_2(self): + pass + + @abc.abstractmethod + def abstract_3(self): + ... + + def body_1(self): + print("foo") + ... + + def body_2(self): + self.body_1() + + +class NonAbstractClass: + def empty_1(self): # safe + ... + + def empty_2(self): # safe + pass + + +# ignore @overload, fixes issue #304 +# ignore overload with other imports, fixes #308 +import typing +import typing as t +import typing as anything +from typing import Union, overload + + +class AbstractClass(ABC): + @overload + def empty_1(self, foo: str): + ... + + @typing.overload + def empty_1(self, foo: int): + ... + + @t.overload + def empty_1(self, foo: list): + ... + + @anything.overload + def empty_1(self, foo: float): + ... + + @abstractmethod + def empty_1(self, foo: Union[str, int, list, float]): + ... diff --git a/src/check_ast.rs b/src/check_ast.rs index ab368dad0a..1e587bb54d 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -452,6 +452,14 @@ where flake8_bugbear::plugins::useless_expression(self, body); } + if self.settings.enabled.contains(&CheckCode::B024) + || self.settings.enabled.contains(&CheckCode::B027) + { + flake8_bugbear::plugins::abstract_base_class( + self, stmt, name, bases, keywords, body, + ); + } + self.check_builtin_shadowing(name, Range::from_located(stmt), false); for expr in bases { diff --git a/src/checks.rs b/src/checks.rs index 12fc74927a..c6639b5dd4 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -97,8 +97,10 @@ pub enum CheckCode { B019, B021, B022, + B024, B025, B026, + B027, // flake8-comprehensions C400, C401, @@ -399,8 +401,10 @@ pub enum CheckKind { CachedInstanceMethod, FStringDocstring, UselessContextlibSuppress, + AbstractBaseClassWithoutAbstractMethod(String), DuplicateTryBlockException(String), StarArgUnpackingAfterKeywordArg, + EmptyMethodWithoutAbstractDecorator(String), // flake8-comprehensions UnnecessaryGeneratorList, UnnecessaryGeneratorSet, @@ -643,8 +647,10 @@ impl CheckCode { CheckCode::B019 => CheckKind::CachedInstanceMethod, CheckCode::B021 => CheckKind::FStringDocstring, CheckCode::B022 => CheckKind::UselessContextlibSuppress, + CheckCode::B024 => CheckKind::AbstractBaseClassWithoutAbstractMethod("...".to_string()), CheckCode::B025 => CheckKind::DuplicateTryBlockException("Exception".to_string()), CheckCode::B026 => CheckKind::StarArgUnpackingAfterKeywordArg, + CheckCode::B027 => CheckKind::EmptyMethodWithoutAbstractDecorator("...".to_string()), // flake8-comprehensions CheckCode::C400 => CheckKind::UnnecessaryGeneratorList, CheckCode::C401 => CheckKind::UnnecessaryGeneratorSet, @@ -886,8 +892,10 @@ impl CheckCode { CheckCode::B019 => CheckCategory::Flake8Bugbear, CheckCode::B021 => CheckCategory::Flake8Bugbear, CheckCode::B022 => CheckCategory::Flake8Bugbear, + CheckCode::B024 => CheckCategory::Flake8Bugbear, CheckCode::B025 => CheckCategory::Flake8Bugbear, CheckCode::B026 => CheckCategory::Flake8Bugbear, + CheckCode::B027 => CheckCategory::Flake8Bugbear, CheckCode::C400 => CheckCategory::Flake8Comprehensions, CheckCode::C401 => CheckCategory::Flake8Comprehensions, CheckCode::C402 => CheckCategory::Flake8Comprehensions, @@ -1092,8 +1100,10 @@ impl CheckKind { CheckKind::CachedInstanceMethod => &CheckCode::B019, CheckKind::FStringDocstring => &CheckCode::B021, CheckKind::UselessContextlibSuppress => &CheckCode::B022, + CheckKind::AbstractBaseClassWithoutAbstractMethod(_) => &CheckCode::B024, CheckKind::DuplicateTryBlockException(_) => &CheckCode::B025, CheckKind::StarArgUnpackingAfterKeywordArg => &CheckCode::B026, + CheckKind::EmptyMethodWithoutAbstractDecorator(_) => &CheckCode::B027, // flake8-comprehensions CheckKind::UnnecessaryGeneratorList => &CheckCode::C400, CheckKind::UnnecessaryGeneratorSet => &CheckCode::C401, @@ -1470,6 +1480,9 @@ impl CheckKind { and therefore this context manager is redundant" .to_string() } + CheckKind::AbstractBaseClassWithoutAbstractMethod(name) => { + format!("`{name}` is an abstract base class, but it has no abstract methods") + } CheckKind::DuplicateTryBlockException(name) => { format!("try-except block with duplicate exception `{name}`") } @@ -1479,6 +1492,12 @@ impl CheckKind { unpacked sequence, and this change of ordering can surprise and mislead readers." .to_string() } + CheckKind::EmptyMethodWithoutAbstractDecorator(name) => { + format!( + "`{name}` is an empty method in an abstract base class, but has no abstract \ + decorator" + ) + } // flake8-comprehensions CheckKind::UnnecessaryGeneratorList => { "Unnecessary generator (rewrite as a `list` comprehension)".to_string() diff --git a/src/checks_gen.rs b/src/checks_gen.rs index f192f3a28c..835441f619 100644 --- a/src/checks_gen.rs +++ b/src/checks_gen.rs @@ -58,8 +58,10 @@ pub enum CheckCodePrefix { B02, B021, B022, + B024, B025, B026, + B027, C, C4, C40, @@ -386,8 +388,10 @@ impl CheckCodePrefix { CheckCode::B019, CheckCode::B021, CheckCode::B022, + CheckCode::B024, CheckCode::B025, CheckCode::B026, + CheckCode::B027, ], CheckCodePrefix::B0 => vec![ CheckCode::B002, @@ -410,8 +414,10 @@ impl CheckCodePrefix { CheckCode::B019, CheckCode::B021, CheckCode::B022, + CheckCode::B024, CheckCode::B025, CheckCode::B026, + CheckCode::B027, ], CheckCodePrefix::B00 => vec![ CheckCode::B002, @@ -456,13 +462,17 @@ impl CheckCodePrefix { CheckCodePrefix::B02 => vec![ CheckCode::B021, CheckCode::B022, + CheckCode::B024, CheckCode::B025, CheckCode::B026, + CheckCode::B027, ], CheckCodePrefix::B021 => vec![CheckCode::B021], CheckCodePrefix::B022 => vec![CheckCode::B022], + CheckCodePrefix::B024 => vec![CheckCode::B024], CheckCodePrefix::B025 => vec![CheckCode::B025], CheckCodePrefix::B026 => vec![CheckCode::B026], + CheckCodePrefix::B027 => vec![CheckCode::B027], CheckCodePrefix::C => vec![ CheckCode::C400, CheckCode::C401, @@ -1205,8 +1215,10 @@ impl CheckCodePrefix { CheckCodePrefix::B02 => PrefixSpecificity::Tens, CheckCodePrefix::B021 => PrefixSpecificity::Explicit, CheckCodePrefix::B022 => PrefixSpecificity::Explicit, + CheckCodePrefix::B024 => PrefixSpecificity::Explicit, CheckCodePrefix::B025 => PrefixSpecificity::Explicit, CheckCodePrefix::B026 => PrefixSpecificity::Explicit, + CheckCodePrefix::B027 => PrefixSpecificity::Explicit, CheckCodePrefix::C => PrefixSpecificity::Category, CheckCodePrefix::C4 => PrefixSpecificity::Hundreds, CheckCodePrefix::C40 => PrefixSpecificity::Tens, diff --git a/src/flake8_bugbear/plugins/abstract_base_class.rs b/src/flake8_bugbear/plugins/abstract_base_class.rs new file mode 100644 index 0000000000..eaa3e0e9ba --- /dev/null +++ b/src/flake8_bugbear/plugins/abstract_base_class.rs @@ -0,0 +1,111 @@ +use fnv::{FnvHashMap, FnvHashSet}; +use rustpython_ast::{Constant, Expr, ExprKind, Keyword, Stmt, StmtKind}; + +use crate::ast::helpers::{compose_call_path, match_call_path}; +use crate::ast::types::Range; +use crate::check_ast::Checker; +use crate::checks::{Check, CheckKind}; + +fn is_abc_class( + bases: &[Expr], + keywords: &[Keyword], + from_imports: &FnvHashMap<&str, FnvHashSet<&str>>, +) -> bool { + keywords.iter().any(|keyword| { + keyword + .node + .arg + .as_ref() + .map(|a| a == "metaclass") + .unwrap_or(false) + && compose_call_path(&keyword.node.value) + .map(|call_path| match_call_path(&call_path, "abc.ABCMeta", from_imports)) + .unwrap_or(false) + }) || bases.iter().any(|base| { + compose_call_path(base) + .map(|call_path| match_call_path(&call_path, "abc.ABC", from_imports)) + .unwrap_or(false) + }) +} + +fn is_empty_body(body: &[Stmt]) -> bool { + body.iter().all(|stmt| match &stmt.node { + StmtKind::Pass => true, + StmtKind::Expr { value } => match &value.node { + ExprKind::Constant { value, .. } => { + matches!(value, Constant::Str(..) | Constant::Ellipsis) + } + _ => false, + }, + _ => false, + }) +} + +fn is_abstractmethod(expr: &Expr, from_imports: &FnvHashMap<&str, FnvHashSet<&str>>) -> bool { + compose_call_path(expr) + .map(|call_path| match_call_path(&call_path, "abc.abstractmethod", from_imports)) + .unwrap_or(false) +} + +fn is_overload(expr: &Expr, from_imports: &FnvHashMap<&str, FnvHashSet<&str>>) -> bool { + compose_call_path(expr) + .map(|call_path| match_call_path(&call_path, "typing.overload", from_imports)) + .unwrap_or(false) +} + +pub fn abstract_base_class( + checker: &mut Checker, + stmt: &Stmt, + name: &str, + bases: &[Expr], + keywords: &[Keyword], + body: &[Stmt], +) { + if bases.len() + keywords.len() == 1 && is_abc_class(bases, keywords, &checker.from_imports) { + let mut has_abstract_method = false; + for stmt in body { + // https://github.com/PyCQA/flake8-bugbear/issues/293 + // Ignore abc's that declares a class attribute that must be set + if let StmtKind::AnnAssign { .. } | StmtKind::Assign { .. } = &stmt.node { + has_abstract_method = true; + continue; + } + + if let StmtKind::FunctionDef { + decorator_list, + body, + .. + } + | StmtKind::AsyncFunctionDef { + decorator_list, + body, + .. + } = &stmt.node + { + let has_abstract_decorator = decorator_list + .iter() + .any(|d| is_abstractmethod(d, &checker.from_imports)); + + has_abstract_method |= has_abstract_decorator; + + if !has_abstract_decorator + && is_empty_body(body) + && !decorator_list + .iter() + .any(|d| is_overload(d, &checker.from_imports)) + { + checker.add_check(Check::new( + CheckKind::EmptyMethodWithoutAbstractDecorator(name.to_string()), + Range::from_located(stmt), + )); + } + } + } + if !has_abstract_method { + checker.add_check(Check::new( + CheckKind::AbstractBaseClassWithoutAbstractMethod(name.to_string()), + Range::from_located(stmt), + )); + } + } +} diff --git a/src/flake8_bugbear/plugins/mod.rs b/src/flake8_bugbear/plugins/mod.rs index 75efdb8523..7cfc44b8a6 100644 --- a/src/flake8_bugbear/plugins/mod.rs +++ b/src/flake8_bugbear/plugins/mod.rs @@ -1,3 +1,4 @@ +pub use abstract_base_class::abstract_base_class; pub use assert_false::assert_false; pub use assert_raises_exception::assert_raises_exception; pub use assignment_to_os_environ::assignment_to_os_environ; @@ -20,6 +21,7 @@ pub use useless_comparison::useless_comparison; pub use useless_contextlib_suppress::useless_contextlib_suppress; pub use useless_expression::useless_expression; +mod abstract_base_class; mod assert_false; mod assert_raises_exception; mod assignment_to_os_environ; diff --git a/src/linter.rs b/src/linter.rs index 6777fcf47f..8d8b397b4c 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -346,8 +346,10 @@ mod tests { #[test_case(CheckCode::B019, Path::new("B019.py"); "B019")] #[test_case(CheckCode::B021, Path::new("B021.py"); "B021")] #[test_case(CheckCode::B022, Path::new("B022.py"); "B022")] + #[test_case(CheckCode::B024, Path::new("B024.py"); "B024")] #[test_case(CheckCode::B025, Path::new("B025.py"); "B025")] #[test_case(CheckCode::B026, Path::new("B026.py"); "B026")] + #[test_case(CheckCode::B027, Path::new("B027.py"); "B027")] #[test_case(CheckCode::C400, Path::new("C400.py"); "C400")] #[test_case(CheckCode::C401, Path::new("C401.py"); "C401")] #[test_case(CheckCode::C402, Path::new("C402.py"); "C402")] diff --git a/src/snapshots/ruff__linter__tests__B024_B024.py.snap b/src/snapshots/ruff__linter__tests__B024_B024.py.snap new file mode 100644 index 0000000000..87c04075be --- /dev/null +++ b/src/snapshots/ruff__linter__tests__B024_B024.py.snap @@ -0,0 +1,86 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + AbstractBaseClassWithoutAbstractMethod: Base_1 + location: + row: 17 + column: 0 + end_location: + row: 22 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: Base_4 + location: + row: 34 + column: 0 + end_location: + row: 40 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: Base_5 + location: + row: 40 + column: 0 + end_location: + row: 46 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: Base_6 + location: + row: 46 + column: 0 + end_location: + row: 52 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: Base_7 + location: + row: 52 + column: 0 + end_location: + row: 58 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: MetaBase_1 + location: + row: 58 + column: 0 + end_location: + row: 63 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: abc_Base_1 + location: + row: 69 + column: 0 + end_location: + row: 74 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: abc_Base_2 + location: + row: 74 + column: 0 + end_location: + row: 79 + column: 0 + fix: ~ +- kind: + AbstractBaseClassWithoutAbstractMethod: abc_set_class_variable_4 + location: + row: 128 + column: 0 + end_location: + row: 130 + column: 0 + fix: ~ + diff --git a/src/snapshots/ruff__linter__tests__B027_B027.py.snap b/src/snapshots/ruff__linter__tests__B027_B027.py.snap new file mode 100644 index 0000000000..52b565f26c --- /dev/null +++ b/src/snapshots/ruff__linter__tests__B027_B027.py.snap @@ -0,0 +1,68 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 13 + column: 4 + end_location: + row: 16 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 16 + column: 4 + end_location: + row: 19 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 19 + column: 4 + end_location: + row: 23 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 23 + column: 4 + end_location: + row: 30 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 31 + column: 4 + end_location: + row: 34 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 80 + column: 4 + end_location: + row: 83 + column: 4 + fix: ~ +- kind: + EmptyMethodWithoutAbstractDecorator: AbstractClass + location: + row: 84 + column: 4 + end_location: + row: 87 + column: 4 + fix: ~ +