Remove __all__ enforcement rules out of binding phase (#5897)

## Summary

This PR moves two rules (`invalid-all-format` and `invalid-all-object`)
out of the name-binding phase, and into the dedicated pass over all
bindings that occurs at the end of the `Checker`. This is part of my
continued quest to separate the semantic model-building logic from the
actual rule enforcement.

crates/ruff/src/checkers/ast/mod.rs

@@ -38,7 +38,7 @@ use rustpython_parser::ast::{
 };
 
 use ruff_diagnostics::{Diagnostic, Fix, IsolationLevel};
-use ruff_python_ast::all::{extract_all_names, AllNamesFlags};
+use ruff_python_ast::all::{extract_all_names, DunderAllFlags};
 use ruff_python_ast::helpers::{extract_handled_exceptions, to_module_path};
 use ruff_python_ast::identifier::Identifier;
 use ruff_python_ast::source_code::{Generator, Indexer, Locator, Quote, Stylist};
@@ -4543,29 +4543,24 @@ impl<'a> Checker<'a> {
                 _ => false,
             }
         {
-            let (names, flags) = extract_all_names(parent, |name| self.semantic.is_builtin(name));
+            let (all_names, all_flags) =
+                extract_all_names(parent, |name| self.semantic.is_builtin(name));
 
-            if self.enabled(Rule::InvalidAllFormat) {
+            let mut flags = BindingFlags::empty();
-                if matches!(flags, AllNamesFlags::INVALID_FORMAT) {
+            if all_flags.contains(DunderAllFlags::INVALID_OBJECT) {
-                    self.diagnostics
+                flags |= BindingFlags::INVALID_ALL_OBJECT;
-                        .push(pylint::rules::invalid_all_format(expr));
-                }
             }
-
+            if all_flags.contains(DunderAllFlags::INVALID_FORMAT) {
-            if self.enabled(Rule::InvalidAllObject) {
+                flags |= BindingFlags::INVALID_ALL_FORMAT;
-                if matches!(flags, AllNamesFlags::INVALID_OBJECT) {
-                    self.diagnostics
-                        .push(pylint::rules::invalid_all_object(expr));
-                }
             }
 
             self.add_binding(
                 id,
                 expr.range(),
                 BindingKind::Export(Export {
-                    names: names.into_boxed_slice(),
+                    names: all_names.into_boxed_slice(),
                 }),
-                BindingFlags::empty(),
+                flags,
             );
             return;
         }
@@ -4779,9 +4774,11 @@ impl<'a> Checker<'a> {
     /// Run any lint rules that operate over a single [`Binding`].
     fn check_bindings(&mut self) {
         if !self.any_enabled(&[
-            Rule::UnusedVariable,
+            Rule::InvalidAllFormat,
-            Rule::UnconventionalImportAlias,
+            Rule::InvalidAllObject,
             Rule::UnaliasedCollectionsAbcSetImport,
+            Rule::UnconventionalImportAlias,
+            Rule::UnusedVariable,
         ]) {
             return;
         }
@@ -4808,7 +4805,16 @@ impl<'a> Checker<'a> {
                     self.diagnostics.push(diagnostic);
                 }
             }
-
+            if self.enabled(Rule::InvalidAllFormat) {
+                if let Some(diagnostic) = pylint::rules::invalid_all_format(binding) {
+                    self.diagnostics.push(diagnostic);
+                }
+            }
+            if self.enabled(Rule::InvalidAllObject) {
+                if let Some(diagnostic) = pylint::rules::invalid_all_object(binding) {
+                    self.diagnostics.push(diagnostic);
+                }
+            }
             if self.enabled(Rule::UnconventionalImportAlias) {
                 if let Some(diagnostic) =
                     flake8_import_conventions::rules::unconventional_import_alias(

crates/ruff/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs

@@ -45,7 +45,7 @@ use crate::checkers::ast::Checker;
 /// - `flake8-bugbear.extend-immutable-calls`
 #[violation]
 pub struct FunctionCallInDefaultArgument {
-    pub name: Option<String>,
+    name: Option<String>,
 }
 
 impl Violation for FunctionCallInDefaultArgument {
@@ -96,7 +96,9 @@ where
                 }
                 visitor::walk_expr(self, expr);
             }
-            Expr::Lambda(_) => {}
+            Expr::Lambda(_) => {
+                // Don't recurse.
+            }
             _ => visitor::walk_expr(self, expr),
         }
     }

crates/ruff/src/rules/pylint/rules/invalid_all_format.rs

@@ -1,7 +1,6 @@
-use rustpython_parser::ast::{Expr, Ranged};
-
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_semantic::Binding;
 
 /// ## What it does
 /// Checks for invalid assignments to `__all__`.
@@ -36,6 +35,10 @@ impl Violation for InvalidAllFormat {
 }
 
 /// PLE0605
-pub(crate) fn invalid_all_format(expr: &Expr) -> Diagnostic {
+pub(crate) fn invalid_all_format(binding: &Binding) -> Option<Diagnostic> {
-    Diagnostic::new(InvalidAllFormat, expr.range())
+    if binding.is_invalid_all_format() {
+        Some(Diagnostic::new(InvalidAllFormat, binding.range))
+    } else {
+        None
+    }
 }

crates/ruff/src/rules/pylint/rules/invalid_all_object.rs

@@ -1,7 +1,6 @@
-use rustpython_parser::ast::{Expr, Ranged};
-
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_semantic::Binding;
 
 /// ## What it does
 /// Checks for the inclusion of invalid objects in `__all__`.
@@ -36,6 +35,10 @@ impl Violation for InvalidAllObject {
 }
 
 /// PLE0604
-pub(crate) fn invalid_all_object(expr: &Expr) -> Diagnostic {
+pub(crate) fn invalid_all_object(binding: &Binding) -> Option<Diagnostic> {
-    Diagnostic::new(InvalidAllObject, expr.range())
+    if binding.is_invalid_all_object() {
+        Some(Diagnostic::new(InvalidAllObject, binding.range))
+    } else {
+        None
+    }
 }

crates/ruff_python_ast/src/all.rs

@@ -3,20 +3,24 @@ use rustpython_parser::ast::{self, Constant, Expr, Stmt};
 
 bitflags! {
     #[derive(Default, Debug, Copy, Clone, PartialEq, Eq)]
-    pub struct AllNamesFlags: u8 {
+    pub struct DunderAllFlags: u8 {
-        const INVALID_FORMAT = 0b0000_0001;
+        /// The right-hand-side assignment to __all__ uses an invalid expression (i.e., an
-        const INVALID_OBJECT = 0b0000_0010;
+        /// expression that doesn't evaluate to a container type, like `__all__ = 1`).
+        const INVALID_FORMAT = 1 << 0;
+        /// The right-hand-side assignment to __all__ contains an invalid member (i.e., a
+        /// non-string, like `__all__ = [1]`).
+        const INVALID_OBJECT = 1 << 1;
     }
 }
 
 /// Extract the names bound to a given __all__ assignment.
 ///
 /// Accepts a closure that determines whether a given name (e.g., `"list"`) is a Python builtin.
-pub fn extract_all_names<F>(stmt: &Stmt, is_builtin: F) -> (Vec<&str>, AllNamesFlags)
+pub fn extract_all_names<F>(stmt: &Stmt, is_builtin: F) -> (Vec<&str>, DunderAllFlags)
 where
     F: Fn(&str) -> bool,
 {
-    fn add_to_names<'a>(elts: &'a [Expr], names: &mut Vec<&'a str>, flags: &mut AllNamesFlags) {
+    fn add_to_names<'a>(elts: &'a [Expr], names: &mut Vec<&'a str>, flags: &mut DunderAllFlags) {
         for elt in elts {
             if let Expr::Constant(ast::ExprConstant {
                 value: Constant::Str(value),
@@ -25,36 +29,36 @@ where
             {
                 names.push(value);
             } else {
-                *flags |= AllNamesFlags::INVALID_OBJECT;
+                *flags |= DunderAllFlags::INVALID_OBJECT;
             }
         }
     }
 
-    fn extract_elts<F>(expr: &Expr, is_builtin: F) -> (Option<&[Expr]>, AllNamesFlags)
+    fn extract_elts<F>(expr: &Expr, is_builtin: F) -> (Option<&[Expr]>, DunderAllFlags)
     where
         F: Fn(&str) -> bool,
     {
         match expr {
             Expr::List(ast::ExprList { elts, .. }) => {
-                return (Some(elts), AllNamesFlags::empty());
+                return (Some(elts), DunderAllFlags::empty());
             }
             Expr::Tuple(ast::ExprTuple { elts, .. }) => {
-                return (Some(elts), AllNamesFlags::empty());
+                return (Some(elts), DunderAllFlags::empty());
             }
             Expr::ListComp(_) => {
                 // Allow comprehensions, even though we can't statically analyze them.
-                return (None, AllNamesFlags::empty());
+                return (None, DunderAllFlags::empty());
             }
             Expr::Name(ast::ExprName { id, .. }) => {
                 // Ex) `__all__ = __all__ + multiprocessing.__all__`
                 if id == "__all__" {
-                    return (None, AllNamesFlags::empty());
+                    return (None, DunderAllFlags::empty());
                 }
             }
             Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
                 // Ex) `__all__ = __all__ + multiprocessing.__all__`
                 if attr == "__all__" {
-                    return (None, AllNamesFlags::empty());
+                    return (None, DunderAllFlags::empty());
                 }
             }
             Expr::Call(ast::ExprCall {
@@ -67,26 +71,22 @@ where
                 if keywords.is_empty() && args.len() <= 1 {
                     if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
                         let id = id.as_str();
-                        if id == "tuple" || id == "list" {
+                        if matches!(id, "tuple" | "list") && is_builtin(id) {
-                            if is_builtin(id) {
+                            let [arg] = args.as_slice() else {
-                                if args.is_empty() {
+                                return (None, DunderAllFlags::empty());
-                                    return (None, AllNamesFlags::empty());
+                            };
+                            match arg {
+                                Expr::List(ast::ExprList { elts, .. })
+                                | Expr::Set(ast::ExprSet { elts, .. })
+                                | Expr::Tuple(ast::ExprTuple { elts, .. }) => {
+                                    return (Some(elts), DunderAllFlags::empty());
                                 }
-                                match &args[0] {
+                                Expr::ListComp(_) | Expr::SetComp(_) | Expr::GeneratorExp(_) => {
-                                    Expr::List(ast::ExprList { elts, .. })
+                                    // Allow comprehensions, even though we can't statically analyze
-                                    | Expr::Set(ast::ExprSet { elts, .. })
+                                    // them.
-                                    | Expr::Tuple(ast::ExprTuple { elts, .. }) => {
+                                    return (None, DunderAllFlags::empty());
-                                        return (Some(elts), AllNamesFlags::empty());
-                                    }
-                                    Expr::ListComp(_)
-                                    | Expr::SetComp(_)
-                                    | Expr::GeneratorExp(_) => {
-                                        // Allow comprehensions, even though we can't statically analyze
-                                        // them.
-                                        return (None, AllNamesFlags::empty());
-                                    }
-                                    _ => {}
                                 }
+                                _ => {}
                             }
                         }
                     }
@@ -94,11 +94,11 @@ where
             }
             _ => {}
         }
-        (None, AllNamesFlags::INVALID_FORMAT)
+        (None, DunderAllFlags::INVALID_FORMAT)
     }
 
     let mut names: Vec<&str> = vec![];
-    let mut flags = AllNamesFlags::empty();
+    let mut flags = DunderAllFlags::empty();
 
     if let Some(value) = match stmt {
         Stmt::Assign(ast::StmtAssign { value, .. }) => Some(value),

crates/ruff_python_semantic/src/binding.rs

@@ -73,6 +73,18 @@ impl<'a> Binding<'a> {
         self.flags.contains(BindingFlags::GLOBAL)
     }
 
+    /// Return `true` if this [`Binding`] represents an assignment to `__all__` with an invalid
+    /// value (e.g., `__all__ = "Foo"`).
+    pub const fn is_invalid_all_format(&self) -> bool {
+        self.flags.contains(BindingFlags::INVALID_ALL_FORMAT)
+    }
+
+    /// Return `true` if this [`Binding`] represents an assignment to `__all__` that includes an
+    /// invalid member (e.g., `__all__ = ["Foo", 1]`).
+    pub const fn is_invalid_all_object(&self) -> bool {
+        self.flags.contains(BindingFlags::INVALID_ALL_OBJECT)
+    }
+
     /// Return `true` if this [`Binding`] represents an unbound variable
     /// (e.g., `x` in `x = 1; del x`).
     pub const fn is_unbound(&self) -> bool {
@@ -234,6 +246,24 @@ bitflags! {
         ///     x = 1
         /// ```
         const GLOBAL = 1 << 4;
+
+        /// The binding represents an export via `__all__`, but the assigned value uses an invalid
+        /// expression (i.e., a non-container type).
+        ///
+        /// For example:
+        /// ```python
+        /// __all__ = 1
+        /// ```
+        const INVALID_ALL_FORMAT = 1 << 5;
+
+        /// The binding represents an export via `__all__`, but the assigned value contains an
+        /// invalid member (i.e., a non-string).
+        ///
+        /// For example:
+        /// ```python
+        /// __all__ = [1]
+        /// ```
+        const INVALID_ALL_OBJECT = 1 << 6;
     }
 }