Allow list() and tuple() calls in __all__ assignments (#2499)

resources/test/fixtures/pyflakes/F822_1.py

@@ -0,0 +1,3 @@
+a = 1
+
+__all__ = list(["a", "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",)

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

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<String>, AllNamesFlags) {
     fn add_to_names(names: &mut Vec<String>, 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<Expr>>, 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<String> = 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 &current_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 &current_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, .. } = &current_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);
             }
         }
     }

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)

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")]

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: ~
+

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: ~
 

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: ~