diff --git a/README.md b/README.md index 4987f7d3f2..35099e619c 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,7 @@ ruff's goal is to achieve feature-parity with Flake8 when used (1) without any p stylistic checks; limiting to Python 3 obviates the need for certain compatibility checks.) Under those conditions, Flake8 implements about 58 rules, give or take. At time of writing, ruff -implements 31 rules. (Note that these 31 rules likely cover a disproportionate share of errors: +implements 33 rules. (Note that these 33 rules likely cover a disproportionate share of errors: unused imports, undefined variables, etc.) Of the unimplemented rules, ruff is missing: diff --git a/resources/test/fixtures/F841.py b/resources/test/fixtures/F841.py index c343debf1e..785095d19f 100644 --- a/resources/test/fixtures/F841.py +++ b/resources/test/fixtures/F841.py @@ -14,3 +14,11 @@ def f(): x = 1 y = 2 z = x + y + + +def g(): + foo = (1, 2) + (a, b) = (1, 2) + + bar = (1, 2) + (c, d) = bar diff --git a/src/ast/operations.rs b/src/ast/operations.rs index b4e16aac65..1801c899f6 100644 --- a/src/ast/operations.rs +++ b/src/ast/operations.rs @@ -100,6 +100,24 @@ pub fn in_nested_block(parent_stack: &[usize], parents: &[&Stmt]) -> bool { false } +/// Check if a node represents an unpacking assignment. +pub fn is_unpacking_assignment(stmt: &Stmt) -> bool { + if let StmtKind::Assign { targets, value, .. } = &stmt.node { + for child in targets { + match &child.node { + ExprKind::Set { .. } | ExprKind::List { .. } | ExprKind::Tuple { .. } => {} + _ => return false, + } + } + match &value.node { + ExprKind::Set { .. } | ExprKind::List { .. } | ExprKind::Tuple { .. } => return false, + _ => {} + } + return true; + } + false +} + /// Struct used to efficiently slice source code at (row, column) Locations. pub struct SourceCodeLocator<'a> { content: &'a str, diff --git a/src/ast/types.rs b/src/ast/types.rs index 953ea878ba..cad8ded844 100644 --- a/src/ast/types.rs +++ b/src/ast/types.rs @@ -35,8 +35,10 @@ impl Scope { #[derive(Clone, Debug)] pub enum BindingKind { + Annotation, Argument, Assignment, + Binding, Builtin, ClassDefinition, Definition, diff --git a/src/check_ast.rs b/src/check_ast.rs index fb1763df81..bc2623cf8b 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -553,7 +553,7 @@ where self.in_literal = true; } } - ExprKind::Tuple { elts, ctx } => { + ExprKind::Tuple { elts, ctx } | ExprKind::List { elts, ctx } => { if matches!(ctx, ExprContext::Store) { let check_too_many_expressions = self.settings.select.contains(&CheckCode::F621); @@ -581,7 +581,7 @@ where } let parent = self.parents[*(self.parent_stack.last().expect("No parent found."))]; - self.handle_node_store(expr, Some(parent)); + self.handle_node_store(expr, parent); } ExprContext::Del => self.handle_node_delete(expr), }, @@ -835,7 +835,7 @@ where ctx: ExprContext::Store, }, ), - Some(parent), + parent, ); self.parents.push(parent); } @@ -853,7 +853,7 @@ where ctx: ExprContext::Store, }, ), - Some(parent), + parent, ); self.parents.push(parent); @@ -1023,7 +1023,7 @@ impl<'a> Checker<'a> { } } - fn handle_node_store(&mut self, expr: &Expr, parent: Option<&Stmt>) { + fn handle_node_store(&mut self, expr: &Expr, parent: &Stmt) { if let ExprKind::Name { id, .. } = &expr.node { let current = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; @@ -1053,35 +1053,59 @@ impl<'a> Checker<'a> { } } - // TODO(charlie): Handle alternate binding types (like `Annotation`). - if id == "__all__" - && matches!(current.kind, ScopeKind::Module) - && parent - .map(|stmt| { - matches!(stmt.node, StmtKind::Assign { .. }) - || matches!(stmt.node, StmtKind::AugAssign { .. }) - || matches!(stmt.node, StmtKind::AnnAssign { .. }) - }) - .unwrap_or_default() + if matches!(parent.node, StmtKind::AnnAssign { value: None, .. }) { + self.add_binding( + id.to_string(), + Binding { + kind: BindingKind::Annotation, + used: None, + location: expr.location, + }, + ); + return; + } + + // TODO(charlie): Include comprehensions here. + if matches!(parent.node, StmtKind::For { .. }) + || matches!(parent.node, StmtKind::AsyncFor { .. }) + || operations::is_unpacking_assignment(parent) { self.add_binding( id.to_string(), Binding { - kind: BindingKind::Export(extract_all_names(parent.unwrap(), current)), + kind: BindingKind::Binding, used: None, location: expr.location, }, ); - } else { + return; + } + + if id == "__all__" + && matches!(current.kind, ScopeKind::Module) + && (matches!(parent.node, StmtKind::Assign { .. }) + || matches!(parent.node, StmtKind::AugAssign { .. }) + || matches!(parent.node, StmtKind::AnnAssign { .. })) + { self.add_binding( id.to_string(), Binding { - kind: BindingKind::Assignment, + kind: BindingKind::Export(extract_all_names(parent, current)), used: None, location: expr.location, }, ); + return; } + + self.add_binding( + id.to_string(), + Binding { + kind: BindingKind::Assignment, + used: None, + location: expr.location, + }, + ); } } diff --git a/src/linter.rs b/src/linter.rs index 91a6865b31..e16c32190e 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -1084,6 +1084,21 @@ mod tests { location: Location::new(16, 5), fix: None, }, + Check { + kind: CheckKind::UnusedVariable("foo".to_string()), + location: Location::new(20, 5), + fix: None, + }, + Check { + kind: CheckKind::UnusedVariable("a".to_string()), + location: Location::new(21, 6), + fix: None, + }, + Check { + kind: CheckKind::UnusedVariable("b".to_string()), + location: Location::new(21, 9), + fix: None, + }, ]; assert_eq!(actual.len(), expected.len()); for i in 0..actual.len() {