-
Notifications
You must be signed in to change notification settings - Fork 429
Fix TSP inferred returns for definition-site computed types #4045
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1248,15 +1248,26 @@ impl<'a> Transaction<'a> { | |
| /// | ||
| /// TSP prefers raw bound types of identifiers since it re-resolves declarations. | ||
| pub fn get_computed_type_at_range(&self, handle: &Handle, range: TextRange) -> Option<Type> { | ||
| // Bare names need declaration-preserving lookup to avoid coercing | ||
| // overloaded functions into synthesized callables. | ||
| if range.is_empty() | ||
| || self.get_ast(handle).is_some_and(|module| { | ||
| Ast::locate_node(&module, range.start()) | ||
| .into_iter() | ||
| .any(|node| node.range() == range && matches!(node, AnyNodeRef::ExprName(_))) | ||
| }) | ||
| { | ||
| // Bare names and definition-name ranges need declaration-preserving | ||
| // lookup to avoid coercing overloaded functions into synthesized | ||
| // callables, and to preserve inferred returns for unannotated defs. | ||
| let is_identifier_name_range = !range.is_empty() | ||
| && self.identifier_at(handle, range.start()).is_some_and( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (optional, non-blocking): |
||
| |IdentifierWithContext { | ||
| identifier, | ||
| context, | ||
| }| { | ||
| identifier.range == range | ||
| && matches!( | ||
| context, | ||
| IdentifierContext::Expr(_) | ||
| | IdentifierContext::FunctionDef { .. } | ||
| | IdentifierContext::MethodDef { .. } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) | ||
| }, | ||
| ); | ||
|
|
||
| if range.is_empty() || is_identifier_name_range { | ||
| return self.get_type_at_preserving_declaration(handle, range.start()); | ||
| } | ||
| self.get_type_trace(handle, range) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linked issue diagnoses
Key::Definitionas yielding the pre-inference function type, yet this fix relies onget_type_at_preserving_declaration-> theFunctionDef/MethodDefarm ofget_type_at_impl_with_options->Key::Definitionto actually carry the inferred return. So the effective change isget_type_trace(the stored trace type) vs. theKey::Definitionbinding. Since that distinction is the crux of the fix, could you add a sentence here (or in the commit message) spelling out the real mechanism? It'll save future readers who go back to the issue and see the seemingly-contradictory claim aboutKey::Definition.