Infer AssertsIdentifier type predicates #58495
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| // An assignability check covers this, but a void initType can become an undefined type through control flow analysis. | ||
| // Since void is not assignable to undefined, we patch initType to handle this, too. | ||
| const assertedType = getUnionType(typesAtReturn, UnionReduction.Subtype); | ||
| const patchedInitType = mapType(initType, t => t.flags & TypeFlags.Void ? undefinedType : t); |
There was a problem hiding this comment.
This is because void changes to undefined in this function, and undefined is not assignable to void:
function booleanOrVoid(a: boolean | void) {
if (typeof a === "undefined") {
a
}
a // undefined
}This feels like a hack, though, and I'd love to change it if there's a cleaner fix.
| @@ -954,14 +954,17 @@ declare function foo({ value1, test1, test2, test3, test4, test5, test6, test7, | |||
| test8?: any; | |||
| test9?: any; | |||
| }): void; | |||
| declare function fa1(x: [true, number] | [false, string]): void; | |||
| declare function fa1(x: [true, number] | [false, string]): asserts x is [false, string]; | |||
There was a problem hiding this comment.
This one is a little weird (the function never returns), but it follows from the types: TS understands that for(;;) {} is an infinite loop, but not while(!!true) {}.
| @@ -1192,8 +1192,8 @@ function unknownNarrowing(x: unknown) { | |||
| } | |||
|
|
|||
| function keyofNarrowing<S extends { [K in keyof S]: string }>(k: keyof S) { | |||
| >keyofNarrowing : <S extends { [K in keyof S]: string; }>(k: keyof S) => void | |||
| > : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^ ^^^^^^^^^ | |||
| >keyofNarrowing : <S extends { [K in keyof S]: string; }>(k: keyof S) => asserts k is (keyof S & number) | (keyof S & symbol) | (keyof S & string) | |||
There was a problem hiding this comment.
This is the one loss: the other changes are all valid predicates.
// asserts k is (keyof S & number) | (keyof S & symbol) | (keyof S & string)
function keyofNarrowing<S extends { [K in keyof S]: string }>(k: keyof S) { ... }This should not be an assertion function because:
(keyof S & number) | (keyof S & symbol) | (keyof S & string)
= keyof S & (number | symbol | string)
= keyof S & PropertyKey
= keyof S
TS isn't able to figure that out, however, so we get a meaningless assertion predicate.
| @@ -68,7 +68,7 @@ import { x } from "foo"; | |||
| import { x2, used2 } from "foo"; | |||
| used1; used2; | |||
|
|
|||
| function f() { | |||
| function f(a, b) { | |||
There was a problem hiding this comment.
This diff is clearly wrong and a bug in my PR. There's a similar issue with the existing inferred type predicates change, see #58493. The fix for that should also apply here.
This feels like it’s going to turn a lot of functions into assertions that were never intended to be, just because a parameter was narrowed somewhere in the function. |
It's not enough for the parameter to be narrowed somewhere in the function. The function has to throw in a way that's tied to that narrowing. We could do an experiment similar to #58173 to see how often this triggers. |
|
Ah, okay - you didn’t mention that in the description in the OP, the algorithm you laid out was just that you check all the return points, not also the throwing points. |
The algorithm only looks at parameter types when you return, or (important!) at the implicit return at the end of a function. When you union those types, the only way I can think of that you'd get something distinct from the initial parameter type is by having a code path that throws or does the equivalent (calls another assertion function or Maybe an example would make it clearer how this works: function assertABC(x: 'A' | 'B' | 'C' | 'D' | 'E') {
if (x === 'A') {
return; // type of x here is 'A'
} else if (x === 'B' || x === 'C') {
throw new Error();
}
// implicit return; type of x here is 'D' | E'
}Unioning the types of If you take a look at the code for the algorithm, it's all about |
|
Ah, so it’s based on how TS re-merges control-flow branches by unioning the narrowed types back together, that’s clever. However do note - that can sometimes create an observably different type on the other side (see: subtype reduction, introducing intersections, etc.) |
|
Forgive me if you’ve accounted for all this already - I’m just spouting the caveats that come to mind when I think about this. |
I won't infer an assertion predicate so long as the initial parameter type is assignable to the asserted type, so superficial differences shouldn't matter. But there are some cases where TS can't figure out the relationship. There's one example of this in the baselines (see my comments on the code). There will probably be others in the wild. This change would feel less aggressive if I could detect whether there was a code path that throws in the function, but I couldn't find anything that quite did what I wanted. |
|
@typescript-bot test it for fun |
|
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 |
|
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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf impact looks pretty minimal… but what are those two new errors in VS Code? |
|
@jakebailey Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
Ouch. I forgot that was something the compiler checked for. The existence of that error means you have to be very careful about exactly what you infer as an assertion… |
|
Summary of breaks (#58495 (comment)):
The fix for all of these is to not infer type assertion predicates for methods or arrow functions because this can produce a confusing error at the call site. See #33622 for why. This definitely limits the scope of this PR: the latest version only runs on plain old function statements. I think the nicest win from this PR is that it lets type predicates naturally flow through assertion wrappers: declare function isFoo(x: any): x is Foo;
// function inferFromType(x: unknown): asserts x is Foo
function inferFromType(x: unknown) {
if (!isFoo(x)) {
throw new Error();
}
} |
This is dope 🚀 |
|
@typescript-bot test it |
|
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 |
|
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 Something interesting changed - please have a look. Details
|
|
@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! |
|
At first I thought the minimatch break had something with old-fashioned JS constructor function statements that were inferred to be type predicates. But it turned out to be more straightforward. The relevant code is here and all the failures are calls to this function: var MAX_PATTERN_LENGTH = 1024 * 64
var assertValidPattern = function (pattern) {
if (typeof pattern !== 'string') {
throw new TypeError('invalid pattern')
}
if (pattern.length > MAX_PATTERN_LENGTH) {
throw new TypeError('pattern is too long')
}
}So I also need to exclude function expressions from inference, not just arrow functions. |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
Ironically, inferring |
Right. So far as I'm aware, the algorithm is correct: it will never infer a type assertion predicate that's inaccurate. But there are many situations (methods, arrow functions, function expressions) where it will push you right into TS2775 / TS2776. |
|
@typescript-bot test it |
|
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 tests comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@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! |
This extends the type predicate inference from #57465 to cover "asserts identifier" predicates as well. For example:
or:
The constraints for inference are:
voidorundefined.The criterion winds up being a bit simpler than it was in #57465:
returnsite (including an implicit return at the end of the function).I wound up implementing "different than" with an assignability check since branching constructs can sometimes change the representation of a parameter's type in superficial ways, e.g.
unknowncan become{} | null | undefined.There's no equivalent of the "false check" that we needed to infer identifier type predicates. Assertion predicates are inherently one-sided. If they fail, your code will throw. This means that we can infer assertions even when the negative type can't be represented. For example:
This wouldn't work as a type predicate (
x is string) because you can't represent "all types except short strings." But it's fine as an imprecise assertion, since there's no way to observe the negative case.A few other things to note:
assertFoocallsisFoo).asserts x is T, notasserts x. The latter would also be interesting, but that feels like a different problem.It's possible that the "asserted type" really is the same as the parameter's declared type, but TS isn't able to figure that out. This happened for one of the baselines. In this case we'll infer a useless type predicate (
function f(x: T): asserts x is T). This is noisy but won't produce spurious errors. There's one example of this in the baselines.I didn't observe any slowdowns from this change locally and there's some reason to hope that's the case. For inferred type predicates, the expensive part was the "false" checks (see #57465 (comment)). There's no need to do that for assertion functions, though, so we won't pay that price. On the other hand, there might be more implicitly
void-returning functions than implicitlyboolean-returning functions, so we'll see!