diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index a16229d35a..f1809dda7b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/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) { - if matches!(flags, AllNamesFlags::INVALID_FORMAT) { - self.diagnostics - .push(pylint::rules::invalid_all_format(expr)); - } + let mut flags = BindingFlags::empty(); + if all_flags.contains(DunderAllFlags::INVALID_OBJECT) { + flags |= BindingFlags::INVALID_ALL_OBJECT; } - - if self.enabled(Rule::InvalidAllObject) { - if matches!(flags, AllNamesFlags::INVALID_OBJECT) { - self.diagnostics - .push(pylint::rules::invalid_all_object(expr)); - } + if all_flags.contains(DunderAllFlags::INVALID_FORMAT) { + flags |= BindingFlags::INVALID_ALL_FORMAT; } 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::UnconventionalImportAlias, + Rule::InvalidAllFormat, + 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( diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs index 0e50957ebe..7306024965 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs +++ b/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, + name: Option, } 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), } } diff --git a/crates/ruff/src/rules/pylint/rules/invalid_all_format.rs b/crates/ruff/src/rules/pylint/rules/invalid_all_format.rs index 5b275a48a1..dc0903428b 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_all_format.rs +++ b/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 { - Diagnostic::new(InvalidAllFormat, expr.range()) +pub(crate) fn invalid_all_format(binding: &Binding) -> Option { + if binding.is_invalid_all_format() { + Some(Diagnostic::new(InvalidAllFormat, binding.range)) + } else { + None + } } diff --git a/crates/ruff/src/rules/pylint/rules/invalid_all_object.rs b/crates/ruff/src/rules/pylint/rules/invalid_all_object.rs index c555e64aa1..378302c4e4 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_all_object.rs +++ b/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 { - Diagnostic::new(InvalidAllObject, expr.range()) +pub(crate) fn invalid_all_object(binding: &Binding) -> Option { + if binding.is_invalid_all_object() { + Some(Diagnostic::new(InvalidAllObject, binding.range)) + } else { + None + } } diff --git a/crates/ruff_python_ast/src/all.rs b/crates/ruff_python_ast/src/all.rs index 903a56d768..9cfb0eef5b 100644 --- a/crates/ruff_python_ast/src/all.rs +++ b/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 { - const INVALID_FORMAT = 0b0000_0001; - const INVALID_OBJECT = 0b0000_0010; + pub struct DunderAllFlags: u8 { + /// The right-hand-side assignment to __all__ uses an invalid expression (i.e., an + /// 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(stmt: &Stmt, is_builtin: F) -> (Vec<&str>, AllNamesFlags) +pub fn extract_all_names(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(expr: &Expr, is_builtin: F) -> (Option<&[Expr]>, AllNamesFlags) + fn extract_elts(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 is_builtin(id) { - if args.is_empty() { - return (None, AllNamesFlags::empty()); + if matches!(id, "tuple" | "list") && is_builtin(id) { + let [arg] = args.as_slice() else { + return (None, DunderAllFlags::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::List(ast::ExprList { elts, .. }) - | Expr::Set(ast::ExprSet { elts, .. }) - | Expr::Tuple(ast::ExprTuple { elts, .. }) => { - 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()); - } - _ => {} + Expr::ListComp(_) | Expr::SetComp(_) | Expr::GeneratorExp(_) => { + // Allow comprehensions, even though we can't statically analyze + // them. + return (None, DunderAllFlags::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), diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 67498862b8..50bd684fe5 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/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; } }