Include decorators in Function and Class definition ranges (#4467)

Cargo.lock

@@ -2000,7 +2000,7 @@ dependencies = [
 [[package]]
 name = "ruff_text_size"
 version = "0.0.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "schemars",
  "serde",
@@ -2071,7 +2071,7 @@ dependencies = [
 [[package]]
 name = "rustpython-ast"
 version = "0.2.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "is-macro",
  "num-bigint",
@@ -2082,7 +2082,7 @@ dependencies = [
 [[package]]
 name = "rustpython-format"
 version = "0.2.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "bitflags 2.3.1",
  "itertools",
@@ -2094,7 +2094,7 @@ dependencies = [
 [[package]]
 name = "rustpython-literal"
 version = "0.2.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "hexf-parse",
  "is-macro",
@@ -2106,7 +2106,7 @@ dependencies = [
 [[package]]
 name = "rustpython-parser"
 version = "0.2.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "anyhow",
  "is-macro",
@@ -2129,7 +2129,7 @@ dependencies = [
 [[package]]
 name = "rustpython-parser-core"
 version = "0.2.0"
-source = "git+https://github.com/RustPython/Parser.git?rev=e1f02fced740cdd99593ae4b14bcfcac725e9e02#e1f02fced740cdd99593ae4b14bcfcac725e9e02"
+source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=335780aeeac1e6fcd85994ba001d7b8ce99fcf65#335780aeeac1e6fcd85994ba001d7b8ce99fcf65"
 dependencies = [
  "is-macro",
  "ruff_text_size",

Cargo.toml

@@ -31,10 +31,10 @@ proc-macro2 = { version = "1.0.51" }
 quote = { version = "1.0.23" }
 regex = { version = "1.7.1" }
 rustc-hash = { version = "1.1.0" }
-ruff_text_size = { git = "https://github.com/RustPython/Parser.git", rev = "e1f02fced740cdd99593ae4b14bcfcac725e9e02" }
+ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "335780aeeac1e6fcd85994ba001d7b8ce99fcf65" }
-rustpython-format = { git = "https://github.com/RustPython/Parser.git", rev = "e1f02fced740cdd99593ae4b14bcfcac725e9e02" }
+rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "335780aeeac1e6fcd85994ba001d7b8ce99fcf65" }
-rustpython-literal = { git = "https://github.com/RustPython/Parser.git", rev = "e1f02fced740cdd99593ae4b14bcfcac725e9e02" }
+rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "335780aeeac1e6fcd85994ba001d7b8ce99fcf65" }
-rustpython-parser = { git = "https://github.com/RustPython/Parser.git", rev = "e1f02fced740cdd99593ae4b14bcfcac725e9e02", default-features = false, features = ["full-lexer", "all-nodes-with-ranges"] }
+rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "335780aeeac1e6fcd85994ba001d7b8ce99fcf65", default-features = false, features = ["full-lexer", "all-nodes-with-ranges"] }
 schemars = { version = "0.8.12" }
 serde = { version = "1.0.152", features = ["derive"] }
 serde_json = { version = "1.0.93", features = ["preserve_order"] }

crates/ruff/src/checkers/ast/mod.rs

@@ -41,6 +41,7 @@ use crate::fs::relativize_path;
 use crate::importer::Importer;
 use crate::noqa::NoqaMapping;
 use crate::registry::{AsRule, Rule};
+use crate::rules::flake8_builtins::rules::AnyShadowing;
 use crate::rules::{
     flake8_2020, flake8_annotations, flake8_async, flake8_bandit, flake8_blind_except,
     flake8_boolean_trap, flake8_bugbear, flake8_builtins, flake8_comprehensions, flake8_datetimez,
@@ -626,11 +627,19 @@ where
 
                 if self.semantic_model.scope().kind.is_class() {
                     if self.settings.rules.enabled(Rule::BuiltinAttributeShadowing) {
-                        flake8_builtins::rules::builtin_attribute_shadowing(self, name, stmt);
+                        flake8_builtins::rules::builtin_attribute_shadowing(
+                            self,
+                            name,
+                            AnyShadowing::from(stmt),
+                        );
                     }
                 } else {
                     if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) {
-                        flake8_builtins::rules::builtin_variable_shadowing(self, name, stmt);
+                        flake8_builtins::rules::builtin_variable_shadowing(
+                            self,
+                            name,
+                            AnyShadowing::from(stmt),
+                        );
                     }
                 }
             }
@@ -804,7 +813,11 @@ where
                 }
 
                 if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) {
-                    flake8_builtins::rules::builtin_variable_shadowing(self, name, stmt);
+                    flake8_builtins::rules::builtin_variable_shadowing(
+                        self,
+                        name,
+                        AnyShadowing::from(stmt),
+                    );
                 }
 
                 if self.settings.rules.enabled(Rule::DuplicateBases) {
@@ -921,7 +934,9 @@ where
                         if let Some(asname) = &alias.asname {
                             if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) {
                                 flake8_builtins::rules::builtin_variable_shadowing(
-                                    self, asname, stmt,
+                                    self,
+                                    asname,
+                                    AnyShadowing::from(stmt),
                                 );
                             }
                         }
@@ -1228,7 +1243,9 @@ where
                         if let Some(asname) = &alias.asname {
                             if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) {
                                 flake8_builtins::rules::builtin_variable_shadowing(
-                                    self, asname, stmt,
+                                    self,
+                                    asname,
+                                    AnyShadowing::from(stmt),
                                 );
                             }
                         }
@@ -2526,11 +2543,19 @@ where
 
                         if self.semantic_model.scope().kind.is_class() {
                             if self.settings.rules.enabled(Rule::BuiltinAttributeShadowing) {
-                                flake8_builtins::rules::builtin_attribute_shadowing(self, id, expr);
+                                flake8_builtins::rules::builtin_attribute_shadowing(
+                                    self,
+                                    id,
+                                    AnyShadowing::from(expr),
+                                );
                             }
                         } else {
                             if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) {
-                                flake8_builtins::rules::builtin_variable_shadowing(self, id, expr);
+                                flake8_builtins::rules::builtin_variable_shadowing(
+                                    self,
+                                    id,
+                                    AnyShadowing::from(expr),
+                                );
                             }
                         }
 
@@ -4323,7 +4348,7 @@ where
                             flake8_builtins::rules::builtin_variable_shadowing(
                                 self,
                                 name,
-                                excepthandler,
+                                AnyShadowing::from(excepthandler),
                             );
                         }
 
@@ -4481,7 +4506,7 @@ where
         }
 
         if self.settings.rules.enabled(Rule::BuiltinArgumentShadowing) {
-            flake8_builtins::rules::builtin_argument_shadowing(self, &arg.arg, arg);
+            flake8_builtins::rules::builtin_argument_shadowing(self, arg);
         }
     }
 
@@ -4686,26 +4711,18 @@ impl<'a> Checker<'a> {
                     {
                         if self.settings.rules.enabled(Rule::RedefinedWhileUnused) {
                             #[allow(deprecated)]
-                            let line = self.locator.compute_line_index(existing.range.start());
+                            let line = self.locator.compute_line_index(
+                                existing
+                                    .trimmed_range(self.semantic_model(), self.locator)
+                                    .start(),
+                            );
 
                             let mut diagnostic = Diagnostic::new(
                                 pyflakes::rules::RedefinedWhileUnused {
                                     name: name.to_string(),
                                     line,
                                 },
-                                matches!(
+                                binding.trimmed_range(self.semantic_model(), self.locator),
-                                    binding.kind,
-                                    BindingKind::ClassDefinition | BindingKind::FunctionDefinition
-                                )
-                                .then(|| {
-                                    binding.source.map_or(binding.range, |source| {
-                                        helpers::identifier_range(
-                                            self.semantic_model.stmts[source],
-                                            self.locator,
-                                        )
-                                    })
-                                })
-                                .unwrap_or(binding.range),
                             );
                             if let Some(parent) = binding.source {
                                 let parent = self.semantic_model.stmts[parent];
@@ -5466,27 +5483,18 @@ impl<'a> Checker<'a> {
                             for index in indices {
                                 let rebound = &self.semantic_model.bindings[*index];
                                 #[allow(deprecated)]
-                                let line = self.locator.compute_line_index(binding.range.start());
+                                let line = self.locator.compute_line_index(
+                                    binding
+                                        .trimmed_range(self.semantic_model(), self.locator)
+                                        .start(),
+                                );
 
                                 let mut diagnostic = Diagnostic::new(
                                     pyflakes::rules::RedefinedWhileUnused {
                                         name: (*name).to_string(),
                                         line,
                                     },
-                                    matches!(
+                                    rebound.trimmed_range(self.semantic_model(), self.locator),
-                                        rebound.kind,
-                                        BindingKind::ClassDefinition
-                                            | BindingKind::FunctionDefinition
-                                    )
-                                    .then(|| {
-                                        rebound.source.map_or(rebound.range, |source| {
-                                            helpers::identifier_range(
-                                                self.semantic_model.stmts[source],
-                                                self.locator,
-                                            )
-                                        })
-                                    })
-                                    .unwrap_or(rebound.range),
                                 );
                                 if let Some(source) = rebound.source {
                                     let parent = &self.semantic_model.stmts[source];

crates/ruff/src/rules/flake8_builtins/rules.rs

@@ -1,8 +1,11 @@
-use rustpython_parser::ast::Ranged;
+use ruff_text_size::TextRange;
+use rustpython_parser::ast::{Arg, Excepthandler, Expr, Ranged, Stmt};
 
 use ruff_diagnostics::Diagnostic;
 use ruff_diagnostics::Violation;
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::helpers::identifier_range;
+use ruff_python_ast::source_code::Locator;
 use ruff_python_stdlib::builtins::BUILTINS;
 
 use crate::checkers::ast::Checker;
@@ -172,46 +175,83 @@ fn shadows_builtin(name: &str, ignorelist: &[String]) -> bool {
 }
 
 /// A001
-pub(crate) fn builtin_variable_shadowing<T>(checker: &mut Checker, name: &str, attributed: &T)
+pub(crate) fn builtin_variable_shadowing(
-where
+    checker: &mut Checker,
-    T: Ranged,
+    name: &str,
-{
+    shadowing: AnyShadowing,
+) {
     if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
         checker.diagnostics.push(Diagnostic::new(
             BuiltinVariableShadowing {
                 name: name.to_string(),
             },
-            attributed.range(),
+            shadowing.range(checker.locator),
         ));
     }
 }
 
 /// A002
-pub(crate) fn builtin_argument_shadowing<T>(checker: &mut Checker, name: &str, attributed: &T)
+pub(crate) fn builtin_argument_shadowing(checker: &mut Checker, argument: &Arg) {
-where
+    if shadows_builtin(
-    T: Ranged,
+        argument.arg.as_str(),
-{
+        &checker.settings.flake8_builtins.builtins_ignorelist,
-    if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
+    ) {
         checker.diagnostics.push(Diagnostic::new(
             BuiltinArgumentShadowing {
-                name: name.to_string(),
+                name: argument.arg.to_string(),
             },
-            attributed.range(),
+            argument.range(),
         ));
     }
 }
 
 /// A003
-pub(crate) fn builtin_attribute_shadowing<T>(checker: &mut Checker, name: &str, attributed: &T)
+pub(crate) fn builtin_attribute_shadowing(
-where
+    checker: &mut Checker,
-    T: Ranged,
+    name: &str,
-{
+    shadowing: AnyShadowing,
+) {
     if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
         checker.diagnostics.push(Diagnostic::new(
             BuiltinAttributeShadowing {
                 name: name.to_string(),
             },
-            attributed.range(),
+            shadowing.range(checker.locator),
         ));
     }
 }
+
+#[derive(Debug, Copy, Clone, PartialEq)]
+pub(crate) enum AnyShadowing<'a> {
+    Expression(&'a Expr),
+    Statement(&'a Stmt),
+    ExceptHandler(&'a Excepthandler),
+}
+
+impl AnyShadowing<'_> {
+    fn range(self, locator: &Locator) -> TextRange {
+        match self {
+            AnyShadowing::Expression(expr) => expr.range(),
+            AnyShadowing::Statement(stmt) => identifier_range(stmt, locator),
+            AnyShadowing::ExceptHandler(handler) => handler.range(),
+        }
+    }
+}
+
+impl<'a> From<&'a Stmt> for AnyShadowing<'a> {
+    fn from(value: &'a Stmt) -> Self {
+        AnyShadowing::Statement(value)
+    }
+}
+
+impl<'a> From<&'a Expr> for AnyShadowing<'a> {
+    fn from(value: &'a Expr) -> Self {
+        AnyShadowing::Expression(value)
+    }
+}
+
+impl<'a> From<&'a Excepthandler> for AnyShadowing<'a> {
+    fn from(value: &'a Excepthandler) -> Self {
+        AnyShadowing::ExceptHandler(value)
+    }
+}

crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A001_A001.py.snap

@@ -104,26 +104,22 @@ A001.py:11:1: A001 Variable `id` is shadowing a Python builtin
 15 | def bytes():
    |
 
-A001.py:13:1: A001 Variable `bytes` is shadowing a Python builtin
+A001.py:13:5: A001 Variable `bytes` is shadowing a Python builtin
    |
 13 | id = 4
 14 | 
-15 | / def bytes():
+15 | def bytes():
-16 | |     pass
+   |     ^^^^^ A001
-   | |________^ A001
+16 |     pass
-17 |   
-18 |   class slice:
    |
 
-A001.py:16:1: A001 Variable `slice` is shadowing a Python builtin
+A001.py:16:7: A001 Variable `slice` is shadowing a Python builtin
    |
 16 |     pass
 17 | 
-18 | / class slice:
+18 | class slice:
-19 | |     pass
+   |       ^^^^^ A001
-   | |________^ A001
+19 |     pass
-20 |   
-21 |   try:
    |
 
 A001.py:21:1: A001 Variable `ValueError` is shadowing a Python builtin

crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A001_A001.py_builtins_ignorelist.snap

@@ -84,26 +84,22 @@ A001.py:9:6: A001 Variable `max` is shadowing a Python builtin
 13 | id = 4
    |
 
-A001.py:13:1: A001 Variable `bytes` is shadowing a Python builtin
+A001.py:13:5: A001 Variable `bytes` is shadowing a Python builtin
    |
 13 | id = 4
 14 | 
-15 | / def bytes():
+15 | def bytes():
-16 | |     pass
+   |     ^^^^^ A001
-   | |________^ A001
+16 |     pass
-17 |   
-18 |   class slice:
    |
 
-A001.py:16:1: A001 Variable `slice` is shadowing a Python builtin
+A001.py:16:7: A001 Variable `slice` is shadowing a Python builtin
    |
 16 |     pass
 17 | 
-18 | / class slice:
+18 | class slice:
-19 | |     pass
+   |       ^^^^^ A001
-   | |________^ A001
+19 |     pass
-20 |   
-21 |   try:
    |
 
 A001.py:21:1: A001 Variable `ValueError` is shadowing a Python builtin

crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap

@@ -29,14 +29,13 @@ A003.py:4:5: A003 Class attribute `dir` is shadowing a Python builtin
 8 |     def __init__(self):
   |
 
-A003.py:11:5: A003 Class attribute `str` is shadowing a Python builtin
+A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
    |
 11 |         self.dir = "."
 12 | 
 13 |     def str(self):
-   |  _____^
+   |         ^^^ A003
-14 | |         pass
+14 |         pass
-   | |____________^ A003
    |
 
 

crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap

@@ -10,14 +10,13 @@ A003.py:2:5: A003 Class attribute `ImportError` is shadowing a Python builtin
 5 |     dir = "/"
   |
 
-A003.py:11:5: A003 Class attribute `str` is shadowing a Python builtin
+A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
    |
 11 |         self.dir = "."
 12 | 
 13 |     def str(self):
-   |  _____^
+   |         ^^^ A003
-14 | |         pass
+14 |         pass
-   | |____________^ A003
    |
 
 

crates/ruff_python_ast/src/helpers.rs

@@ -1090,21 +1090,40 @@ pub fn match_parens(start: TextSize, locator: &Locator) -> Option<TextRange> {
 /// Specifically, this method returns the range of a function or class name,
 /// rather than that of the entire function or class body.
 pub fn identifier_range(stmt: &Stmt, locator: &Locator) -> TextRange {
-    if matches!(
+    match stmt {
-        stmt,
+        Stmt::ClassDef(ast::StmtClassDef {
-        Stmt::ClassDef(_) | Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_)
+            decorator_list,
-    ) {
+            range,
-        let contents = &locator.contents()[stmt.range()];
+            ..
+        })
+        | Stmt::FunctionDef(ast::StmtFunctionDef {
+            decorator_list,
+            range,
+            ..
+        })
+        | Stmt::AsyncFunctionDef(ast::StmtAsyncFunctionDef {
+            decorator_list,
+            range,
+            ..
+        }) => {
+            let header_range = decorator_list.last().map_or(*range, |last_decorator| {
+                TextRange::new(last_decorator.end(), range.end())
+            });
 
-        for (tok, range) in lexer::lex_starts_at(contents, Mode::Module, stmt.start()).flatten() {
+            let contents = locator.slice(header_range);
-            if matches!(tok, Tok::Name { .. }) {
+
-                return range;
+            let mut tokens =
-            }
+                lexer::lex_starts_at(contents, Mode::Module, header_range.start()).flatten();
-        }
+            tokens
+                .find_map(|(t, range)| t.is_name().then_some(range))
+                .unwrap_or_else(|| {
                     error!("Failed to find identifier for {:?}", stmt);
-    }
 
-    stmt.range()
+                    header_range
+                })
+        }
+        _ => stmt.range(),
+    }
 }
 
 /// Return the ranges of [`Tok::Name`] tokens within a specified node.

crates/ruff_python_formatter/src/cst/mod.rs

@@ -953,12 +953,7 @@ impl From<(ast::Stmt, &Locator<'_>)> for Stmt {
                 };
 
                 Stmt {
-                    range: TextRange::new(
+                    range: TextRange::new(range.start(), body.end()),
-                        decorator_list
-                            .first()
-                            .map_or(range.start(), ast::Ranged::start),
-                        body.end(),
-                    ),
                     node: StmtKind::FunctionDef {
                         name: name.into(),
                         args: Box::new((*args, locator).into()),
@@ -999,12 +994,7 @@ impl From<(ast::Stmt, &Locator<'_>)> for Stmt {
                 };
 
                 Stmt {
-                    range: TextRange::new(
+                    range: TextRange::new(range.start(), body.end()),
-                        decorator_list
-                            .first()
-                            .map_or(range.start(), |expr| expr.range().start()),
-                        body.end(),
-                    ),
                     node: StmtKind::AsyncFunctionDef {
                         name: name.into(),
                         args: Box::new((*args, locator).into()),

crates/ruff_python_semantic/src/binding.rs

@@ -1,7 +1,10 @@
 use std::num::TryFromIntError;
 use std::ops::{Deref, Index, IndexMut};
 
+use crate::model::SemanticModel;
 use bitflags::bitflags;
+use ruff_python_ast::helpers;
+use ruff_python_ast::source_code::Locator;
 use ruff_text_size::TextRange;
 
 use crate::node::NodeId;
@@ -108,6 +111,18 @@ impl<'a> Binding<'a> {
         }
         existing.is_definition()
     }
+
+    /// Returns the appropriate visual range for highlighting this binding.
+    pub fn trimmed_range(&self, semantic_model: &SemanticModel, locator: &Locator) -> TextRange {
+        match self.kind {
+            BindingKind::ClassDefinition | BindingKind::FunctionDefinition => {
+                self.source.map_or(self.range, |source| {
+                    helpers::identifier_range(semantic_model.stmts[source], locator)
+                })
+            }
+            _ => self.range,
+        }
+    }
 }
 
 /// ID uniquely identifying a [Binding] in a program.