Repository navigation
[ty] Return type inference - #29253
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 98.24%. The percentage of expected errors that received a diagnostic held steady at 98.24%. The number of fully passing files held steady at 134/146. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
|
| Project | Old Time | New Time | Change |
|---|---|---|---|
paroxython |
0.05s | 0.08s | +61% |
pegen |
0.05s | 0.07s | +58% |
5757658 to
2768ea3
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
dc3ba2c to
80251ba
Compare
80251ba to
f2daf73
Compare
f2daf73 to
3343aa2
Compare
3343aa2 to
dc6472c
Compare
dc6472c to
7e37569
Compare
7e37569 to
b9f44c5
Compare
| // def get(self): | ||
| // return self.value | ||
| // | ||
| if let Some(class) = class |
There was a problem hiding this comment.
Hmm, does this assume that the method is a class method, and not a static method? In this case, it should be returning int:
class Box[T]:
@staticmethod
def identity(value: T):
return value
x: int = Box[int].identity(1) # error: T@Box | Unknown| // Add `None` if the function can implicitly return it | ||
| if is_end_of_scope_reachable(db, self.scope()) { | ||
| return_type = return_type.add(Type::none(db, env)); | ||
| } |
There was a problem hiding this comment.
It looks like protocols and abstract method stubs contribute None right now -- maybe they should be excluded?
class Provider(Protocol):
def get(self): ...
class Impl:
def get(self) -> int:
return 1
def use(p: Provider) -> int:
return p.get() # error: None | Unknown
use(Impl()) # acceptedThere was a problem hiding this comment.
Great call. I added two tests for this and excluded abstract methods and protocol methods completely, for now. I attempted to explain the reasoning for doing so in the mdtests.
sharkdp
left a comment
There was a problem hiding this comment.
Thank you for the review, great findings.
| } | ||
|
|
||
| Some(return_type.build()) | ||
| } |
There was a problem hiding this comment.
It is, yes. I also spent some time to make it look straightforward, so good that you're noticing that 😄
|
|
||
| # TODO: remove this explicit `Unknown` return type when type inference does not expose | ||
| # the internal `Wrapped` class anymore. | ||
| def personify(cls) -> Unknown: |
There was a problem hiding this comment.
so Animal below would be inferred as <class 'Wrapped'>. This is exactly the same scenario that is described in the "Internal structure that should not leak out" mdtest in return_type_inference.md. These nested classes should not be exposed, because at runtime, a new class would be created for every call of personify, and <class 'Wrapped'> doesn't represent that.
I have a fix for this already, I just wanted to focus this initial PR on the basic implementation.
| // Add `None` if the function can implicitly return it | ||
| if is_end_of_scope_reachable(db, self.scope()) { | ||
| return_type = return_type.add(Type::none(db, env)); | ||
| } |
There was a problem hiding this comment.
Great call. I added two tests for this and excluded abstract methods and protocol methods completely, for now. I attempted to explain the reasoning for doing so in the mdtests.
| // def get(self): | ||
| // return self.value | ||
| // | ||
| if let Some(class) = class |
|
Great work! |
Co-authored-by: Shunsuke Shibayama <[email protected]>
Summary
This PR adds return type inference as a new feature to ty. The feature is currently disabled by default. Users can activate the feature by setting
analysis.return-type-inferenceto"always".In a standalone function, the inferred return type is simply the union of the types of all reachable
returned expressions, as shown in the image on the right. If control flow can reach the end of the function scope, we addNoneto the union. In a (non-final) method on a (non-final) class, we additionally union withUnknownto allow child classes to widen the intended return type. For example, imagineHere, we infer a return type of
None | UnknownforTaskBase.compute. Otherwise, if we inferredNone, we would prohibit child classes from returning something else, which seems undesirable. Another interesting scenario is when the base class method is annotated, but the child class method is not:Here, we respect the base class definition and infer
Child.methodas returningint, notint | Unknown.This work is based on #17371 by @mtshiba (thank you!)
There are a few notable things that are not implemented here:
my_function()may be inferred asint, butmy_functionitself would still be inferred asdef my_function() -> Unknown. We have existing tests with TODOs for this and I plan to tackle this in a follow up.functools.wrapsthat's present in the ecosystem report. This will hopefully be solved by [functools] Support binding wrapped methods python/typeshed#16515NotImplementedwhich hasn't been resolved yet. The solution is described here.Divergentas a provisional value. I did think about using the new μ-types, but that currently leaves too many questions unanswered: How would we represent an anonymous μ-type? How would the (provisional) recursive markerRinteract with other types during inference (especially since those operations require "closed" recursive types, andRwould still be open)?closes astral-sh/ty#128
Test Plan