[ty] Fix panic on incomplete except handlers#23708
Merged
charliermarsh merged 3 commits intomainfrom Mar 5, 2026
Merged
Conversation
Typing conformance resultsNo changes detected ✅ |
|
Memory usage reportMemory usage unchanged ✅ |
charliermarsh
requested review from
AlexWaygood,
BurntSushi,
Gankra,
MichaReiser,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
BurntSushi
approved these changes
Mar 4, 2026
| /// ## Panics | ||
| /// May panic if `self` is from another file than `model`. | ||
| fn definition<'db>(&self, model: &SemanticModel<'db>) -> Definition<'db>; | ||
| fn definition<'db>(&self, model: &SemanticModel<'db>) -> Option<Definition<'db>>; |
Member
There was a problem hiding this comment.
Should the docs here be updated to say when callers ought to return None? It's somewhat odd that in some cases it will panic (e.g., via expect_single_definition) but in others it will return None.
MichaReiser
reviewed
Mar 4, 2026
Comment on lines
+311
to
+326
| if handler.name.is_some() { | ||
| Some( | ||
| handler | ||
| .definition(self)? | ||
| .scope(self.db) | ||
| .file_scope_id(self.db), | ||
| ) | ||
| } else { | ||
| handler | ||
| .type_ | ||
| .as_deref() | ||
| .and_then(|handled_exceptions| { | ||
| index.try_expression_scope_id(handled_exceptions) | ||
| }) | ||
| .or(Some(FileScopeId::global())) | ||
| } |
Member
There was a problem hiding this comment.
The name.is_some and definiton(self)? sort of encode the same check. Can we just use if let Some(definition) = handler.definition(..) instead?
| impl HasDefinition for $ty { | ||
| #[inline] | ||
| fn definition<'db>(&self, model: &SemanticModel<'db>) -> Definition<'db> { | ||
| fn definition<'db>(&self, model: &SemanticModel<'db>) -> Option<Definition<'db>> { |
Member
There was a problem hiding this comment.
This is a bit unfortunate. Have you explored whether we could always create a definition in ExceptHandler even if the name is missing? Are there any undesired side effects if we would do so?
Member
Author
There was a problem hiding this comment.
Hah, isn't this what you suggested in astral-sh/ty#2401 (comment)?
Member
There was a problem hiding this comment.
Haha, I completely forgot about that. It's certainly the easiest change :)
Member
There was a problem hiding this comment.
We could also consider removing the HasDefinition implementation on ExceptHandler. I don't know what the fallout of that is
Edit: Or reconsider implementing HasDefinition and HasType for ExceptHandlerExceptHandler. Maybe it's better to change the call sites to call the same methods on the name instead
Ahhh... this doesn't work because name is not an AST node. Sooo annoying
Member
Author
There was a problem hiding this comment.
Okay, I changed things up to remove HasDefinition from ExceptHandler.
MichaReiser
reviewed
Mar 4, 2026
| ast::AnyNodeRef::StmtFunctionDef(function) => Some( | ||
| function | ||
| .definition(self) | ||
| .definition(self)? |
Member
There was a problem hiding this comment.
I feel like all these early returns assume that function.definition is always Some (maybe that's fine; it just feels a bit fragile). If it's none, we'd probably want to prefer using the fallback scope instead of returning None.
MichaReiser
reviewed
Mar 5, 2026
| impl_binding_has_ty_def!(ast::StmtClassDef); | ||
| impl_binding_has_ty_def!(ast::Parameter); | ||
| impl_binding_has_ty_def!(ast::ParameterWithDefault); | ||
| impl_binding_has_ty_def!(ast::ExceptHandlerExceptHandler); |
Member
There was a problem hiding this comment.
I don't think it was necessary to remove both the HasDefinition and HasType implementations for ExceptHandlerExceptHandler. We can still implement HasType manually, since inferred_type already returns an Option.
Comment on lines
+311
to
+313
| handler.type_.as_deref().and_then(|handled_exceptions| { | ||
| index.try_expression_scope_id(handled_exceptions) | ||
| }) |
Member
There was a problem hiding this comment.
Suggested change
| handler.type_.as_deref().and_then(|handled_exceptions| { | |
| index.try_expression_scope_id(handled_exceptions) | |
| }) | |
| index.try_expression_scoipe_id(handler.type_.as_deref()?) |
Comment on lines
+331
to
+341
| /// 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)) | ||
| } |
Member
There was a problem hiding this comment.
The API doesn't feel well aligned with HasType and HasDefinition. Maybe introduce a MaybeHasDefinition or HasOptionalDefinition trait and implement that instead. I agree, it feels a bit overkill but it's unfortunately the only way to add a method to a foreign type
charliermarsh
added a commit
that referenced
this pull request
Mar 5, 2026
## Summary See: #23708 (comment).
carljm
added a commit
that referenced
this pull request
Mar 5, 2026
* main: Update conformance suite commit hash (#23746) conformance.py: Collapse the summary paragraph when nothing changed (#23745) [ty] Make inferred specializations line up with source types more better (#23715) Bump 0.15.5 (#23743) [ty] Render all changed diagnostics in conformance.py (#23613) [ty] Split deferred checks out of `types/infer/builder.rs` (#23740) Discover markdown files by default in preview mode (#23434) [ty] Use `HasOptionalDefinition` for `except` handlers (#23739) [ty] Fix precedence of `all` selector in TOML configurations (#23723) [ty] Make `all` selector case sensitive (#23713) [ty] Add a diagnostic if a `TypeVar` is used to specialize a `ParamSpec`, or vice versa (#23738) [ty] Override home directory in ty tests (#23724) [ty] More type-variable default validation (#23639) [ty] Validate bare ParamSpec usage in type annotations, and support stringified ParamSpecs as the first argument to `Callable` (#23625) [ty] Add `all` selector to ty.json's `schema` (#23721) [ty] Add quotes to related issues links (#23720) [ty] Fix panic on incomplete except handlers (#23708)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
We no longer assume that
ExceptHandlerExceptHandleralways has a definition.Closes astral-sh/ty#2401.