diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/control_var_used_after_block.py b/crates/ruff_linter/resources/test/fixtures/pylint/control_var_used_after_block.py index 8ffa20a9bc32e..25b68dbd1e053 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/control_var_used_after_block.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/control_var_used_after_block.py @@ -1,36 +1,49 @@ import typing from typing import cast +# TODO: Figure this out +# for global_var in []: +# _ = global_var +# pass + +# _ = global_var + def foo(): # For control var used outside block for event in []: + _ = event pass _ = event # # Tuple destructuring - # for a, b, c in []: - # pass + for a, b, c in []: + pass - # _ = a - # _ = b - # _ = c + _ = a + _ = b + _ = c # # Array destructuring - # for [d, e, f] in []: - # pass - - # _ = d - # _ = e - # _ = f - - # # With -> for, variable reused - # with None as i: - # for i in []: # error - # pass - - # # For -> with, variable reused - # for i in []: - # with None as i: # error - # pass + for [d, e, f] in []: + pass + + _ = d + _ = e + _ = f + + with None as i: + pass + + _ = i + + with None as i: + for n in []: + pass + + _ = n + + _ = n + + 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 c24fd4a4bf279..41e4d3285c749 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -87,6 +87,12 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { for scope_id in checker.analyze.scopes.iter().rev().copied() { let scope = &checker.semantic.scopes[scope_id]; + // TODO: Put in the right place + // TODO: How to make this run with the global scope outside of a function? + if checker.enabled(Rule::ControlVarUsedAfterBlock) { + pylint::rules::control_var_used_after_block(checker, scope, &mut diagnostics); + } + if checker.enabled(Rule::UndefinedLocal) { pyflakes::rules::undefined_local(checker, scope_id, scope, &mut diagnostics); } @@ -346,11 +352,6 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if checker.enabled(Rule::UnusedVariable) { pyflakes::rules::unused_variable(checker, scope, &mut diagnostics); } - // TODO: Put in the right place - // TODO: How to make this run with the global scope outside of a function? - if checker.enabled(Rule::ControlVarUsedAfterBlock) { - pylint::rules::control_var_used_after_block(checker, scope); - } if checker.enabled(Rule::UnusedAnnotation) { pyflakes::rules::unused_annotation(checker, scope, &mut diagnostics); diff --git a/crates/ruff_linter/src/rules/pylint/rules/control_var_used_after_block.rs b/crates/ruff_linter/src/rules/pylint/rules/control_var_used_after_block.rs index 6dfcb43f9447f..36fb600cc8265 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/control_var_used_after_block.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/control_var_used_after_block.rs @@ -1,13 +1,14 @@ use std::{fmt, iter}; use regex::Regex; +use ruff_python_ast::identifier::Identifier; use ruff_python_ast::{self as ast, Arguments, Expr, ExprContext, Stmt, WithItem}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor}; -use ruff_python_semantic::{Binding, Scope, SemanticModel}; +use ruff_python_semantic::{Binding, NodeId, Scope, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -98,12 +99,18 @@ impl fmt::Display for BlockKind { } /// WPS441: Forbid control variables after the block body. -pub(crate) fn control_var_used_after_block(checker: &Checker, scope: &Scope) { - println!("qwerqwere control_var_used_after_block"); +pub(crate) fn control_var_used_after_block( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + println!("Running control_var_used_after_block"); // if scope.uses_locals() && scope.kind.is_function() { // return; // } + println!("nodes {:#?}", checker.semantic().nodes()); + for (name, binding) in scope .bindings() .map(|(name, binding_id)| (name, checker.semantic().binding(binding_id))) @@ -130,10 +137,71 @@ pub(crate) fn control_var_used_after_block(checker: &Checker, scope: &Scope) { // None // }) { - println!("asdf {:?} {:?}", name, binding); - println!("asdfasdf {:?}", binding.references); + println!("Binding {:?} {:?}", name, binding); + // Find for-loop variable bindings + let binding_statement = binding.statement(checker.semantic()).unwrap(); + let binding_source_node_id = binding.source.unwrap(); + // The node_id of the for-loop that contains the binding + // let binding_statement_id = checker + // .semantic() + // .parent_statement_id(binding_source_node_id) + // .unwrap(); + let binding_statement_id = checker.semantic().statement_id(binding_source_node_id); + + println!("binding_statement={:?}", binding_statement); + println!("Binding references {:?}", binding.references); + + // Loop over the references of those bindings to see if they're in the same block-scope for reference in binding.references() { - println!("asdfasdfasdf {:?}", checker.semantic().reference(reference)); + let reference = checker.semantic().reference(reference); + let reference_node_id = reference.expression_id().unwrap(); + + // Traverse the hierarchy and look for a block match + let statement_hierarchy: Vec = checker + .semantic() + .parent_statement_ids(reference_node_id) + .collect(); + + println!( + "\t\x1b[32mreference\x1b[0m {:?} <- {:?}", + reference, statement_hierarchy + ); + + let mut found_match = false; + for ancestor_node_id in statement_hierarchy { + let ancestor_statement = checker.semantic().statement(ancestor_node_id); + println!("\t\tancestor_statement={:?}", ancestor_statement); + println!( + "\t\tbinding_source_node_id={:?} binding_statement_id={:?} ancestor_node_id={:?}", + binding_source_node_id, binding_statement_id, ancestor_node_id + ); + if (binding_statement_id == ancestor_node_id) { + println!("\t\to"); + found_match = true; + break; + } else { + println!("\t\tx"); + } + } + + if (!found_match) { + println!("\t\temits error={:?}", reference_node_id); + let block_kind = match binding_statement { + Stmt::For(_) => BlockKind::For, + Stmt::With(_) => BlockKind::With, + _ => { + panic!("unexpected block item") + } + }; + + diagnostics.push(Diagnostic::new( + ControlVarUsedAfterBlock { + control_var_name: name.to_owned(), + block_kind: block_kind, + }, + reference.range(), + )) + } // TODO: Look if the reference is under the same block as the binding // (see `too_many_nested_blocks` for an example of how to work with blocks) diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 5bfa87821c1a1..b076f80537461 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1144,6 +1144,12 @@ impl<'a> SemanticModel<'a> { None } + /// TODO + #[inline] + pub fn nodes(&self) -> &Nodes<'a> { + &self.nodes + } + /// Return the [`Stmt`] corresponding to the given [`NodeId`]. #[inline] pub fn node(&self, node_id: NodeId) -> &NodeRef<'a> { @@ -1176,6 +1182,15 @@ impl<'a> SemanticModel<'a> { .expect("No statement found") } + /// Return the [`Stmt`] corresponding to the given [`NodeId`]. + #[inline] + pub fn statement_id(&self, node_id: NodeId) -> NodeId { + self.nodes + .ancestor_ids(node_id) + .find(|id| self.nodes[*id].is_statement()) + .expect("No statement found") + } + /// Given a [`Stmt`], return its parent, if any. #[inline] pub fn parent_statement(&self, node_id: NodeId) -> Option<&'a Stmt> { @@ -1193,6 +1208,13 @@ impl<'a> SemanticModel<'a> { .nth(1) } + /// Given a [`NodeId`], return the [`NodeId`] of the parent statement, if any. + pub fn parent_statement_ids(&self, node_id: NodeId) -> impl Iterator + '_ { + self.nodes + .ancestor_ids(node_id) + .filter(|id| self.nodes[*id].is_statement()) + } + /// Return the [`Expr`] corresponding to the given [`NodeId`]. #[inline] pub fn expression(&self, node_id: NodeId) -> Option<&'a Expr> {