Fix TSP inferred returns for definition-site computed types#4045
Fix TSP inferred returns for definition-site computed types#4045WilliamK112 wants to merge 1 commit into
Conversation
|
@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D110964202. |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Nice, focused fix. A few clarifying questions/nits inline — none blocking.
| { | ||
| // 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. |
There was a problem hiding this comment.
The linked issue diagnoses Key::Definition as yielding the pre-inference function type, yet this fix relies on get_type_at_preserving_declaration -> the FunctionDef/MethodDef arm of get_type_at_impl_with_options -> Key::Definition to actually carry the inferred return. So the effective change is get_type_trace (the stored trace type) vs. the Key::Definition binding. 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 about Key::Definition.
| context, | ||
| IdentifierContext::Expr(_) | ||
| | IdentifierContext::FunctionDef { .. } | ||
| | IdentifierContext::MethodDef { .. } |
There was a problem hiding this comment.
ClassDef is intentionally excluded here, but it reads like it could be an oversight. Could you add a one-line note explaining why? My understanding is class def-sites don't have the inferred-return problem and their get_type_trace type is already correct, so they don't need the declaration-preserving path — but a comment would make that explicit.
| // 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( |
There was a problem hiding this comment.
Nit (optional, non-blocking): identifier_at runs here and then again inside get_type_at_preserving_declaration, so the identifier is located twice per call. This isn't introduced by the PR (the old code also walked the AST twice), so fine to leave as-is — just flagging in case a variant that accepts the already-resolved identifier feels worthwhile.
3 participants
Fixes #4042.
Summary
returnTypeat definition sites.untyped-def-behavior = check-and-infer-return-type.Validation
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly test_get_computed_type_definition_site_uses_inferred_return_type -- --nocapturecargo +stable-x86_64-pc-windows-gnu test -p pyrefly get_type_queriescargo +stable-x86_64-pc-windows-gnu test -p pyrefly tsp_interactioncargo +stable-x86_64-pc-windows-gnu fmt --all --checkgit diff --checkcargo +stable-x86_64-pc-windows-gnu test -p pyrefly test_shape_dsl_reports_multiple_errors -- --nocaptureI also attempted the full
cargo +stable-x86_64-pc-windows-gnu test -p pyreflysuite. It reached6188 passed, 1 failed; the only failure was the unrelatedtest::shape_dsl::test_shape_dsl_reports_multiple_errorsinternal watchdog timeout (Error: Test took too long (> 20s)) during the full parallel run. The same test passed when rerun directly, as shown above.