-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Infer type predicates from function bodies using control flow analysis #57465
Infer type predicates from function bodies using control flow analysis #57465
Conversation
The TypeScript team hasn't accepted the linked issue #16069. If you can get it accepted, this PR will have a better chance of being reviewed. |
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.
I'll look into the self-check errors.
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.
This file is deleted because we now (correctly) infer a type guard that's compatible with Array.isArray
, which makes the error go away!
|
||
// String escaping issue (please help!) | ||
function dunderguard(__x: number | string) { | ||
>dunderguard : (__x: number | string) => ___x is string |
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.
Note the three _
s on the type predicate. This is a bug caused by the param.name.escapedText as string
type assertion. I'm not sure how to fix it.
@@ -65,5 +65,5 @@ verify.quickInfos({ | |||
7: "(method) GuardInterface.isFollower(): this is FollowerGuard", | |||
13: "let leaderStatus: boolean", | |||
14: "let checkedLeaderStatus: boolean", | |||
15: "function isLeaderGuard(g: RoyalGuard): boolean" | |||
15: "function isLeaderGuard(g: RoyalGuard): g is LeadGuard" |
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.
This is correct, a type predicate now flows where it did not before.
@typescript-bot perf test this faster |
Heya @RyanCavanaugh, I've started to run the faster perf test suite on this PR at e2684f1. You can monitor the build here. Update: The results are in! |
I'm thinking this will break things like... let isNumberable = (x: string | number) => typeof x === 'number';
isNumberable = x => !isNaN(parseInt(String(x))); ...as a function returning |
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@fatcerberus true, but that can also cut the other way! See the deleted errors file for Ryan pointed out on another issue that inferring a type guard breaks this sort of code as well #38390 (comment) const a = [1, "foo", 2, "bar"].filter(x => typeof x === "string");
a.push(10); // ok now, error with my PR I assume the +3.89% check time on compiler-unions is bad? Is there any information on how I can run this locally and see what's going on? The CI/self-check found a flaw in my criteria for inferring a type predicate. I actually need to be even more strict! I should not infer a type predicate for this function, even though function isShortString(x: unknown) {
return typeof x === 'string' && x.length < 10;
}
declare let str: string;
if (isShortString(str)) {
str; // string
} else {
str; // never
} This is actually quite interesting. My approach has no way of distinguishing function isString(x: unknown) {
return typeof x === 'string';
} They both have I need to think a little more about whether this is fixable or if it's a flaw with this whole approach. |
Ouch, that's tricky. If there's a |
@fatcerberus yeah, see updated comment. I need to think about it a little more, but this might be a deal-breaker for this approach. |
nevermind, I'm dumb, you haven't ruled out all strings. ignore this |
I'm feeling somewhat hopeful that this approach can be salvaged. Instead of requiring that:
what we actually need is:
i.e. that this relationship holds for all subtypes of the declared type. I'm hoping that in practice this just means I need to check that the relationship holds for T=initType and T=trueType, i.e. add one more check. Again, this would be much easier if a function could return a different type predicate for the true and false cases! |
That fix seemed to work and all tests pass, so we should be back in business. This is going to be a bit slower than the version that @typescript-bot tested earlier but I'm feeling more confident that it's correct. |
@typescript-bot test top200 @typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at d0e385e. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at d0e385e. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at d0e385e. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at d0e385e. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the faster perf test suite on this PR at d0e385e. You can monitor the build here. Update: The results are in! |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: lodash
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Some errors reported by the bot could be seen as mistakes in the user's code. Why the result of a array, filtered to be an array of numbers, should be typed as anything else than an array of number ? Building on top of the current approach, but increasing complexity : A "read" type and a "write" type, for mutable objects. The read type would represent the latest known type at a point in the control flow. In the case of the array filter, this would mean "this array can hold strings and numbers, but at this point, it only holds numbers". This is clearly a whole new thing. Bonus : at the end of the function, if the read type is the same as the write type, and the object is returned, then you can infer a type predicate. This part is out of the scope of your current PR, if I understood correctly. |
This idea is great! I seem to have come up with an implementation for fallback that doesn't require compilation support. declare const notMatched: unique symbol
function isWhat<Input = unknown, T = never>(
match: (input: Input, _: typeof notMatched) => T | typeof notMatched
): (
(x: Input) => x is [T] extends [Input] ? Input & T : never
) {
return ((x: any): boolean => {
try {
return match(x, notMatched) !== notMatched
} catch (e) {
if ([notMatched, void 0, null, TypeError].includes(e as any)) {
return false
}
throw e
}
}) as any
}
const strs0 = [1, '1', true].filter(
// ^? string[]
isWhat((t, _) => typeof t === 'string' ? t : _)
)
const strs1 = [1, '1', true].filter(
// ^? string[]
isWhat(t => {
if (typeof t === 'string') return t
throw void 0
})
) However, this requires an import. But we can simplify the usage of this function by using webpack or built-in global functions. |
i'd love to be able to this would allow me to do stuff like this, or even a |
@btoo adding support for multiple returns isn't that difficult. I have a branch that implements it here: main...danvk:TypeScript:multi-return-predicate
|
Hi, is it intentional that this doesn't work using |
Yes, |
I would expect it to behave identically to |
|
In particular, see #50387 (comment) for an explanation of why this would have some surprising negative consequences. |
I'm feeling a bit confused about why a type guard for truthiness checking could not be performed. Currently I can filter out false types by manually add: const isTrue = <T>(o: T | undefined | null | false | 0 | ""): o is T => !!o;
const array = [0, "", 5, "hello", false as const, null, undefined]
const result: (string | number)[] = array.filter(isTrue) but not with |
@dest1n1s that version of const isTrue = <T,>(o: T | undefined | null | false | 0 | ""): o is T => !!o;
let x = Math.random() > 0.5 ? 1 : 0;
if (isTrue(x)) {
x // number -- ok
} else {
x // never -- wrong! could be 0!
} See my blog post, The The Hidden Side of Type Predicates. To safely infer a type guard for |
Any explanation why Boolean doesn't work? I see Boolean discussion how about just |
Cross-linking to #42384 (feature request to narrow parent object by property guard) |
Fixes #16069
Fixes #38390
Fixes #10734
Fixes #50734
Fixes #12798
This PR uses the TypeScript's existing control flow analysis to infer type predicates for boolean-returning functions where appropriate. For example:
This currently has an inferred return type of
boolean
, but with this PR it becomes a type predicate:I filed #16069 seven years ago (!) and thought it would be interesting to try and fix it. It turned out to be cleaner and simpler than I thought: only ~65 LOC in one new function. I think it's a nice win!
How this works
A function is a candidate for an inferred type guard if:
boolean
.return
statement and no implicit returns (this could potentially be relaxed later).If so, then the function looks something like this:
For each parameter, this PR determine what its flow type would be in each branch if the function looked like this instead:
if
trueType != T
then we have a candidate for a type predicate.We still need to check what a
false
return value means because of the semantics of type predicates. If we have:then
x
is astring
if this function returnstrue
. But if it returnsfalse
thenx
must be anumber
. In other words, in order to be a type predicate, a function should returntrue
if and only if the predicate holds.We can test this directly by plugging in
trueType
to the synthesizedif
statement and seeing what's left in theelse
branch:If it's
never
then we've demonstrated the "only if" condition.Note that the previous versions of this PR did slightly different checks. The original version had problems with subtypes and my initial fix made more calls to
getFlowTypeOfReference
than was necessary. This version directly tests the condition that we want.Wins
TypeScript is now able to infer type guards in many places where it's convenient, e.g. calls to
filter
:Since this piggybacks off of the existing flow type code, all forms of narrowing that TypeScript understands will work.
There are a few other non-obvious wins:
Type guards now flow
They also compose in new ways:
This is a gentle nudge away from truthiness footguns
We don't infer a type guard here if you check for truthiness, only if you check for non-nullishness:
This is because of the
false
case: if the truthiness test returnsfalse
, thenx
could be0
. Until TypeScript can represent "numbers other than 0" or it has a way to return distinct type predicates for thetrue
andfalse
cases, there's nothing that can be inferred from the truthiness test here.If you're working with object types, on the other hand, there is no footgun and a truthiness test will infer a predicate:
This provides a tangible incentive to do non-null checks instead of truthiness checks in the cases where you should be doing that anyway, so I call this a win. Notably the example in the original issue tests for truthiness rather than non-null.
Type guards are more discoverable
Type predicates are an incredibly useful feature, but you'd never learn about them without reading the documentation or seeing one in a declaration file. Now you can discover them by inspecting symbols in your own code:
This makes them feel like they're more a part of the language.
Inferred type guards in interfaces are checked
While this PR defers to explicit type predicates, it will check an inferred predicate in this case:
Interesting cases
The identity function on booleans is, in theory, a type guard:
This seems correct but not very useful: why not just test the boolean? I've specifically prohibited inferring type predicates on boolean parameters in this PR. If nothing else this significantly reduces the number of diffs in the baselines.
Here's another interesting case:
If this returns
true
thenx
is astring
. But if it returnsfalse
thenx
could still be astring
. So it would not be valid to infer a type predicate in this case. This is why we can't just check the trueType. In general, combining conditions like this will prevent inference of a type predicate. It would be nice if there were a way for a function to return distinct type predicates for the true and false cases (#15048). This would make inference much more powerful. But that would be a bigger change.I remember RyanC saying once that a function's return type shouldn't be a restatement of its implementation in the type system. In some cases this can feel like a move in that direction:
Like any function, a type guard can be called with any subtype of its declared parameter types. We need to consider this when inferring a type guard:
The issue here is that
x
narrows tostring
andunknown
if you inline this check in anif
/else
, andExclude<unknown, string> = unknown
. But we can't infer a type predicate here becauseisShortString
could be called with astring
. This broke the test originally used in this PR, see this comment.A function could potentially narrow the parameter type before it gets to the
return
, say via an assertion:It's debatable what we should do in this case. Inferring
x is string
isn't wrong but will produce imprecise types in theelse
branch (which will includeDate
). This PR plays it safe by not inferring a type predicate in this case, at the cost of an extra call togetFlowTypeOfReference
.Breaks to Existing Code
Most new errors in existing code are more or less elaborate variations on #38390 (comment)
In other words, this PR allows TS to infer a narrower type for an array or other variable, and then you do something that required the broader type: pushing, reassigning, calling
indexOf
. See this comment for a full run-down of the changes that @typescript-bot and I have found.Performance
We're doing additional work to infer type predicates, so performance is certainly a concern. @typescript-bot found the most significant slowdown with Compiler-Unions, a +1.25% increase in Check time.
Some fraction of the slowdown is from additional work being done in
getTypePredicateFromBody
(my addition), but some is also because TypeScript is inferring more precise types in more places thanks to the newly-detected type guards.If there are performance concerns with the current implementation, there are a few options for reducing its scope:
Array.prototype.filter
).Possible extensions
There are a few possible extensions of this that I've chosen to keep out of scope for this PR:
this
. It's possible for a boolean-returning method to be athis
predicate. This should be a straightforward extension of this PR.boolean
. Ifdates
is(Date|null)[]
, thendates.filter(d => d)
is a fine way to filter thenull
s. This PR makes you writedates.filter(d => !!d)
. I believe this is a limitation of type predicates in general, not of this PR.