[ty] Use HasOptionalDefinition for except handlers (#23739)

charliermarsh · web-flow · commit 3dc78b0a84ee · 2026-03-05T10:30:55.000-05:00
## Summary See: #23708 (comment).
diff --git a/crates/ty_ide/src/goto.rs b/crates/ty_ide/src/goto.rs
@@ -21,8 +21,8 @@ use ty_python_semantic::types::ide_support::{
     typed_dict_key_definition,
 };
 use ty_python_semantic::{
-    HasDefinition, HasType, ImportAliasResolution, SemanticModel, TypeQualifiers,
-    definitions_for_imported_symbol, definitions_for_name,
+    HasDefinition, HasOptionalDefinition, HasType, ImportAliasResolution, SemanticModel,
+    TypeQualifiers, definitions_for_imported_symbol, definitions_for_name,
 };
 
 #[derive(Clone, Debug)]
@@ -331,7 +331,7 @@ impl GotoTarget<'_> {
             GotoTarget::ImportSymbolAlias { alias, .. }
             | GotoTarget::ImportModuleAlias { alias, .. }
             | GotoTarget::ImportExportedName { alias, .. } => alias.inferred_type(model),
-            GotoTarget::ExceptVariable(except) => model.except_handler_type(except),
+            GotoTarget::ExceptVariable(except) => except.inferred_type(model),
             GotoTarget::KeywordArgument { keyword, .. } => keyword.value.inferred_type(model),
             // When asking the type of a callable, usually you want the callable itself?
             // (i.e. the type of `MyClass` in `MyClass()` is `<class MyClass>` and not `() -> MyClass`)
@@ -515,8 +515,8 @@ impl GotoTarget<'_> {
             )),
 
             // For exception variables, they are their own definitions (like parameters)
-            GotoTarget::ExceptVariable(except_handler) => model
-                .except_handler_definition(except_handler)
+            GotoTarget::ExceptVariable(except_handler) => except_handler
+                .optional_definition(model)
                 .map(|definition| vec![ResolvedDefinition::Definition(definition)]),
 
             // Patterns are glorified assignments but we have to look them up by ident
diff --git a/crates/ty_python_semantic/src/lib.rs b/crates/ty_python_semantic/src/lib.rs
@@ -14,7 +14,8 @@ pub use program::{Program, ProgramSettings};
 pub use python_platform::PythonPlatform;
 use rustc_hash::FxHasher;
 pub use semantic_model::{
-    Completion, HasDefinition, HasType, MemberDefinition, NameKind, SemanticModel,
+    Completion, HasDefinition, HasOptionalDefinition, HasType, MemberDefinition, NameKind,
+    SemanticModel,
 };
 pub use suppression::{
     UNUSED_IGNORE_COMMENT, is_unused_ignore_comment_lint, suppress_all, suppress_single,
diff --git a/crates/ty_python_semantic/src/semantic_model.rs b/crates/ty_python_semantic/src/semantic_model.rs
@@ -304,14 +304,10 @@ impl<'db> SemanticModel<'db> {
                     .scope(self.db)
                     .file_scope_id(self.db),
             ),
-            ast::AnyNodeRef::ExceptHandlerExceptHandler(handler) => self
-                .except_handler_definition(handler)
+            ast::AnyNodeRef::ExceptHandlerExceptHandler(handler) => handler
+                .optional_definition(self)
                 .map(|definition| definition.scope(self.db).file_scope_id(self.db))
-                .or_else(|| {
-                    handler.type_.as_deref().and_then(|handled_exceptions| {
-                        index.try_expression_scope_id(handled_exceptions)
-                    })
-                })
+                .or_else(|| index.try_expression_scope_id(handler.type_.as_deref()?))
                 .or(Some(FileScopeId::global())),
             ast::AnyNodeRef::TypeParamTypeVar(var) => {
                 Some(var.definition(self).scope(self.db).file_scope_id(self.db))
@@ -328,29 +324,6 @@ impl<'db> SemanticModel<'db> {
         }
     }
 
-    /// Returns the definition for an exception-handler variable.
-    ///
-    /// Exception handlers only have a definition when they bind a name (`except E as name:`).
-    pub fn except_handler_definition(
-        &self,
-        handler: &ast::ExceptHandlerExceptHandler,
-    ) -> Option<Definition<'db>> {
-        handler.name.as_ref()?;
-        let index = semantic_index(self.db, self.file);
-        Some(index.expect_single_definition(handler))
-    }
-
-    /// Returns the inferred type of an exception-handler variable.
-    ///
-    /// Exception handlers only bind a variable when they have a name (`except E as name:`).
-    pub fn except_handler_type(
-        &self,
-        handler: &ast::ExceptHandlerExceptHandler,
-    ) -> Option<Type<'db>> {
-        let definition = self.except_handler_definition(handler)?;
-        Some(binding_type(self.db, definition))
-    }
-
     /// Get a "safe" [`ast::AnyNodeRef`] to use for referring to the given (sub-)AST node.
     ///
     /// If we're analyzing a string annotation, it will return the string literal's node.
@@ -543,6 +516,14 @@ pub trait HasDefinition {
     fn definition<'db>(&self, model: &SemanticModel<'db>) -> Definition<'db>;
 }
 
+pub trait HasOptionalDefinition {
+    /// Returns the definition of `self`, if it has one.
+    ///
+    /// ## Panics
+    /// May panic if `self` is from another file than `model`.
+    fn optional_definition<'db>(&self, model: &SemanticModel<'db>) -> Option<Definition<'db>>;
+}
+
 impl HasType for ast::ExprRef<'_> {
     fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
         let index = semantic_index(model.db, model.file);
@@ -679,6 +660,21 @@ impl HasType for ast::Alias {
     }
 }
 
+impl HasOptionalDefinition for ast::ExceptHandlerExceptHandler {
+    fn optional_definition<'db>(&self, model: &SemanticModel<'db>) -> Option<Definition<'db>> {
+        self.name.as_ref()?;
+        let index = semantic_index(model.db, model.file);
+        Some(index.expect_single_definition(self))
+    }
+}
+
+impl HasType for ast::ExceptHandlerExceptHandler {
+    fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
+        let definition = self.optional_definition(model)?;
+        Some(binding_type(model.db, definition))
+    }
+}
+
 /// Implemented by types for which the semantic index tracks their scope.
 pub(crate) trait HasTrackedScope: HasNodeIndex {}
 
