Infer type predicates for functions with multiple returns #58154
Conversation
This file contains 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
typescript-bot
added
the
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
|
👀 So minimal perf impact and no breaks beyond the self-check circularity issue. Maybe this is a good idea? 🤔 |
|
@typescript-bot test top800 |
|
@jakebailey Here are the results of running the top 800 repos comparing Everything looks good! |
|
I put up #58173, which reports an error whenever it infers a type predicate for a function with multiple |
|
Results of the experiment:
So not many functions match this pattern in the wild, but a few do and there's also not a big perf impact. Up to TS team whether this is a helpful generalization. See @jakebailey's comment #58173 (comment) |
|
I would love to have this, since it allows splitting up type predicates instead of having to write them as a single long expression. I'm surprised that things like function isArrayOfStrings(x: unknown): x is string[] {
if (!x instanceof Array) return false;
return x.every((y) => typeof y === 'string');
}didn't occur more in your experiment, since I've seen it pretty often for more complicated type predicates. |
|
@anka-213 if there are examples in the wild that you can point to, that would be helpful. This makes me realize that my experiment (#58173) would not tell us about existing type predicates with 2+ returns, since the predicate inference code doesn't run if there's an explicit return type annotation. In other words, it would not have detected your FWIW you can break up the long expression by factoring out a variable, you just can't break up the control flow: // function isArrayOfStrings(x: unknown): x is string[]
function isArrayOfStrings(x: unknown) {
const isArray = Array.isArray(x);
return isArray && (x.every((y) => typeof y === 'string'));
} |
danvk
changed the title
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
I've merged upstream changes and brought the implementation more in line with I can open an issue like typescript-bot suggests if that would be a more appropriate way to discuss the merits of this extension. |
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Other than the compiler-unions emit time, this looks like ~0 impact. I can take a look at whether there's some giant type predicate being emitted that would account for that, but I'm skeptical since we didn't see change in this number back in April and the behavior of the PR shouldn't be different. |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-on to #57465
The original PR restricted itself to functions with a single
return. This PR lifts that restriction.This allows us to infer predicates for constructs like this:
or
How this works
Type predicate inference works by rewriting
returnstatements from:to:
If
TrueType != InitTypethen we have a candidate for a type predicate. To generalize this to multiple returns, we determine the TrueType at each return statement and union them. These are all the parameter types for which the function can return true.To verify the "if and only if" semantics of type predicates, we feed the type predicate type back into the function and look at all the false cases. For a valid type predicate, these should all be
never.I've also special-cased
return trueandreturn false. These weren't especially relevant whengetTypePredicateFromBodyonly allowed onereturn, but they're very relevant now.Why this might be a good idea
Like #57465, this should only produce valid type predicates, which should result in more precise types.
This makes inferred type predicates less sensitive to seemingly irrelevant refactors, such as:
These kinds of refactors don't affect inferred return types, so it may be surprising that they affect inferred type predicates.
While this does result in predicate inference running on more functions, the perf impact seems to be minimal.
Why this might not be a good idea
Multi-return type predicates are not as common as single-return type predicates:
So the impact will be relatively small compared to the original change.
This does make the type predicate inference code a bit harder to follow since we're always working with
Type[]instead ofType.Moreover, I got a new circularity error inThis error no longer happens.emitter.ts. This would also have been a new circularity error with #57465, but the type predicate inference code didn't run because one of the functions had two returns. This change increases the odds that this happens on existing code.