Skip to content

[ty] Fix panic on incomplete except handlers #23708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
charliermarsh merged 3 commits into from
Mar 5, 2026
Merged

[ty] Fix panic on incomplete except handlers #23708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc016d2
[ty] Fix panic on incomplete except handlers
charliermarsh Mar 4, 2026
4c68f97
Make optional
charliermarsh Mar 4, 2026
223ae1f
Remove implementation
charliermarsh Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions crates/ty_ide/src/completion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5334,6 +5334,23 @@ except Type<CURSOR>:
);
}

// Ref: https://github.com/astral-sh/ty/issues/2401
#[test]
fn no_panic_incomplete_except_handler() {
let builder = completion_test_builder(
"\
try:
print()
except <CURSOR># Trigger completion/hover here
",
);

assert_snapshot!(
builder.skip_keywords().skip_builtins().skip_auto_import().build().snapshot(),
@"<No completions found after filtering out completions>",
);
}

// Ref: https://github.com/astral-sh/ty/issues/572
#[test]
fn scope_id_missing_global1() {
Expand Down
38 changes: 23 additions & 15 deletions crates/ty_ide/src/goto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,13 +459,15 @@ impl GotoTarget<'_> {
Some(defs)
}

GotoTarget::ClassDef(class) => Some(vec![ResolvedDefinition::Definition(
class.definition(model),
)]),
GotoTarget::ClassDef(class) => class
.definition(model)
.map(ResolvedDefinition::Definition)
.map(|definition| vec![definition]),

GotoTarget::Parameter(parameter) => Some(vec![ResolvedDefinition::Definition(
parameter.definition(model),
)]),
GotoTarget::Parameter(parameter) => parameter
.definition(model)
.map(ResolvedDefinition::Definition)
.map(|definition| vec![definition]),

// For import aliases (offset within 'y' or 'z' in "from x import y as z")
GotoTarget::ImportSymbolAlias { asname, .. } => Some(definitions_for_name(
Expand Down Expand Up @@ -515,11 +517,13 @@ impl GotoTarget<'_> {
)),

// For exception variables, they are their own definitions (like parameters)
GotoTarget::ExceptVariable(except_handler) => {
Some(vec![ResolvedDefinition::Definition(
except_handler.definition(model),
)])
}
GotoTarget::ExceptVariable(except_handler) => except_handler.name.as_ref().map(|_| {
vec![ResolvedDefinition::Definition(
except_handler
.definition(model)
.expect("named except handlers should have a definition"),
)]
}),

// Patterns are glorified assignments but we have to look them up by ident
// because they're not expressions
Expand Down Expand Up @@ -949,9 +953,10 @@ impl GotoTarget<'_> {

None
}
Some(AnyNodeRef::ExceptHandlerExceptHandler(handler)) => {
Some(GotoTarget::ExceptVariable(handler))
}
Some(AnyNodeRef::ExceptHandlerExceptHandler(handler)) => handler
.name
.is_some()
.then_some(GotoTarget::ExceptVariable(handler)),
Some(AnyNodeRef::Keyword(keyword)) => {
// Find the containing call expression from the ancestor chain
let call_expression = covering_node
Expand Down Expand Up @@ -1139,7 +1144,10 @@ impl Ranged for GotoTarget<'_> {
} => *component_range,
GotoTarget::StringAnnotationSubexpr { subrange, .. } => *subrange,
GotoTarget::ImportModuleAlias { asname, .. } => asname.range,
GotoTarget::ExceptVariable(except) => except.name.as_ref().unwrap().range,
GotoTarget::ExceptVariable(except) => except
.name
.as_ref()
.map_or(except.range(), |name| name.range),
GotoTarget::KeywordArgument { keyword, .. } => keyword.arg.as_ref().unwrap().range,
GotoTarget::PatternMatchRest(rest) => rest.rest.as_ref().unwrap().range,
GotoTarget::PatternKeywordArgument(keyword) => keyword.attr.range,
Expand Down
14 changes: 14 additions & 0 deletions crates/ty_ide/src/hover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4952,6 +4952,20 @@ def function():
");
}

// Ref: https://github.com/astral-sh/ty/issues/2401
#[test]
fn hover_incomplete_except_handler() {
let test = cursor_test(
"\
try:
print()
except <CURSOR># Trigger completion/hover here
",
);

assert_snapshot!(test.hover(), @"Hover provided no content");
}

impl CursorTest {
fn hover(&self) -> String {
use std::fmt::Write;
Expand Down
67 changes: 49 additions & 18 deletions crates/ty_python_semantic/src/semantic_model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,33 +285,48 @@ impl<'db> SemanticModel<'db> {
// Nodes implementing `HasDefinition`
ast::AnyNodeRef::StmtFunctionDef(function) => Some(
function
.definition(self)
.definition(self)?

@MichaReiser MichaReiser Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.scope(self.db)
.file_scope_id(self.db),
),
ast::AnyNodeRef::StmtClassDef(class) => {
Some(class.definition(self).scope(self.db).file_scope_id(self.db))
}
ast::AnyNodeRef::Parameter(parameter) => Some(
parameter
.definition(self)
ast::AnyNodeRef::StmtClassDef(class) => Some(
class
.definition(self)?
.scope(self.db)
.file_scope_id(self.db),
),
ast::AnyNodeRef::ParameterWithDefault(parameter) => Some(
ast::AnyNodeRef::Parameter(parameter) => Some(
parameter
.definition(self)
.definition(self)?
.scope(self.db)
.file_scope_id(self.db),
),
ast::AnyNodeRef::ExceptHandlerExceptHandler(handler) => Some(
handler
.definition(self)
ast::AnyNodeRef::ParameterWithDefault(parameter) => Some(
parameter
.definition(self)?
.scope(self.db)
.file_scope_id(self.db),
),
ast::AnyNodeRef::ExceptHandlerExceptHandler(handler) => {
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()))
}

@MichaReiser MichaReiser Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name.is_some and definiton(self)? sort of encode the same check. Can we just use if let Some(definition) = handler.definition(..) instead?

}
ast::AnyNodeRef::TypeParamTypeVar(var) => {
Some(var.definition(self).scope(self.db).file_scope_id(self.db))
Some(var.definition(self)?.scope(self.db).file_scope_id(self.db))
}

// Fallback
Expand Down Expand Up @@ -514,7 +529,7 @@ pub trait HasDefinition {
///
/// ## 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>>;

@BurntSushi BurntSushi Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

impl HasType for ast::ExprRef<'_> {
Expand Down Expand Up @@ -621,16 +636,16 @@ macro_rules! impl_binding_has_ty_def {
($ty: ty) => {
impl HasDefinition for $ty {
#[inline]
fn definition<'db>(&self, model: &SemanticModel<'db>) -> Definition<'db> {
fn definition<'db>(&self, model: &SemanticModel<'db>) -> Option<Definition<'db>> {

@MichaReiser MichaReiser Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@charliermarsh charliermarsh Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, isn't this what you suggested in astral-sh/ty#2401 (comment)?

@charliermarsh charliermarsh Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a look.

@MichaReiser MichaReiser Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I completely forgot about that. It's certainly the easiest change :)

@MichaReiser MichaReiser Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@charliermarsh charliermarsh Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I changed things up to remove HasDefinition from ExceptHandler.

let index = semantic_index(model.db, model.file);
index.expect_single_definition(self)
Some(index.expect_single_definition(self))
}
}

impl HasType for $ty {
#[inline]
fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
let binding = HasDefinition::definition(self, model);
let binding = HasDefinition::definition(self, model)?;
Some(binding_type(model.db, binding))
}
}
Expand All @@ -641,9 +656,25 @@ impl_binding_has_ty_def!(ast::StmtFunctionDef);
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);

@MichaReiser MichaReiser Mar 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

impl_binding_has_ty_def!(ast::TypeParamTypeVar);

impl HasDefinition for ast::ExceptHandlerExceptHandler {
#[inline]
fn 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 {
#[inline]
fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
let binding = HasDefinition::definition(self, model)?;
Some(binding_type(model.db, binding))
}
}

impl HasType for ast::Alias {
fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
if &self.name == "*" {
Expand Down
6 changes: 5 additions & 1 deletion crates/ty_python_semantic/src/types/ide_support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,11 @@ pub fn definitions_and_overloads_for_function<'db>(
.map(ResolvedDefinition::Definition)
.collect()
} else {
vec![ResolvedDefinition::Definition(function.definition(model))]
function
.definition(model)
.map(ResolvedDefinition::Definition)
.into_iter()
.collect()
}
}

Expand Down
Loading