[ty] infer function's return type #17371
Open
+1,582
−123
Conversation
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
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
Author
|
If a function contains a yield expression, i.e. if it is a generator, current type inference is incorrect. But supporting this requires solving another issues, so I will leave it as a TODO for now. As for return type inference for normal functions, I think the work is completed. |
mtshiba
force-pushed
the
infer-return-type
branch
4 times, most recently
from
3ba99d1 to
35f4a75
Compare
MichaReiser
changed the title
Contributor
|
Sorry that I haven't gotten around to reviewing this PR yet. There's just been a lot to do, and providing this feature is lower on my priority list than some other features. But I realize that places a burden on you to keep it up to date with (rapidly changing) main branch. But I will try to find time to review it soon. |
mtshiba
force-pushed
the
infer-return-type
branch
from
0a16b46 to
126aef8
Compare
mtshiba
force-pushed
the
infer-return-type
branch
from
91c66c1 to
22e8594
Compare
mtshiba
force-pushed
the
infer-return-type
branch
from
22e8594 to
391ee91
Compare
carljm
reviewed
May 22, 2025
crates/ty_python_semantic/resources/mdtest/function/return_type.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/function/return_type.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
mtshiba
force-pushed
the
infer-return-type
branch
from
ac2b3d2 to
a778047
Compare
mtshiba
force-pushed
the
infer-return-type
branch
from
a72b60b to
bbb07d6
Compare
mtshiba
force-pushed
the
infer-return-type
branch
from
bbb07d6 to
e7024ae
Compare
Contributor
Author
|
Parts not directly related to this PR have been moved to #20566, which aims to make recursive type inference converge more generally. The handling of recursive type inference in this PR should be replaced with something more sophisticated, but I think it's best to land this PR first and complete it in #20566. |
mtshiba
commented
Sep 25, 2025
carljm
reviewed
Sep 25, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This is looking good! Some comments and questions inline.
I will be a bit hesitant to land this before type-of-self lands, because type-of-self is critical, and the more new type information we introduce, the more blockers we encounter there. (This already happened with the dict literal inference PR.)
Comment on lines
+626
to
+630
| /// When using `ScopeInference` during type inference, | ||
| /// use this method to get the cycle recovery type so that divergent types are propagated. | ||
| pub(super) fn cycle_recovery(&self) -> Option<Type<'db>> { | ||
| self.fallback_type() | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we need this method if it's just an alias for fallback_type() method?
| if let Some(previous_cycle_value) = callee_ty.infer_return_type(db) { | ||
| // In fixed-point iteration of return type inference, the return type must be monotonically widened and not "oscillate". | ||
| // Here, monotonicity is guaranteed by pre-unioning the type of the previous iteration into the current result. | ||
| union = union.add(previous_cycle_value.recursive_type_normalized(db, &visitor)); |
Contributor
There was a problem hiding this comment.
Suggested change
| union = union.add(previous_cycle_value.recursive_type_normalized(db, &visitor)); | |
| // TODO: this means that every `infer_return_type` creates a self-cycle that must be iterated. | |
| // Instead, have Salsa provide both previous and current value so we can do this in the | |
| // recovery function. | |
| union = union.add(previous_cycle_value.recursive_type_normalized(db, &visitor)); |
| // In the following cases, the bound type may not be the same as the RHS value type. | ||
| if let AnyNodeRef::ExprAttribute(ast::ExprAttribute { value, attr, .. }) = node { | ||
| let value_ty = self.try_expression_type(value).unwrap_or_else(|| { | ||
| let value_ty = self.try_expression_type(value.as_ref()).unwrap_or_else(|| { |
Contributor
There was a problem hiding this comment.
We could probably implement Into<ExpressionNodeKey> for &Box<Expr> to avoid the need for this?
Comment on lines
+4765
to
+4774
| self.infer_optional_expression(ret.value.as_deref(), TypeContext::default()); | ||
| let range = ret | ||
| .value | ||
| .as_ref() | ||
| .map_or(ret.range(), |value| value.range()); | ||
| let expression = ret | ||
| .value | ||
| .as_ref() | ||
| .map(|expr| ExpressionNodeKey::from(&**expr)); | ||
| self.record_returnee(expression, range); |
Contributor
There was a problem hiding this comment.
Why do we need to store the expression key instead of storing the type directly?
| .filter_map(|returnee| { | ||
| match returnee | ||
| .expression | ||
| .map_or(Type::none(self.db()), |expression| { |
Contributor
There was a problem hiding this comment.
Why would we fallback to Type::none here?
It seems like we should just expect here, we should always have inferred a type for every returnee expression.
| if visitor.level() == 0 && self == visitor.div { | ||
| // int | Divergent = int | (int | (int | ...)) = int | ||
| return Type::Never; | ||
| } else if visitor.level() >= 1 && self.has_divergent_type(db, visitor.div) { |
Contributor
There was a problem hiding this comment.
If the level >= 1 and self does not contain visitor.div, do we need to keep walking self recursively, or could we just short-circuit and return self? It seems we will not change anything in the visit.
It's not really clear to me that we even need all the many added recursive_type_normalized methods in this PR, and all the level and visit_no_shift stuff either. Wouldn't it be an equivalent implementation if we just match on unions here, visit all their elements, and for any other type, check has_divergent_type and return div if so?
| } | ||
|
|
||
| /// Infers this method scope's types and returns the inferred return type. | ||
| #[salsa::tracked(cycle_fn=return_type_cycle_recover, cycle_initial=return_type_cycle_initial, heap_size=get_size2::heap_size)] |
Contributor
There was a problem hiding this comment.
Does this need to be a separate Salsa query, or could we just infer the return type of self.function(db)?
We could add the union with Unknown here rather than inside ScopeInference::infer_return_type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
This PR closes astral-sh/ty#128.
FunctionType::inferred_return_typeis added to infer the return type of a function when its return type is not specified.TODOs
- [ ] infer generator's return type- [ ] infer coroutine's return type- [ ] infer lambda function's return typeTest Plan
New test cases are added to
mdtest/function/return_type.md.