From ee01e666c5d7201923c62a107517e09ed05aaa89 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 2 Feb 2023 15:45:14 -0500 Subject: [PATCH] Allow list() and tuple() calls in __all__ assignments (#2499) --- .../fixtures/pyflakes/{F822.py => F822_0.py} | 0 resources/test/fixtures/pyflakes/F822_1.py | 3 + .../fixtures/pylint/invalid_all_format.py | 12 ++ .../fixtures/pylint/invalid_all_object.py | 4 + src/ast/operations.rs | 121 ++++++++++++------ src/checkers/ast.rs | 3 +- src/rules/pyflakes/mod.rs | 3 +- ...les__pyflakes__tests__F822_F822_0.py.snap} | 0 ...ules__pyflakes__tests__F822_F822_1.py.snap | 16 +++ ..._tests__PLE0604_invalid_all_object.py.snap | 10 ++ ..._tests__PLE0605_invalid_all_format.py.snap | 20 +++ 11 files changed, 151 insertions(+), 41 deletions(-) rename resources/test/fixtures/pyflakes/{F822.py => F822_0.py} (100%) create mode 100644 resources/test/fixtures/pyflakes/F822_1.py rename src/rules/pyflakes/snapshots/{ruff__rules__pyflakes__tests__F822_F822.py.snap => ruff__rules__pyflakes__tests__F822_F822_0.py.snap} (100%) create mode 100644 src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_1.py.snap diff --git a/resources/test/fixtures/pyflakes/F822.py b/resources/test/fixtures/pyflakes/F822_0.py similarity index 100% rename from resources/test/fixtures/pyflakes/F822.py rename to resources/test/fixtures/pyflakes/F822_0.py diff --git a/resources/test/fixtures/pyflakes/F822_1.py b/resources/test/fixtures/pyflakes/F822_1.py new file mode 100644 index 0000000000..83936dccb5 --- /dev/null +++ b/resources/test/fixtures/pyflakes/F822_1.py @@ -0,0 +1,3 @@ +a = 1 + +__all__ = list(["a", "b"]) diff --git a/resources/test/fixtures/pylint/invalid_all_format.py b/resources/test/fixtures/pylint/invalid_all_format.py index 192d6aa42b..60e508f381 100644 --- a/resources/test/fixtures/pylint/invalid_all_format.py +++ b/resources/test/fixtures/pylint/invalid_all_format.py @@ -6,6 +6,10 @@ __all__ += {"world"} # [invalid-all-format] __all__ = {"world"} + ["Hello"] # [invalid-all-format] +__all__ = {"world"} + list(["Hello"]) # [invalid-all-format] + +__all__ = list(["Hello"]) + {"world"} # [invalid-all-format] + __all__ = (x for x in ["Hello", "world"]) # [invalid-all-format] __all__ = {x for x in ["Hello", "world"]} # [invalid-all-format] @@ -17,3 +21,11 @@ __all__ = ("Hello",) __all__ = ["Hello"] + ("world",) __all__ = [x for x in ["Hello", "world"]] + +__all__ = list(["Hello", "world"]) + +__all__ = list({"Hello", "world"}) + +__all__ = list(["Hello"]) + list(["world"]) + +__all__ = tuple(["Hello"]) + ("world",) diff --git a/resources/test/fixtures/pylint/invalid_all_object.py b/resources/test/fixtures/pylint/invalid_all_object.py index 74cf738dce..e5bf110215 100644 --- a/resources/test/fixtures/pylint/invalid_all_object.py +++ b/resources/test/fixtures/pylint/invalid_all_object.py @@ -4,8 +4,12 @@ __all__ = ( Worm, ) +__all__ = list([None, "Fruit", "Worm"]) # [invalid-all-object] + + class Fruit: pass + class Worm: pass diff --git a/src/ast/operations.rs b/src/ast/operations.rs index 7021ef614f..a2940e64cc 100644 --- a/src/ast/operations.rs +++ b/src/ast/operations.rs @@ -6,9 +6,10 @@ use rustpython_parser::lexer; use rustpython_parser::lexer::Tok; use crate::ast::helpers::any_over_expr; -use crate::ast::types::{Binding, BindingKind, Scope}; +use crate::ast::types::{BindingKind, Scope}; use crate::ast::visitor; use crate::ast::visitor::Visitor; +use crate::checkers::ast::Checker; bitflags! { #[derive(Default)] @@ -20,9 +21,9 @@ bitflags! { /// Extract the names bound to a given __all__ assignment. pub fn extract_all_names( + checker: &Checker, stmt: &Stmt, scope: &Scope, - bindings: &[Binding], ) -> (Vec, AllNamesFlags) { fn add_to_names(names: &mut Vec, elts: &[Expr], flags: &mut AllNamesFlags) { for elt in elts { @@ -38,13 +39,66 @@ pub fn extract_all_names( } } + fn extract_elts<'a>( + checker: &'a Checker, + expr: &'a Expr, + ) -> (Option<&'a Vec>, AllNamesFlags) { + match &expr.node { + ExprKind::List { elts, .. } => { + return (Some(elts), AllNamesFlags::empty()); + } + ExprKind::Tuple { elts, .. } => { + return (Some(elts), AllNamesFlags::empty()); + } + ExprKind::ListComp { .. } => { + // Allow comprehensions, even though we can't statically analyze them. + return (None, AllNamesFlags::empty()); + } + ExprKind::Call { + func, + args, + keywords, + .. + } => { + // Allow `tuple()` and `list()` calls. + if keywords.is_empty() && args.len() <= 1 { + if checker.resolve_call_path(func).map_or(false, |call_path| { + call_path.as_slice() == ["", "tuple"] + || call_path.as_slice() == ["", "list"] + }) { + if args.is_empty() { + return (None, AllNamesFlags::empty()); + } + match &args[0].node { + ExprKind::List { elts, .. } + | ExprKind::Set { elts, .. } + | ExprKind::Tuple { elts, .. } => { + return (Some(elts), AllNamesFlags::empty()); + } + ExprKind::ListComp { .. } + | ExprKind::SetComp { .. } + | ExprKind::GeneratorExp { .. } => { + // Allow comprehensions, even though we can't statically analyze + // them. + return (None, AllNamesFlags::empty()); + } + _ => {} + } + } + } + } + _ => {} + } + (None, AllNamesFlags::INVALID_FORMAT) + } + let mut names: Vec = vec![]; let mut flags = AllNamesFlags::empty(); // Grab the existing bound __all__ values. if let StmtKind::AugAssign { .. } = &stmt.node { if let Some(index) = scope.values.get("__all__") { - if let BindingKind::Export(existing) = &bindings[*index].kind { + if let BindingKind::Export(existing) = &checker.bindings[*index].kind { names.extend_from_slice(existing); } } @@ -56,45 +110,36 @@ pub fn extract_all_names( StmtKind::AugAssign { value, .. } => Some(value), _ => None, } { - match &value.node { - ExprKind::List { elts, .. } | ExprKind::Tuple { elts, .. } => { - add_to_names(&mut names, elts, &mut flags); - } - ExprKind::BinOp { left, right, .. } => { - let mut current_left = left; - let mut current_right = right; - while let Some(elts) = match ¤t_right.node { - ExprKind::List { elts, .. } => Some(elts), - ExprKind::Tuple { elts, .. } => Some(elts), - _ => { - flags |= AllNamesFlags::INVALID_FORMAT; - None - } - } { + if let ExprKind::BinOp { left, right, .. } = &value.node { + let mut current_left = left; + let mut current_right = right; + loop { + // Process the right side, which should be a "real" value. + let (elts, new_flags) = extract_elts(checker, current_right); + flags |= new_flags; + if let Some(elts) = 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, &mut flags); - break; - } - _ => { - flags |= AllNamesFlags::INVALID_FORMAT; - break; - } + } + + // Process the left side, which can be a "real" value or the "rest" of the + // binary operation. + if let ExprKind::BinOp { left, right, .. } = ¤t_left.node { + current_left = left; + current_right = right; + } else { + let (elts, new_flags) = extract_elts(checker, current_left); + flags |= new_flags; + if let Some(elts) = elts { + add_to_names(&mut names, elts, &mut flags); } + break; } } - ExprKind::ListComp { .. } => { - // Allow list comprehensions, even though we can't statically analyze them. - // TODO(charlie): Allow `list()` and `tuple()` calls too, and extract the members - // from them (even if, e.g., it's `list({...})`). - } - _ => { - flags |= AllNamesFlags::INVALID_FORMAT; + } else { + let (elts, new_flags) = extract_elts(checker, value); + flags |= new_flags; + if let Some(elts) = elts { + add_to_names(&mut names, elts, &mut flags); } } } diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index c8d539451f..fd27f768d3 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -4201,8 +4201,7 @@ impl<'a> Checker<'a> { } _ => false, } { - let (all_names, all_names_flags) = - extract_all_names(parent, current, &self.bindings); + let (all_names, all_names_flags) = extract_all_names(self, parent, current); if self.settings.rules.enabled(&Rule::InvalidAllFormat) && matches!(all_names_flags, AllNamesFlags::INVALID_FORMAT) diff --git a/src/rules/pyflakes/mod.rs b/src/rules/pyflakes/mod.rs index 260c855e36..806d259c9b 100644 --- a/src/rules/pyflakes/mod.rs +++ b/src/rules/pyflakes/mod.rs @@ -98,7 +98,8 @@ mod tests { #[test_case(Rule::UndefinedName, Path::new("F821_6.py"); "F821_6")] #[test_case(Rule::UndefinedName, Path::new("F821_7.py"); "F821_7")] #[test_case(Rule::UndefinedName, Path::new("F821_8.pyi"); "F821_8")] - #[test_case(Rule::UndefinedExport, Path::new("F822.py"); "F822")] + #[test_case(Rule::UndefinedExport, Path::new("F822_0.py"); "F822_0")] + #[test_case(Rule::UndefinedExport, Path::new("F822_1.py"); "F822_1")] #[test_case(Rule::UndefinedLocal, Path::new("F823.py"); "F823")] #[test_case(Rule::UnusedVariable, Path::new("F841_0.py"); "F841_0")] #[test_case(Rule::UnusedVariable, Path::new("F841_1.py"); "F841_1")] diff --git a/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822.py.snap b/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_0.py.snap similarity index 100% rename from src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822.py.snap rename to src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_0.py.snap diff --git a/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_1.py.snap b/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_1.py.snap new file mode 100644 index 0000000000..d40ef04643 --- /dev/null +++ b/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F822_F822_1.py.snap @@ -0,0 +1,16 @@ +--- +source: src/rules/pyflakes/mod.rs +expression: diagnostics +--- +- kind: + UndefinedExport: + name: b + location: + row: 3 + column: 0 + end_location: + row: 3 + column: 7 + fix: ~ + parent: ~ + diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_invalid_all_object.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_invalid_all_object.py.snap index 054706e766..5552f55bdc 100644 --- a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_invalid_all_object.py.snap +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0604_invalid_all_object.py.snap @@ -12,4 +12,14 @@ expression: diagnostics column: 7 fix: ~ parent: ~ +- kind: + InvalidAllObject: ~ + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 7 + fix: ~ + parent: ~ diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_invalid_all_format.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_invalid_all_format.py.snap index c0a9fd1def..a7bd2ae80b 100644 --- a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_invalid_all_format.py.snap +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0605_invalid_all_format.py.snap @@ -62,4 +62,24 @@ expression: diagnostics column: 7 fix: ~ parent: ~ +- kind: + InvalidAllFormat: ~ + location: + row: 13 + column: 0 + end_location: + row: 13 + column: 7 + fix: ~ + parent: ~ +- kind: + InvalidAllFormat: ~ + location: + row: 15 + column: 0 + end_location: + row: 15 + column: 7 + fix: ~ + parent: ~