From 94551a203eebe919a7602ad2844190c585ed4f58 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 27 Jan 2023 17:26:33 +0100 Subject: [PATCH] feat: pylint `PLE0604` and `PLE0605` (#2241) --- README.md | 2 + resources/test/fixtures/pylint/PLE0604.py | 11 +++++ resources/test/fixtures/pylint/PLE0605.py | 7 +++ ruff.schema.json | 4 ++ src/ast/operations.rs | 42 +++++++++++++---- src/checkers/ast.rs | 23 +++++++--- src/registry.rs | 2 + src/rules/pylint/mod.rs | 2 + src/rules/pylint/rules/invalid_all_format.rs | 25 +++++++++++ src/rules/pylint/rules/invalid_all_object.rs | 25 +++++++++++ src/rules/pylint/rules/mod.rs | 4 ++ ...es__pylint__tests__PLE0604_PLE0604.py.snap | 15 +++++++ ...es__pylint__tests__PLE0605_PLE0605.py.snap | 45 +++++++++++++++++++ 13 files changed, 192 insertions(+), 15 deletions(-) create mode 100644 resources/test/fixtures/pylint/PLE0604.py create mode 100644 resources/test/fixtures/pylint/PLE0605.py create mode 100644 src/rules/pylint/rules/invalid_all_format.rs create mode 100644 src/rules/pylint/rules/invalid_all_object.rs create mode 100644 src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_PLE0604.py.snap create mode 100644 src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_PLE0605.py.snap diff --git a/README.md b/README.md index 0b2ce37b45..25eac7effc 100644 --- a/README.md +++ b/README.md @@ -1248,6 +1248,8 @@ For more, see [Pylint](https://pypi.org/project/pylint/) on PyPI. | ---- | ---- | ------- | --- | | PLE0117 | nonlocal-without-binding | Nonlocal name `{name}` found without binding | | | PLE0118 | used-prior-global-declaration | Name `{name}` is used prior to global declaration on line {line} | | +| PLE0604 | invalid-all-object | Invalid object in `__all__`, must contain only strings | | +| PLE0605 | invalid-all-format | Invalid format for `__all__`, must be `tuple` or `list` | | | PLE1142 | await-outside-async | `await` should be used within an async function | | #### Refactor (PLR) diff --git a/resources/test/fixtures/pylint/PLE0604.py b/resources/test/fixtures/pylint/PLE0604.py new file mode 100644 index 0000000000..74cf738dce --- /dev/null +++ b/resources/test/fixtures/pylint/PLE0604.py @@ -0,0 +1,11 @@ +__all__ = ( + None, # [invalid-all-object] + Fruit, + Worm, +) + +class Fruit: + pass + +class Worm: + pass diff --git a/resources/test/fixtures/pylint/PLE0605.py b/resources/test/fixtures/pylint/PLE0605.py new file mode 100644 index 0000000000..71acce9e96 --- /dev/null +++ b/resources/test/fixtures/pylint/PLE0605.py @@ -0,0 +1,7 @@ +__all__ = ("CONST") # [invalid-all-format] + +__all__ = ["Hello"] + {"world"} # [invalid-all-format] + +__all__ += {"world"} # [invalid-all-format] + +__all__ = {"world"} + ["Hello"] # [invalid-all-format] diff --git a/ruff.schema.json b/ruff.schema.json index f55b62f279..48ffe864da 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1615,6 +1615,10 @@ "PLE011", "PLE0117", "PLE0118", + "PLE06", + "PLE060", + "PLE0604", + "PLE0605", "PLE1", "PLE11", "PLE114", diff --git a/src/ast/operations.rs b/src/ast/operations.rs index 820d4ae7a3..6d5870ff1b 100644 --- a/src/ast/operations.rs +++ b/src/ast/operations.rs @@ -1,3 +1,4 @@ +use bitflags::bitflags; use rustc_hash::FxHashMap; use rustpython_ast::{Cmpop, Located}; use rustpython_parser::ast::{Constant, Expr, ExprKind, Stmt, StmtKind}; @@ -9,9 +10,21 @@ use crate::ast::types::{Binding, BindingKind, Scope}; use crate::ast::visitor; use crate::ast::visitor::Visitor; +bitflags! { + #[derive(Default)] + pub struct AllNamesFlags: u32 { + const INVALID_FORMAT = 0b0000_0001; + const INVALID_OBJECT = 0b0000_0010; + } +} + /// Extract the names bound to a given __all__ assignment. -pub fn extract_all_names(stmt: &Stmt, scope: &Scope, bindings: &[Binding]) -> Vec { - fn add_to_names(names: &mut Vec, elts: &[Expr]) { +pub fn extract_all_names( + stmt: &Stmt, + scope: &Scope, + bindings: &[Binding], +) -> (Vec, AllNamesFlags) { + fn add_to_names(names: &mut Vec, elts: &[Expr], flags: &mut AllNamesFlags) { for elt in elts { if let ExprKind::Constant { value: Constant::Str(value), @@ -19,11 +32,14 @@ pub fn extract_all_names(stmt: &Stmt, scope: &Scope, bindings: &[Binding]) -> Ve } = &elt.node { names.push(value.to_string()); + } else { + *flags |= AllNamesFlags::INVALID_OBJECT; } } } let mut names: Vec = vec![]; + let mut flags = AllNamesFlags::empty(); // Grab the existing bound __all__ values. if let StmtKind::AugAssign { .. } = &stmt.node { @@ -42,7 +58,7 @@ pub fn extract_all_names(stmt: &Stmt, scope: &Scope, bindings: &[Binding]) -> Ve } { match &value.node { ExprKind::List { elts, .. } | ExprKind::Tuple { elts, .. } => { - add_to_names(&mut names, elts); + add_to_names(&mut names, elts, &mut flags); } ExprKind::BinOp { left, right, .. } => { let mut current_left = left; @@ -50,27 +66,35 @@ pub fn extract_all_names(stmt: &Stmt, scope: &Scope, bindings: &[Binding]) -> Ve while let Some(elts) = match ¤t_right.node { ExprKind::List { elts, .. } => Some(elts), ExprKind::Tuple { elts, .. } => Some(elts), - _ => None, + _ => { + flags |= AllNamesFlags::INVALID_FORMAT; + None + } } { - add_to_names(&mut names, elts); + add_to_names(&mut names, elts, &mut flags); match ¤t_left.node { ExprKind::BinOp { left, right, .. } => { current_left = left; current_right = right; } ExprKind::List { elts, .. } | ExprKind::Tuple { elts, .. } => { - add_to_names(&mut names, elts); + add_to_names(&mut names, elts, &mut flags); + break; + } + _ => { + flags |= AllNamesFlags::INVALID_FORMAT; break; } - _ => break, } } } - _ => {} + _ => { + flags |= AllNamesFlags::INVALID_FORMAT; + } } } - names + (names, flags) } #[derive(Default)] diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 1dec1bf468..8b89d24af3 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -17,7 +17,7 @@ use rustpython_parser::parser; use smallvec::smallvec; use crate::ast::helpers::{binding_range, collect_call_path, extract_handler_names}; -use crate::ast::operations::extract_all_names; +use crate::ast::operations::{extract_all_names, AllNamesFlags}; use crate::ast::relocate::relocate_expr; use crate::ast::types::{ Binding, BindingKind, CallPath, ClassDef, ExecutionContext, FunctionDef, Lambda, Node, Range, @@ -4157,14 +4157,25 @@ impl<'a> Checker<'a> { } _ => false, } { + let (all_names, all_names_flags) = + extract_all_names(parent, current, &self.bindings); + + if self.settings.rules.enabled(&Rule::InvalidAllFormat) + && matches!(all_names_flags, AllNamesFlags::INVALID_FORMAT) + { + pylint::rules::invalid_all_format(self, expr); + } + + if self.settings.rules.enabled(&Rule::InvalidAllObject) + && matches!(all_names_flags, AllNamesFlags::INVALID_OBJECT) + { + pylint::rules::invalid_all_object(self, expr); + } + self.add_binding( id, Binding { - kind: BindingKind::Export(extract_all_names( - parent, - current, - &self.bindings, - )), + kind: BindingKind::Export(all_names), runtime_usage: None, synthetic_usage: None, typing_usage: None, diff --git a/src/registry.rs b/src/registry.rs index c08b7eb2dd..dd5c77255e 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -78,6 +78,8 @@ ruff_macros::define_rule_mapping!( F842 => violations::UnusedAnnotation, F901 => violations::RaiseNotImplemented, // pylint + PLE0604 => rules::pylint::rules::InvalidAllObject, + PLE0605 => rules::pylint::rules::InvalidAllFormat, PLC0414 => violations::UselessImportAlias, PLC3002 => violations::UnnecessaryDirectLambdaCall, PLE0117 => violations::NonlocalWithoutBinding, diff --git a/src/rules/pylint/mod.rs b/src/rules/pylint/mod.rs index 5e99ecb52b..3071e1ee42 100644 --- a/src/rules/pylint/mod.rs +++ b/src/rules/pylint/mod.rs @@ -34,6 +34,8 @@ mod tests { #[test_case(Rule::MagicValueComparison, Path::new("magic_value_comparison.py"); "PLR2004")] #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"); "PLW0120")] #[test_case(Rule::GlobalVariableNotAssigned, Path::new("global_variable_not_assigned.py"); "PLW0602")] + #[test_case(Rule::InvalidAllFormat, Path::new("PLE0605.py"); "PLE0605")] + #[test_case(Rule::InvalidAllObject, Path::new("PLE0604.py"); "PLE0604")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/src/rules/pylint/rules/invalid_all_format.rs b/src/rules/pylint/rules/invalid_all_format.rs new file mode 100644 index 0000000000..e942adad35 --- /dev/null +++ b/src/rules/pylint/rules/invalid_all_format.rs @@ -0,0 +1,25 @@ +use ruff_macros::derive_message_formats; +use rustpython_ast::Expr; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::define_violation; +use crate::registry::Diagnostic; +use crate::violation::Violation; + +define_violation!( + pub struct InvalidAllFormat; +); +impl Violation for InvalidAllFormat { + #[derive_message_formats] + fn message(&self) -> String { + format!("Invalid format for `__all__`, must be `tuple` or `list`") + } +} + +/// PLE0605 +pub fn invalid_all_format(checker: &mut Checker, expr: &Expr) { + checker + .diagnostics + .push(Diagnostic::new(InvalidAllFormat, Range::from_located(expr))); +} diff --git a/src/rules/pylint/rules/invalid_all_object.rs b/src/rules/pylint/rules/invalid_all_object.rs new file mode 100644 index 0000000000..5425b78f80 --- /dev/null +++ b/src/rules/pylint/rules/invalid_all_object.rs @@ -0,0 +1,25 @@ +use ruff_macros::derive_message_formats; +use rustpython_ast::Expr; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::define_violation; +use crate::registry::Diagnostic; +use crate::violation::Violation; + +define_violation!( + pub struct InvalidAllObject; +); +impl Violation for InvalidAllObject { + #[derive_message_formats] + fn message(&self) -> String { + format!("Invalid object in `__all__`, must contain only strings") + } +} + +/// PLE0604 +pub fn invalid_all_object(checker: &mut Checker, expr: &Expr) { + checker + .diagnostics + .push(Diagnostic::new(InvalidAllObject, Range::from_located(expr))); +} diff --git a/src/rules/pylint/rules/mod.rs b/src/rules/pylint/rules/mod.rs index b146b831ca..3704634d89 100644 --- a/src/rules/pylint/rules/mod.rs +++ b/src/rules/pylint/rules/mod.rs @@ -1,5 +1,7 @@ pub use await_outside_async::await_outside_async; pub use constant_comparison::constant_comparison; +pub use invalid_all_format::{invalid_all_format, InvalidAllFormat}; +pub use invalid_all_object::{invalid_all_object, InvalidAllObject}; pub use magic_value_comparison::magic_value_comparison; pub use merge_isinstance::merge_isinstance; pub use property_with_parameters::property_with_parameters; @@ -12,6 +14,8 @@ pub use useless_import_alias::useless_import_alias; mod await_outside_async; mod constant_comparison; +mod invalid_all_format; +mod invalid_all_object; mod magic_value_comparison; mod merge_isinstance; mod property_with_parameters; diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_PLE0604.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_PLE0604.py.snap new file mode 100644 index 0000000000..054706e766 --- /dev/null +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_PLE0604.py.snap @@ -0,0 +1,15 @@ +--- +source: src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + InvalidAllObject: ~ + location: + row: 1 + column: 0 + end_location: + row: 1 + column: 7 + fix: ~ + parent: ~ + diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_PLE0605.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_PLE0605.py.snap new file mode 100644 index 0000000000..daf58a12ba --- /dev/null +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_PLE0605.py.snap @@ -0,0 +1,45 @@ +--- +source: src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + InvalidAllFormat: ~ + location: + row: 1 + column: 0 + end_location: + row: 1 + column: 7 + fix: ~ + parent: ~ +- kind: + InvalidAllFormat: ~ + location: + row: 3 + column: 0 + end_location: + row: 3 + column: 7 + fix: ~ + parent: ~ +- kind: + InvalidAllFormat: ~ + location: + row: 5 + column: 0 + end_location: + row: 5 + column: 7 + fix: ~ + parent: ~ +- kind: + InvalidAllFormat: ~ + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 7 + fix: ~ + parent: ~ +