From ed1d8d04b483de209ce56e0d5e2025a5690d8646 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 15 Jan 2024 20:08:22 -0500 Subject: [PATCH] Recursively visit deferred AST nodes --- .../test/fixtures/pyflakes/F401_21.py | 2 + .../test/fixtures/pyflakes/F401_22.py | 11 +++ .../ast/analyze/deferred_for_loops.rs | 4 +- .../checkers/ast/analyze/deferred_lambdas.rs | 4 +- .../checkers/ast/analyze/deferred_scopes.rs | 2 +- .../src/checkers/ast/analyze/statement.rs | 2 +- .../ruff_linter/src/checkers/ast/deferred.rs | 28 +++++-- crates/ruff_linter/src/checkers/ast/mod.rs | 83 ++++++++++--------- crates/ruff_linter/src/rules/pyflakes/mod.rs | 1 + ...les__pyflakes__tests__F401_F401_22.py.snap | 4 + 10 files changed, 93 insertions(+), 48 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F401_22.py create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_22.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_21.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_21.py index 0ba6920596964b..0ab6cd6e0a38c4 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_21.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_21.py @@ -1,3 +1,5 @@ +"""Test: ensure that we visit type definitions and lambdas recursively.""" + from __future__ import annotations from collections import Counter diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_22.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_22.py new file mode 100644 index 00000000000000..4dddf165ad19af --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_22.py @@ -0,0 +1,11 @@ +"""Test: ensure that we visit type definitions and lambdas recursively.""" + +from __future__ import annotations + +import re +import os +from typing import cast + +cast(lambda: re.match, 1) + +cast(lambda: cast(lambda: os.path, 1), 1) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index c336121bfdaa11..ee2378dd8bd0e7 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs @@ -6,8 +6,8 @@ use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pyupgrade, refurb} /// Run lint rules over all deferred for-loops in the [`SemanticModel`]. pub(crate) fn deferred_for_loops(checker: &mut Checker) { - while !checker.deferred.for_loops.is_empty() { - let for_loops = std::mem::take(&mut checker.deferred.for_loops); + while !checker.analyze.for_loops.is_empty() { + let for_loops = std::mem::take(&mut checker.analyze.for_loops); for snapshot in for_loops { checker.semantic.restore(snapshot); diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs index f331ef113efd11..421a972e760bb4 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs @@ -6,8 +6,8 @@ use crate::rules::{flake8_pie, pylint, refurb}; /// Run lint rules over all deferred lambdas in the [`SemanticModel`]. pub(crate) fn deferred_lambdas(checker: &mut Checker) { - while !checker.deferred.lambdas.is_empty() { - let lambdas = std::mem::take(&mut checker.deferred.lambdas); + while !checker.analyze.lambdas.is_empty() { + let lambdas = std::mem::take(&mut checker.analyze.lambdas); for snapshot in lambdas { checker.semantic.restore(snapshot); diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index ceb40016ef10f5..acb40c189c960d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -77,7 +77,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { }; let mut diagnostics: Vec = vec![]; - for scope_id in checker.deferred.scopes.iter().rev().copied() { + for scope_id in checker.analyze.scopes.iter().rev().copied() { let scope = &checker.semantic.scopes[scope_id]; if checker.enabled(Rule::UndefinedLocal) { diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index af634230f47092..c9bbe4539a401c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1264,7 +1264,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, ]) { - checker.deferred.for_loops.push(checker.semantic.snapshot()); + checker.analyze.for_loops.push(checker.semantic.snapshot()); } if checker.enabled(Rule::LoopVariableOverridesIterator) { flake8_bugbear::rules::loop_variable_overrides_iterator(checker, target, iter); diff --git a/crates/ruff_linter/src/checkers/ast/deferred.rs b/crates/ruff_linter/src/checkers/ast/deferred.rs index 829621545241e0..e1ca4a86563759 100644 --- a/crates/ruff_linter/src/checkers/ast/deferred.rs +++ b/crates/ruff_linter/src/checkers/ast/deferred.rs @@ -2,16 +2,34 @@ use ruff_python_ast::Expr; use ruff_python_semantic::{ScopeId, Snapshot}; use ruff_text_size::TextRange; -/// A collection of AST nodes that are deferred for later analysis. -/// Used to, e.g., store functions, whose bodies shouldn't be analyzed until all -/// module-level definitions have been analyzed. +/// A collection of AST nodes that are deferred for later visitation. Used to, e.g., store +/// functions, whose bodies shouldn't be visited until all module-level definitions have been +/// visited. #[derive(Debug, Default)] -pub(crate) struct Deferred<'a> { - pub(crate) scopes: Vec, +pub(crate) struct Visit<'a> { pub(crate) string_type_definitions: Vec<(TextRange, &'a str, Snapshot)>, pub(crate) future_type_definitions: Vec<(&'a Expr, Snapshot)>, pub(crate) type_param_definitions: Vec<(&'a Expr, Snapshot)>, pub(crate) functions: Vec, pub(crate) lambdas: Vec, +} + +impl Visit<'_> { + /// Returns `true` if there are no deferred nodes. + pub(crate) fn is_empty(&self) -> bool { + self.string_type_definitions.is_empty() + && self.future_type_definitions.is_empty() + && self.type_param_definitions.is_empty() + && self.functions.is_empty() + && self.lambdas.is_empty() + } +} + +/// A collection of AST nodes to be analyzed after the AST traversal. Used to, e.g., store +/// all `for` loops, so that they can be analyzed after the entire AST has been visited. +#[derive(Debug, Default)] +pub(crate) struct Analyze { + pub(crate) scopes: Vec, + pub(crate) lambdas: Vec, pub(crate) for_loops: Vec, } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index ffa3f6c66972eb..61fe2f63f67528 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -52,14 +52,14 @@ use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind}; use ruff_python_semantic::analyze::{imports, typing, visibility}; use ruff_python_semantic::{ BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module, - ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, Snapshot, - StarImport, SubmoduleImport, + ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport, + SubmoduleImport, }; use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS}; use ruff_source_file::{Locator, OneIndexed, SourceRow}; use crate::checkers::ast::annotation::AnnotationContext; -use crate::checkers::ast::deferred::Deferred; +use crate::checkers::ast::deferred::{Analyze, Visit}; use crate::docstrings::extraction::ExtractionTarget; use crate::importer::Importer; use crate::noqa::NoqaMapping; @@ -105,8 +105,10 @@ pub(crate) struct Checker<'a> { importer: Importer<'a>, /// The [`SemanticModel`], built up over the course of the AST traversal. semantic: SemanticModel<'a>, - /// A set of deferred nodes to be processed after the current traversal (e.g., function bodies). - deferred: Deferred<'a>, + /// A set of deferred nodes to be visited after the current traversal (e.g., function bodies). + visit: Visit<'a>, + /// A set of deferred nodes to be analyzed after the AST traversal (e.g., `for` loops). + analyze: Analyze, /// The cumulative set of diagnostics computed across all lint rules. pub(crate) diagnostics: Vec, /// The list of names already seen by flake8-bugbear diagnostics, to avoid duplicate violations.. @@ -145,7 +147,8 @@ impl<'a> Checker<'a> { indexer, importer, semantic: SemanticModel::new(&settings.typing_modules, path, module), - deferred: Deferred::default(), + visit: Visit::default(), + analyze: Analyze::default(), diagnostics: Vec::default(), flake8_bugbear_seen: Vec::default(), cell_offsets, @@ -596,7 +599,7 @@ where self.semantic.push_scope(ScopeKind::Function(function_def)); self.semantic.flags -= SemanticModelFlags::EXCEPTION_HANDLER; - self.deferred.functions.push(self.semantic.snapshot()); + self.visit.functions.push(self.semantic.snapshot()); // Extract any global bindings from the function body. if let Some(globals) = Globals::from_body(body) { @@ -652,7 +655,7 @@ where if let Some(type_params) = type_params { self.visit_type_params(type_params); } - self.deferred + self.visit .type_param_definitions .push((value, self.semantic.snapshot())); self.semantic.pop_scope(); @@ -785,7 +788,7 @@ where match stmt { Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => { let scope_id = self.semantic.scope_id; - self.deferred.scopes.push(scope_id); + self.analyze.scopes.push(scope_id); self.semantic.pop_scope(); // Function scope self.semantic.pop_definition(); self.semantic.pop_scope(); // Type parameter scope @@ -798,7 +801,7 @@ where } Stmt::ClassDef(ast::StmtClassDef { name, .. }) => { let scope_id = self.semantic.scope_id; - self.deferred.scopes.push(scope_id); + self.analyze.scopes.push(scope_id); self.semantic.pop_scope(); // Class scope self.semantic.pop_definition(); self.semantic.pop_scope(); // Type parameter scope @@ -835,13 +838,13 @@ where && self.semantic.future_annotations() { if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = expr { - self.deferred.string_type_definitions.push(( + self.visit.string_type_definitions.push(( expr.range(), value.to_str(), self.semantic.snapshot(), )); } else { - self.deferred + self.visit .future_type_definitions .push((expr, self.semantic.snapshot())); } @@ -945,7 +948,8 @@ where } self.semantic.push_scope(ScopeKind::Lambda(lambda)); - self.deferred.lambdas.push(self.semantic.snapshot()); + self.visit.lambdas.push(self.semantic.snapshot()); + self.analyze.lambdas.push(self.semantic.snapshot()); } Expr::IfExp(ast::ExprIfExp { test, @@ -1252,7 +1256,7 @@ where } Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { if self.semantic.in_type_definition() && !self.semantic.in_typing_literal() { - self.deferred.string_type_definitions.push(( + self.visit.string_type_definitions.push(( expr.range(), value.to_str(), self.semantic.snapshot(), @@ -1273,7 +1277,7 @@ where | Expr::ListComp(_) | Expr::DictComp(_) | Expr::SetComp(_) => { - self.deferred.scopes.push(self.semantic.scope_id); + self.analyze.scopes.push(self.semantic.scope_id); self.semantic.pop_scope(); } _ => {} @@ -1447,7 +1451,7 @@ where bound: Some(bound), .. }) = type_param { - self.deferred + self.visit .type_param_definitions .push((bound, self.semantic.snapshot())); } @@ -1809,8 +1813,8 @@ impl<'a> Checker<'a> { fn visit_deferred_future_type_definitions(&mut self) { let snapshot = self.semantic.snapshot(); - while !self.deferred.future_type_definitions.is_empty() { - let type_definitions = std::mem::take(&mut self.deferred.future_type_definitions); + while !self.visit.future_type_definitions.is_empty() { + let type_definitions = std::mem::take(&mut self.visit.future_type_definitions); for (expr, snapshot) in type_definitions { self.semantic.restore(snapshot); @@ -1824,8 +1828,8 @@ impl<'a> Checker<'a> { fn visit_deferred_type_param_definitions(&mut self) { let snapshot = self.semantic.snapshot(); - while !self.deferred.type_param_definitions.is_empty() { - let type_params = std::mem::take(&mut self.deferred.type_param_definitions); + while !self.visit.type_param_definitions.is_empty() { + let type_params = std::mem::take(&mut self.visit.type_param_definitions); for (type_param, snapshot) in type_params { self.semantic.restore(snapshot); @@ -1839,8 +1843,8 @@ impl<'a> Checker<'a> { fn visit_deferred_string_type_definitions(&mut self, allocator: &'a typed_arena::Arena) { let snapshot = self.semantic.snapshot(); - while !self.deferred.string_type_definitions.is_empty() { - let type_definitions = std::mem::take(&mut self.deferred.string_type_definitions); + while !self.visit.string_type_definitions.is_empty() { + let type_definitions = std::mem::take(&mut self.visit.string_type_definitions); for (range, value, snapshot) in type_definitions { if let Ok((expr, kind)) = parse_type_annotation(value, range, self.locator.contents()) @@ -1887,8 +1891,8 @@ impl<'a> Checker<'a> { fn visit_deferred_functions(&mut self) { let snapshot = self.semantic.snapshot(); - while !self.deferred.functions.is_empty() { - let deferred_functions = std::mem::take(&mut self.deferred.functions); + while !self.visit.functions.is_empty() { + let deferred_functions = std::mem::take(&mut self.visit.functions); for snapshot in deferred_functions { self.semantic.restore(snapshot); @@ -1906,11 +1910,12 @@ impl<'a> Checker<'a> { self.semantic.restore(snapshot); } + /// Visit all deferred lambdas. Returns a list of snapshots, such that the caller can restore + /// the semantic model to the state it was in before visiting the deferred lambdas. fn visit_deferred_lambdas(&mut self) { let snapshot = self.semantic.snapshot(); - let mut deferred: Vec = Vec::with_capacity(self.deferred.lambdas.len()); - while !self.deferred.lambdas.is_empty() { - let lambdas = std::mem::take(&mut self.deferred.lambdas); + while !self.visit.lambdas.is_empty() { + let lambdas = std::mem::take(&mut self.visit.lambdas); for snapshot in lambdas { self.semantic.restore(snapshot); @@ -1927,15 +1932,23 @@ impl<'a> Checker<'a> { self.visit_parameters(parameters); } self.visit_expr(body); - - deferred.push(snapshot); } } - // Reset the deferred lambdas, so we can analyze them later on. - self.deferred.lambdas = deferred; self.semantic.restore(snapshot); } + /// Recursively visit all deferred AST nodes, including lambdas, functions, and type + /// annotations. + fn visit_deferred(&mut self, allocator: &'a typed_arena::Arena) { + while !self.visit.is_empty() { + self.visit_deferred_functions(); + self.visit_deferred_type_param_definitions(); + self.visit_deferred_lambdas(); + self.visit_deferred_future_type_definitions(); + self.visit_deferred_string_type_definitions(&allocator); + } + } + /// Run any lint rules that operate over the module exports (i.e., members of `__all__`). fn visit_exports(&mut self) { let snapshot = self.semantic.snapshot(); @@ -2043,12 +2056,8 @@ pub(crate) fn check_ast( // Visit any deferred syntax nodes. Take care to visit in order, such that we avoid adding // new deferred nodes after visiting nodes of that kind. For example, visiting a deferred // function can add a deferred lambda, but the opposite is not true. - checker.visit_deferred_functions(); - checker.visit_deferred_type_param_definitions(); - checker.visit_deferred_lambdas(); - checker.visit_deferred_future_type_definitions(); let allocator = typed_arena::Arena::new(); - checker.visit_deferred_string_type_definitions(&allocator); + checker.visit_deferred(&allocator); checker.visit_exports(); // Check docstrings, bindings, and unresolved references. @@ -2060,7 +2069,7 @@ pub(crate) fn check_ast( // Reset the scope to module-level, and check all consumed scopes. checker.semantic.scope_id = ScopeId::global(); - checker.deferred.scopes.push(ScopeId::global()); + checker.analyze.scopes.push(ScopeId::global()); analyze::deferred_scopes(&mut checker); checker.diagnostics diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index 477a72b20cc74d..13552252764b08 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -54,6 +54,7 @@ mod tests { #[test_case(Rule::UnusedImport, Path::new("F401_19.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_20.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_21.py"))] + #[test_case(Rule::UnusedImport, Path::new("F401_22.py"))] #[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.py"))] #[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.ipynb"))] #[test_case(Rule::UndefinedLocalWithImportStar, Path::new("F403.py"))] diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_22.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_22.py.snap new file mode 100644 index 00000000000000..d0b409f39ee0ba --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_22.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/pyflakes/mod.rs +--- +