Remove Identifier usages for isolating exception names (#5797)

## Summary

The motivating change here is to remove `let range =
except_handler.try_identifier().unwrap();` and instead just do
`name.range()`, since exception names now have ranges attached to them
by the parse. This also required some refactors (which are improvements)
to the built-in attribute shadowing rules, since at least one invocation
relied on passing in the exception handler and calling
`.try_identifier()`. Now that we have easy access to identifiers, we can
remove the whole `AnyShadowing` abstraction.

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

@@ -35,7 +35,6 @@ use crate::fs::relativize_path;
 use crate::importer::Importer;
 use crate::noqa::NoqaMapping;
 use crate::registry::Rule;
-use crate::rules::flake8_builtins::helpers::AnyShadowing;
 
 use crate::rules::{
     airflow, flake8_2020, flake8_annotations, flake8_async, flake8_bandit, flake8_blind_except,
@@ -614,7 +613,7 @@ where
                             self,
                             class_def,
                             name,
-                            AnyShadowing::from(stmt),
+                            name.range(),
                         );
                     }
                 } else {
@@ -622,7 +621,7 @@ where
                         flake8_builtins::rules::builtin_variable_shadowing(
                             self,
                             name,
-                            AnyShadowing::from(stmt),
+                            name.range(),
                         );
                     }
                 }
@@ -753,11 +752,7 @@ where
                     flake8_bugbear::rules::f_string_docstring(self, body);
                 }
                 if self.enabled(Rule::BuiltinVariableShadowing) {
-                    flake8_builtins::rules::builtin_variable_shadowing(
+                    flake8_builtins::rules::builtin_variable_shadowing(self, name, name.range());
-                        self,
-                        name,
-                        AnyShadowing::from(stmt),
-                    );
                 }
                 if self.enabled(Rule::DuplicateBases) {
                     pylint::rules::duplicate_bases(self, name, bases);
@@ -837,7 +832,7 @@ where
                                 flake8_builtins::rules::builtin_variable_shadowing(
                                     self,
                                     asname,
-                                    AnyShadowing::from(stmt),
+                                    asname.range(),
                                 );
                             }
                         }
@@ -1091,7 +1086,7 @@ where
                                 flake8_builtins::rules::builtin_variable_shadowing(
                                     self,
                                     asname,
-                                    AnyShadowing::from(stmt),
+                                    asname.range(),
                                 );
                             }
                         }
@@ -2265,7 +2260,7 @@ where
                     }
                 }
             }
-            Expr::Name(ast::ExprName { id, ctx, range: _ }) => {
+            Expr::Name(ast::ExprName { id, ctx, range }) => {
                 match ctx {
                     ExprContext::Load => {
                         self.handle_node_load(expr);
@@ -2338,18 +2333,13 @@ where
                         if let ScopeKind::Class(class_def) = self.semantic.scope().kind {
                             if self.enabled(Rule::BuiltinAttributeShadowing) {
                                 flake8_builtins::rules::builtin_attribute_shadowing(
-                                    self,
+                                    self, class_def, id, *range,
-                                    class_def,
-                                    id,
-                                    AnyShadowing::from(expr),
                                 );
                             }
                         } else {
                             if self.enabled(Rule::BuiltinVariableShadowing) {
                                 flake8_builtins::rules::builtin_variable_shadowing(
-                                    self,
+                                    self, id, *range,
-                                    id,
-                                    AnyShadowing::from(expr),
                                 );
                             }
                         }
@@ -3880,7 +3870,6 @@ where
                 body,
                 range: _,
             }) => {
-                let name = name.as_deref();
                 if self.enabled(Rule::BareExcept) {
                     if let Some(diagnostic) = pycodestyle::rules::bare_except(
                         type_.as_deref(),
@@ -3892,17 +3881,25 @@ where
                     }
                 }
                 if self.enabled(Rule::RaiseWithoutFromInsideExcept) {
-                    flake8_bugbear::rules::raise_without_from_inside_except(self, name, body);
+                    flake8_bugbear::rules::raise_without_from_inside_except(
+                        self,
+                        name.as_deref(),
+                        body,
+                    );
                 }
                 if self.enabled(Rule::BlindExcept) {
-                    flake8_blind_except::rules::blind_except(self, type_.as_deref(), name, body);
+                    flake8_blind_except::rules::blind_except(
+                        self,
+                        type_.as_deref(),
+                        name.as_deref(),
+                        body,
+                    );
                 }
                 if self.enabled(Rule::TryExceptPass) {
                     flake8_bandit::rules::try_except_pass(
                         self,
                         except_handler,
                         type_.as_deref(),
-                        name,
                         body,
                         self.settings.flake8_bandit.check_typed_exception,
                     );
@@ -3912,7 +3909,6 @@ where
                         self,
                         except_handler,
                         type_.as_deref(),
-                        name,
                         body,
                         self.settings.flake8_bandit.check_typed_exception,
                     );
@@ -3929,66 +3925,66 @@ where
                 if self.enabled(Rule::BinaryOpException) {
                     pylint::rules::binary_op_exception(self, except_handler);
                 }
-                match name {
-                    Some(name) => {
-                        let range = except_handler.try_identifier().unwrap();
 
-                        if self.enabled(Rule::AmbiguousVariableName) {
+                if let Some(name) = name {
-                            if let Some(diagnostic) =
+                    if self.enabled(Rule::AmbiguousVariableName) {
-                                pycodestyle::rules::ambiguous_variable_name(name, range)
+                        if let Some(diagnostic) =
-                            {
+                            pycodestyle::rules::ambiguous_variable_name(name.as_str(), name.range())
-                                self.diagnostics.push(diagnostic);
+                        {
-                            }
+                            self.diagnostics.push(diagnostic);
                         }
-                        if self.enabled(Rule::BuiltinVariableShadowing) {
+                    }
-                            flake8_builtins::rules::builtin_variable_shadowing(
+                    if self.enabled(Rule::BuiltinVariableShadowing) {
-                                self,
+                        flake8_builtins::rules::builtin_variable_shadowing(
-                                name,
+                            self,
-                                AnyShadowing::from(except_handler),
-                            );
-                        }
-
-                        // Store the existing binding, if any.
-                        let existing_id = self.semantic.lookup_symbol(name);
-
-                        // Add the bound exception name to the scope.
-                        let binding_id = self.add_binding(
                             name,
-                            range,
+                            name.range(),
-                            BindingKind::Assignment,
-                            BindingFlags::empty(),
-                        );
-
-                        walk_except_handler(self, except_handler);
-
-                        // If the exception name wasn't used in the scope, emit a diagnostic.
-                        if !self.semantic.is_used(binding_id) {
-                            if self.enabled(Rule::UnusedVariable) {
-                                let mut diagnostic = Diagnostic::new(
-                                    pyflakes::rules::UnusedVariable { name: name.into() },
-                                    range,
-                                );
-                                if self.patch(Rule::UnusedVariable) {
-                                    diagnostic.try_set_fix(|| {
-                                        pyflakes::fixes::remove_exception_handler_assignment(
-                                            except_handler,
-                                            self.locator,
-                                        )
-                                        .map(Fix::automatic)
-                                    });
-                                }
-                                self.diagnostics.push(diagnostic);
-                            }
-                        }
-
-                        self.add_binding(
-                            name,
-                            range,
-                            BindingKind::UnboundException(existing_id),
-                            BindingFlags::empty(),
                         );
                     }
-                    None => walk_except_handler(self, except_handler),
+
+                    // Store the existing binding, if any.
+                    let existing_id = self.semantic.lookup_symbol(name.as_str());
+
+                    // Add the bound exception name to the scope.
+                    let binding_id = self.add_binding(
+                        name.as_str(),
+                        name.range(),
+                        BindingKind::Assignment,
+                        BindingFlags::empty(),
+                    );
+
+                    walk_except_handler(self, except_handler);
+
+                    // If the exception name wasn't used in the scope, emit a diagnostic.
+                    if !self.semantic.is_used(binding_id) {
+                        if self.enabled(Rule::UnusedVariable) {
+                            let mut diagnostic = Diagnostic::new(
+                                pyflakes::rules::UnusedVariable {
+                                    name: name.to_string(),
+                                },
+                                name.range(),
+                            );
+                            if self.patch(Rule::UnusedVariable) {
+                                diagnostic.try_set_fix(|| {
+                                    pyflakes::fixes::remove_exception_handler_assignment(
+                                        except_handler,
+                                        self.locator,
+                                    )
+                                    .map(Fix::automatic)
+                                });
+                            }
+                            self.diagnostics.push(diagnostic);
+                        }
+                    }
+
+                    self.add_binding(
+                        name.as_str(),
+                        name.range(),
+                        BindingKind::UnboundException(existing_id),
+                        BindingFlags::empty(),
+                    );
+                } else {
+                    walk_except_handler(self, except_handler);
                 }
             }
         }

crates/ruff/src/rules/flake8_bandit/rules/try_except_continue.rs

@@ -55,16 +55,14 @@ pub(crate) fn try_except_continue(
     checker: &mut Checker,
     except_handler: &ExceptHandler,
     type_: Option<&Expr>,
-    _name: Option<&str>,
     body: &[Stmt],
     check_typed_exception: bool,
 ) {
-    if body.len() == 1
+    if matches!(body, [Stmt::Continue(_)]) {
-        && body[0].is_continue_stmt()
+        if check_typed_exception || is_untyped_exception(type_, checker.semantic()) {
-        && (check_typed_exception || is_untyped_exception(type_, checker.semantic()))
+            checker
-    {
+                .diagnostics
-        checker
+                .push(Diagnostic::new(TryExceptContinue, except_handler.range()));
-            .diagnostics
+        }
-            .push(Diagnostic::new(TryExceptContinue, except_handler.range()));
     }
 }

crates/ruff/src/rules/flake8_bandit/rules/try_except_pass.rs

@@ -51,16 +51,14 @@ pub(crate) fn try_except_pass(
     checker: &mut Checker,
     except_handler: &ExceptHandler,
     type_: Option<&Expr>,
-    _name: Option<&str>,
     body: &[Stmt],
     check_typed_exception: bool,
 ) {
-    if body.len() == 1
+    if matches!(body, [Stmt::Pass(_)]) {
-        && body[0].is_pass_stmt()
+        if check_typed_exception || is_untyped_exception(type_, checker.semantic()) {
-        && (check_typed_exception || is_untyped_exception(type_, checker.semantic()))
+            checker
-    {
+                .diagnostics
-        checker
+                .push(Diagnostic::new(TryExceptPass, except_handler.range()));
-            .diagnostics
+        }
-            .push(Diagnostic::new(TryExceptPass, except_handler.range()));
     }
 }

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

@@ -1,44 +1,5 @@
-use ruff_text_size::TextRange;
-use rustpython_parser::ast::{ExceptHandler, Expr, Ranged, Stmt};
-
-use ruff_python_ast::identifier::{Identifier, TryIdentifier};
 use ruff_python_stdlib::builtins::BUILTINS;
 
 pub(super) fn shadows_builtin(name: &str, ignorelist: &[String]) -> bool {
     BUILTINS.contains(&name) && ignorelist.iter().all(|ignore| ignore != name)
 }
-
-#[derive(Debug, Copy, Clone, PartialEq)]
-pub(crate) enum AnyShadowing<'a> {
-    Expression(&'a Expr),
-    Statement(&'a Stmt),
-    ExceptHandler(&'a ExceptHandler),
-}
-
-impl Identifier for AnyShadowing<'_> {
-    fn identifier(&self) -> TextRange {
-        match self {
-            AnyShadowing::Expression(expr) => expr.range(),
-            AnyShadowing::Statement(stmt) => stmt.identifier(),
-            AnyShadowing::ExceptHandler(handler) => handler.try_identifier().unwrap(),
-        }
-    }
-}
-
-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/rules/builtin_attribute_shadowing.rs

@@ -1,12 +1,12 @@
+use ruff_text_size::TextRange;
+use rustpython_parser::ast;
+
 use ruff_diagnostics::Diagnostic;
 use ruff_diagnostics::Violation;
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_ast::identifier::Identifier;
-use rustpython_parser::ast;
 
 use crate::checkers::ast::Checker;
-
+use crate::rules::flake8_builtins::helpers::shadows_builtin;
-use super::super::helpers::{shadows_builtin, AnyShadowing};
 
 /// ## What it does
 /// Checks for any class attributes that use the same name as a builtin.
@@ -67,7 +67,7 @@ pub(crate) fn builtin_attribute_shadowing(
     checker: &mut Checker,
     class_def: &ast::StmtClassDef,
     name: &str,
-    shadowing: AnyShadowing,
+    range: TextRange,
 ) {
     if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
         // Ignore shadowing within `TypedDict` definitions, since these are only accessible through
@@ -84,7 +84,7 @@ pub(crate) fn builtin_attribute_shadowing(
             BuiltinAttributeShadowing {
                 name: name.to_string(),
             },
-            shadowing.identifier(),
+            range,
         ));
     }
 }

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

@@ -1,11 +1,11 @@
+use ruff_text_size::TextRange;
+
 use ruff_diagnostics::Diagnostic;
 use ruff_diagnostics::Violation;
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_ast::identifier::Identifier;
 
 use crate::checkers::ast::Checker;
-
+use crate::rules::flake8_builtins::helpers::shadows_builtin;
-use super::super::helpers::{shadows_builtin, AnyShadowing};
 
 /// ## What it does
 /// Checks for variable (and function) assignments that use the same name
@@ -59,17 +59,13 @@ impl Violation for BuiltinVariableShadowing {
 }
 
 /// A001
-pub(crate) fn builtin_variable_shadowing(
+pub(crate) fn builtin_variable_shadowing(checker: &mut Checker, name: &str, range: TextRange) {
-    checker: &mut Checker,
-    name: &str,
-    shadowing: AnyShadowing,
-) {
     if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
         checker.diagnostics.push(Diagnostic::new(
             BuiltinVariableShadowing {
                 name: name.to_string(),
             },
-            shadowing.identifier(),
+            range,
         ));
     }
 }

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

@@ -1,28 +1,28 @@
 ---
 source: crates/ruff/src/rules/flake8_builtins/mod.rs
 ---
-A001.py:1:1: A001 Variable `sum` is shadowing a Python builtin
+A001.py:1:16: A001 Variable `sum` is shadowing a Python builtin
   |
 1 | import some as sum
-  | ^^^^^^^^^^^^^^^^^^ A001
+  |                ^^^ A001
 2 | from some import other as int
 3 | from directory import new as dir
   |
 
-A001.py:2:1: A001 Variable `int` is shadowing a Python builtin
+A001.py:2:27: A001 Variable `int` is shadowing a Python builtin
   |
 1 | import some as sum
 2 | from some import other as int
-  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A001
+  |                           ^^^ A001
 3 | from directory import new as dir
   |
 
-A001.py:3:1: A001 Variable `dir` is shadowing a Python builtin
+A001.py:3:30: A001 Variable `dir` is shadowing a Python builtin
   |
 1 | import some as sum
 2 | from some import other as int
 3 | from directory import new as dir
-  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A001
+  |                              ^^^ A001
 4 | 
 5 | print = 1
   |

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

@@ -1,19 +1,19 @@
 ---
 source: crates/ruff/src/rules/flake8_builtins/mod.rs
 ---
-A001.py:1:1: A001 Variable `sum` is shadowing a Python builtin
+A001.py:1:16: A001 Variable `sum` is shadowing a Python builtin
   |
 1 | import some as sum
-  | ^^^^^^^^^^^^^^^^^^ A001
+  |                ^^^ A001
 2 | from some import other as int
 3 | from directory import new as dir
   |
 
-A001.py:2:1: A001 Variable `int` is shadowing a Python builtin
+A001.py:2:27: A001 Variable `int` is shadowing a Python builtin
   |
 1 | import some as sum
 2 | from some import other as int
-  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A001
+  |                           ^^^ A001
 3 | from directory import new as dir
   |
 

crates/ruff_python_ast/src/identifier.rs

@@ -138,22 +138,6 @@ impl TryIdentifier for Pattern {
     }
 }
 
-impl TryIdentifier for ExceptHandler {
-    /// Return the [`TextRange`] of a named exception in an [`ExceptHandler`].
-    ///
-    /// For example, return the range of `e` in:
-    /// ```python
-    /// try:
-    ///     ...
-    /// except ValueError as e:
-    ///     ...
-    /// ```
-    fn try_identifier(&self) -> Option<TextRange> {
-        let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { name, .. }) = self;
-        name.as_ref().map(Ranged::range)
-    }
-}
-
 /// Return the [`TextRange`] of the `except` token in an [`ExceptHandler`].
 pub fn except(handler: &ExceptHandler, locator: &Locator) -> TextRange {
     IdentifierTokenizer::new(locator.contents(), handler.range())