diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py b/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py index 28d5af1f3be580..bd2a3f2e02eb4b 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py @@ -108,3 +108,33 @@ def f(): def f(): toplevel = tt = 1 + + +def f(provided: int) -> int: + match provided: + case [_, *x]: + pass + + +def f(provided: int) -> int: + match provided: + case x: + pass + + +def f(provided: int) -> int: + match provided: + case Foo(bar) as x: + pass + + +def f(provided: int) -> int: + match provided: + case {"foo": 0, **x}: + pass + + +def f(provided: int) -> int: + match provided: + case {**x}: + pass diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index bbb6940d8a5be2..a3565f4393ce69 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -809,7 +809,6 @@ where let name = alias.asname.as_ref().unwrap_or(&alias.name); self.add_binding( name, - // NOTE: Incorrect, needs to be the range of the alias. alias.identifier(self.locator), BindingKind::FutureImportation, BindingFlags::empty(), @@ -830,7 +829,6 @@ where let qualified_name = &alias.name; self.add_binding( name, - // NOTE: Incorrect, needs to be the range of `name`. alias.identifier(self.locator), BindingKind::SubmoduleImportation(SubmoduleImportation { qualified_name, @@ -842,7 +840,6 @@ where let qualified_name = &alias.name; self.add_binding( name, - // NOTE: Incorrect, needs to be the range of `name`. alias.identifier(self.locator), BindingKind::Importation(Importation { qualified_name }), if alias @@ -1088,7 +1085,6 @@ where self.add_binding( name, - // NOTE: Incorrect, needs to be the range of `name`. alias.identifier(self.locator), BindingKind::FutureImportation, BindingFlags::empty(), @@ -1150,7 +1146,6 @@ where helpers::format_import_from_member(level, module, &alias.name); self.add_binding( name, - // NOTE: Incorrect, needs to be the range of `name`. alias.identifier(self.locator), BindingKind::FromImportation(FromImportation { qualified_name }), if alias @@ -1877,7 +1872,6 @@ where self.add_binding( name, - // NOTE: Correct! stmt.identifier(self.locator), BindingKind::FunctionDefinition, BindingFlags::empty(), @@ -2101,7 +2095,6 @@ where self.semantic.pop_definition(); self.add_binding( name, - // NOTE: Correct! stmt.identifier(self.locator), BindingKind::ClassDefinition, BindingFlags::empty(), @@ -3931,7 +3924,6 @@ where // Add the bound exception name to the scope. let binding_id = self.add_binding( name, - // NOTE: Correct, we already extract this. range, BindingKind::Assignment, BindingFlags::empty(), @@ -3942,7 +3934,6 @@ where // Remove it from the scope immediately after. self.add_binding( name, - // NOTE: Correct range, BindingKind::UnboundException, BindingFlags::empty(), @@ -4056,7 +4047,9 @@ where fn visit_pattern(&mut self, pattern: &'b Pattern) { if let Pattern::MatchAs(ast::PatternMatchAs { - name: Some(name), .. + // If there's a pattern, then there's an alias; skip pattern, skip `as`, take alias. + name: Some(name), + .. }) | Pattern::MatchStar(ast::PatternMatchStar { name: Some(name), @@ -4068,9 +4061,7 @@ where { self.add_binding( name, - // TODO(charlie): This needs to use the range of the identifier, not the range of - // the pattern. - pattern.range(), + pattern.identifier(self.locator), BindingKind::Assignment, BindingFlags::empty(), ); @@ -4447,7 +4438,6 @@ impl<'a> Checker<'a> { ) { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::Annotation, BindingFlags::empty(), @@ -4458,7 +4448,6 @@ impl<'a> Checker<'a> { if matches!(parent, Stmt::For(_) | Stmt::AsyncFor(_)) { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::LoopVar, BindingFlags::empty(), @@ -4469,7 +4458,6 @@ impl<'a> Checker<'a> { if helpers::is_unpacking_assignment(parent, expr) { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::UnpackedAssignment, BindingFlags::empty(), @@ -4523,7 +4511,6 @@ impl<'a> Checker<'a> { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::Export(Export { names }), BindingFlags::empty(), @@ -4538,7 +4525,6 @@ impl<'a> Checker<'a> { { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::NamedExprAssignment, BindingFlags::empty(), @@ -4548,7 +4534,6 @@ impl<'a> Checker<'a> { self.add_binding( id, - // NOTE: Correct, range of the name. expr.range(), BindingKind::Assignment, BindingFlags::empty(), diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_3.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_3.py.snap index fbf428003b2899..be49b031196d95 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_3.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_3.py.snap @@ -502,6 +502,9 @@ F841_3.py:110:5: F841 [*] Local variable `toplevel` is assigned to but never use 109 109 | def f(): 110 |- toplevel = tt = 1 110 |+ tt = 1 +111 111 | +112 112 | +113 113 | def f(provided: int) -> int: F841_3.py:110:16: F841 [*] Local variable `tt` is assigned to but never used | @@ -517,5 +520,68 @@ F841_3.py:110:16: F841 [*] Local variable `tt` is assigned to but never used 109 109 | def f(): 110 |- toplevel = tt = 1 110 |+ toplevel = 1 +111 111 | +112 112 | +113 113 | def f(provided: int) -> int: + +F841_3.py:115:19: F841 Local variable `x` is assigned to but never used + | +113 | def f(provided: int) -> int: +114 | match provided: +115 | case [_, *x]: + | ^ F841 +116 | pass + | + = help: Remove assignment to unused variable `x` + +F841_3.py:121:14: F841 Local variable `x` is assigned to but never used + | +119 | def f(provided: int) -> int: +120 | match provided: +121 | case x: + | ^ F841 +122 | pass + | + = help: Remove assignment to unused variable `x` + +F841_3.py:127:18: F841 Local variable `bar` is assigned to but never used + | +125 | def f(provided: int) -> int: +126 | match provided: +127 | case Foo(bar) as x: + | ^^^ F841 +128 | pass + | + = help: Remove assignment to unused variable `bar` + +F841_3.py:127:26: F841 Local variable `x` is assigned to but never used + | +125 | def f(provided: int) -> int: +126 | match provided: +127 | case Foo(bar) as x: + | ^ F841 +128 | pass + | + = help: Remove assignment to unused variable `x` + +F841_3.py:133:27: F841 Local variable `x` is assigned to but never used + | +131 | def f(provided: int) -> int: +132 | match provided: +133 | case {"foo": 0, **x}: + | ^ F841 +134 | pass + | + = help: Remove assignment to unused variable `x` + +F841_3.py:139:17: F841 Local variable `x` is assigned to but never used + | +137 | def f(provided: int) -> int: +138 | match provided: +139 | case {**x}: + | ^ F841 +140 | pass + | + = help: Remove assignment to unused variable `x` diff --git a/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs b/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs index 2073339f69cbb2..ab6f737cfb94a2 100644 --- a/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs +++ b/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs @@ -2,7 +2,7 @@ use rustpython_parser::ast::{self, Excepthandler, MatchCase, Stmt}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers; +use ruff_python_ast::identifier; use crate::checkers::ast::Checker; @@ -102,7 +102,7 @@ pub(crate) fn useless_else_on_loop( if !orelse.is_empty() && !loop_exits_early(body) { checker.diagnostics.push(Diagnostic::new( UselessElseOnLoop, - helpers::else_range(stmt, checker.locator).unwrap(), + identifier::else_(stmt, checker.locator).unwrap(), )); } } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 733e6dd0da4d8e..532f1deebeb26b 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1069,30 +1069,6 @@ pub fn match_parens(start: TextSize, locator: &Locator) -> Option { } } -/// Return the `Range` of `else` in `For`, `AsyncFor`, and `While` statements. -pub fn else_range(stmt: &Stmt, locator: &Locator) -> Option { - match stmt { - Stmt::For(ast::StmtFor { body, orelse, .. }) - | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) - | Stmt::While(ast::StmtWhile { body, orelse, .. }) - if !orelse.is_empty() => - { - let body_end = body.last().expect("Expected body to be non-empty").end(); - let or_else_start = orelse - .first() - .expect("Expected orelse to be non-empty") - .start(); - let contents = &locator.contents()[TextRange::new(body_end, or_else_start)]; - - lexer::lex_starts_at(contents, Mode::Module, body_end) - .flatten() - .find(|(kind, _)| matches!(kind, Tok::Else)) - .map(|(_, range)| range) - } - _ => None, - } -} - /// Return the `Range` of the first `Tok::Colon` token in a `Range`. pub fn first_colon_range(range: TextRange, locator: &Locator) -> Option { let contents = &locator.contents()[range]; @@ -1521,13 +1497,14 @@ mod tests { use std::borrow::Cow; use anyhow::Result; - use ruff_text_size::TextSize; - use rustpython_ast::{Cmpop, Expr}; + use ruff_text_size::{TextLen, TextRange, TextSize}; + use rustpython_ast::{Cmpop, Expr, Stmt}; use rustpython_parser::ast::Suite; use rustpython_parser::Parse; use crate::helpers::{ - has_trailing_content, locate_cmpops, resolve_imported_module_path, LocatedCmpop, + elif_else_range, first_colon_range, has_trailing_content, locate_cmpops, + resolve_imported_module_path, LocatedCmpop, }; use crate::source_code::Locator; @@ -1607,6 +1584,48 @@ y = 2 ); } + #[test] + fn extract_first_colon_range() { + let contents = "with a: pass"; + let locator = Locator::new(contents); + let range = first_colon_range( + TextRange::new(TextSize::from(0), contents.text_len()), + &locator, + ) + .unwrap(); + assert_eq!(&contents[range], ":"); + assert_eq!(range, TextRange::new(TextSize::from(6), TextSize::from(7))); + } + + #[test] + fn extract_elif_else_range() -> Result<()> { + let contents = "if a: + ... +elif b: + ... +"; + let stmt = Stmt::parse(contents, "")?; + let stmt = Stmt::as_if_stmt(&stmt).unwrap(); + let locator = Locator::new(contents); + let range = elif_else_range(stmt, &locator).unwrap(); + assert_eq!(range.start(), TextSize::from(14)); + assert_eq!(range.end(), TextSize::from(18)); + + let contents = "if a: + ... +else: + ... +"; + let stmt = Stmt::parse(contents, "")?; + let stmt = Stmt::as_if_stmt(&stmt).unwrap(); + let locator = Locator::new(contents); + let range = elif_else_range(stmt, &locator).unwrap(); + assert_eq!(range.start(), TextSize::from(14)); + assert_eq!(range.end(), TextSize::from(18)); + + Ok(()) + } + #[test] fn extract_cmpop_location() -> Result<()> { let contents = "x == 1"; diff --git a/crates/ruff_python_ast/src/identifier.rs b/crates/ruff_python_ast/src/identifier.rs index e3e30b70415225..97e88e7d8df5d5 100644 --- a/crates/ruff_python_ast/src/identifier.rs +++ b/crates/ruff_python_ast/src/identifier.rs @@ -20,7 +20,7 @@ use std::ops::{Add, Sub}; use std::str::Chars; use ruff_text_size::{TextLen, TextRange, TextSize}; -use rustpython_ast::{Alias, Arg}; +use rustpython_ast::{Alias, Arg, Pattern}; use rustpython_parser::ast::{self, Excepthandler, Ranged, Stmt}; use ruff_python_whitespace::is_python_whitespace; @@ -123,6 +123,110 @@ impl Identifier for Alias { } } +impl Identifier for Pattern { + /// Return the [`TextRange`] of the identifier in the given pattern. + /// + /// For example, return the range of `z` in: + /// ```python + /// match x: + /// # Pattern::MatchAs + /// case z: + /// ... + /// ``` + /// + /// Or: + /// ```python + /// match x: + /// # Pattern::MatchAs + /// case y as z: + /// ... + /// ``` + /// + /// Or : + /// ```python + /// match x: + /// # Pattern::MatchMapping + /// case {"a": 1, **z} + /// ... + /// ``` + /// + /// Or : + /// ```python + /// match x: + /// # Pattern::MatchStar + /// case *z: + /// ... + /// ``` + fn identifier(&self, locator: &Locator) -> TextRange { + match self { + Pattern::MatchAs(ast::PatternMatchAs { + name: Some(_), + pattern, + range, + }) => { + if let Some(pattern) = pattern { + // Identify `z` in: + // ```python + // match x: + // case Foo(bar) as z: + // ... + // ``` + IdentifierTokenizer::starts_at(pattern.end(), locator.contents()) + .nth(1) + .expect("Unable to identify identifier in pattern") + } else { + // Identify `z` in: + // ```python + // match x: + // case z: + // ... + // ``` + *range + } + } + Pattern::MatchMapping(ast::PatternMatchMapping { + patterns, + rest: Some(_), + .. + }) => { + if let Some(pattern) = patterns.last() { + // Identify `z` in: + // ```python + // match x: + // case {"a": 1, **z} + // ... + // ``` + IdentifierTokenizer::starts_at(pattern.end(), locator.contents()) + .next() + .expect("Unable to identify identifier in pattern") + } else { + // Identify `z` in: + // ```python + // match x: + // case {**z} + // ... + // ``` + IdentifierTokenizer::starts_at(self.start(), locator.contents()) + .next() + .expect("Unable to identify identifier in pattern") + } + } + Pattern::MatchStar(ast::PatternMatchStar { name: Some(_), .. }) => { + // Identify `z` in: + // ```python + // match x: + // case *z: + // ... + // ``` + IdentifierTokenizer::starts_at(self.start(), locator.contents()) + .next() + .expect("Unable to identify identifier in pattern") + } + _ => self.range(), + } + } +} + impl Identifier for Excepthandler { /// Return the [`TextRange`] of a named exception in an [`Excepthandler`]. /// @@ -161,6 +265,25 @@ pub fn except(handler: &Excepthandler, locator: &Locator) -> TextRange { .expect("Failed to find `except` token in `Excepthandler`") } +/// Return the [`TextRange`] of the `else` token in a `For`, `AsyncFor`, or `While` statement. +pub fn else_(stmt: &Stmt, locator: &Locator) -> Option { + let (Stmt::For(ast::StmtFor { body, orelse, .. }) + | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) + | Stmt::While(ast::StmtWhile { body, orelse, .. })) = stmt else { + return None; + }; + + if orelse.is_empty() { + return None; + } + + IdentifierTokenizer::starts_at( + body.last().expect("Expected body to be non-empty").end(), + locator.contents(), + ) + .next() +} + /// Return `true` if the given character is a valid identifier character. fn is_python_identifier(c: char) -> bool { c.is_alphanumeric() || c == '_' || c == '.' @@ -168,10 +291,14 @@ fn is_python_identifier(c: char) -> bool { /// Simple zero allocation tokenizer for Python identifiers. /// -/// The tokenizer must start at an offset that is trivia (e.g. not inside of a multiline string). +/// The tokenizer must operate over a range that can only contain identifiers, keywords, and +/// comments (along with whitespace and continuation characters). It does not support other tokens, +/// like operators, literals, or delimiters. It also does not differentiate between keywords and +/// identifiers, treating every valid token as an "identifier". /// -/// The tokenizer doesn't guarantee any correctness after it returned a [`TokenKind::Other`]. That's why it -/// will return [`TokenKind::Bogus`] for every character after until it reaches the end of the file. +/// This is useful for cases like, e.g., identifying the alias name in an aliased import (`bar` in +/// `import foo as bar`), where we're guaranteed to only have identifiers and keywords in the +/// relevant range. pub(crate) struct IdentifierTokenizer<'a> { cursor: Cursor<'a>, offset: TextSize, @@ -300,11 +427,11 @@ impl<'a> Cursor<'a> { #[cfg(test)] mod tests { use anyhow::Result; - use ruff_text_size::{TextLen, TextRange, TextSize}; + use ruff_text_size::{TextRange, TextSize}; use rustpython_ast::Stmt; use rustpython_parser::Parse; - use crate::helpers::{elif_else_range, else_range, first_colon_range}; + use crate::identifier; use crate::identifier::Identifier; use crate::source_code::Locator; @@ -447,7 +574,7 @@ else: .trim(); let stmt = Stmt::parse(contents, "")?; let locator = Locator::new(contents); - let range = else_range(&stmt, &locator).unwrap(); + let range = identifier::else_(&stmt, &locator).unwrap(); assert_eq!(&contents[range], "else"); assert_eq!( range, @@ -455,46 +582,4 @@ else: ); Ok(()) } - - #[test] - fn extract_first_colon_range() { - let contents = "with a: pass"; - let locator = Locator::new(contents); - let range = first_colon_range( - TextRange::new(TextSize::from(0), contents.text_len()), - &locator, - ) - .unwrap(); - assert_eq!(&contents[range], ":"); - assert_eq!(range, TextRange::new(TextSize::from(6), TextSize::from(7))); - } - - #[test] - fn extract_elif_else_range() -> Result<()> { - let contents = "if a: - ... -elif b: - ... -"; - let stmt = Stmt::parse(contents, "")?; - let stmt = Stmt::as_if_stmt(&stmt).unwrap(); - let locator = Locator::new(contents); - let range = elif_else_range(stmt, &locator).unwrap(); - assert_eq!(range.start(), TextSize::from(14)); - assert_eq!(range.end(), TextSize::from(18)); - - let contents = "if a: - ... -else: - ... -"; - let stmt = Stmt::parse(contents, "")?; - let stmt = Stmt::as_if_stmt(&stmt).unwrap(); - let locator = Locator::new(contents); - let range = elif_else_range(stmt, &locator).unwrap(); - assert_eq!(range.start(), TextSize::from(14)); - assert_eq!(range.end(), TextSize::from(18)); - - Ok(()) - } }