Single-expression only type predicates #57552
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
Heya @RyanCavanaugh, I've started to run the regular perf test suite on this PR at 4970249. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @RyanCavanaugh, I've started to run the faster perf test suite on this PR at 29cdadc. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Happy to see this caught your interest 😊 My suspicion is that at least some of the slowdown in Compiler-Unions is due to the extra work that happens indirectly because of the new type guards themselves, rather than the direct new work of determining which functions are type guards. I found this sort of "ablation test" useful in teasing these two apart: i.e. do all the work, but don't actually infer the type predicate. With my PR as of last week I got results that made sense: ~4% slowdown on the PR, ~1.7% slowdown on the ablation → ~2.3% of the slowdown is due to more elaborate type checking. |
|
Not just from a perf perspective, but my current take (subject to additional thinking) is that this is also a correctness / DX improvement. Any function that does anything but the type guard itself is likely to either a) have additional constraints that mean it's not really a type guard or b) only appear to guard as a sort of side effect of implementation. But I want to think about that more |
|
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Newest commit no-ops the result as suggested above (for perf testing only) |
|
@typescript-bot perf test this faster |
|
Heya @RyanCavanaugh, I've started to run the faster perf test suite on this PR at 466e186. You can monitor the build here. Update: The results are in! |
|
Running the two versions against #54148 and Definitely a legitimate inference, and maybe a little confusing that you'd change the return type by factoring out a local variable. But not a huge loss. |
|
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
As I mentioned on twitter this feature is very similar to flow's From a DevX perspective it is super annoying because you can't do small stylistic refactors like you mentioned like extracting a variable. So you ended up doing stuff like chaining guard functions to work around the limitation. function isBar(arg: Bar): %checks {
return arg.type === 'bar';
}
function isFoo(arg: Foo): %checks {
return isBar(arg.foo);
}Though difference between the flow solution and this TS one was that flow requires an explicit There's probably some scope for a lint rule to catch it (though it'd be a first of its kind and I'm not sure if it's fully possible) but it'd be cool if TS had more advanced rules. |
A few thoughts on how this feature compares with
|
I don't think TS really has this problem since you're not forced to infer the predicateness of the function; you can just write |
That's entirely true. FWIW flow also supports TS style type guard annotations now (but safer than TS cos flow validates the predicate matches the refinement). The thing that I'd be scared about as a user is "actions at a distance". Eg the small refactor of one function causes type errors in other files. Eg if your codebase relies upon inferred return types you could end up with a really spooky action at a distance as a refined variable changes type which changes the inferred return type of a function which causes a type error. |
That's a pretty good point and makes me think more about the expression vs declaration distinction -- function expressions are almost always "local", whereas declarations are much more likely to be exported with an inferred type. People might not realize they're exporting a type predicate and it might not be their intent. |
|
@bradzacher - There's probably some scope for a lint rule to catch it (though it'd be a first of its kind and I'm not sure if it's fully possible) but it'd be cool if TS had more advanced rules. @RyanCavanaugh - People might not realize they're exporting a type predicate and it might not be their intent. Suggestion 1I hear Brad saying is that "inference" can be turned around to be a confirmation of the users declared typePredicate, to catch cases where a mismatch was not intended. Using it that way wouldn't cause the problem that Ryan mentions. I also hear Brad saying use it only on special occasions, like a lint rule, avoiding the performance issue, at least while editing. I wonder if this functionality (now purposed to be a typePredicate checker, as opposed to a silent inferr-er) would be hard to separate out as a separate API function for use by lint? Yes, probably hard to separate it out from a whole compile. In that case how about adding a command line flag to allow a "super check", setting that flag to coincide with the occasions when lint would be used immediately before or afterwards anyway. A super check run could include typePredicate checking, and/or any similar expensive checking too expensive for a regular run. Having to add Suggestion 2For some cases where the typePredicate would very be verbose/redundant, the user might prefer not to type it in. If there were some way in the server API to request an inferred type predicate (using the super check flag mentioned in 1?) then it could appear after Maybe I should have posted this on #57465. I guess I'll double post it - I can always delete one later. |
Variant on #57465 with tighter rules for inference:
Hoping this gets perf cost down to ~0% while still getting ~all of the value