-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Narrow generic conditional and indexed access return types when checking return statements #56941
Conversation
@typescript-bot perf test this |
Heya @gabritto, I've started to run the regular perf test suite on this PR at a3a54ce. You can monitor the build here. Update: The results are in! |
This comment was marked as outdated.
This comment was marked as outdated.
@typescript-bot perf test this |
Heya @gabritto, I've started to run the regular perf test suite on this PR at 1b7489f. You can monitor the build here. Update: The results are in! |
if (!Meteor.settings) { | ||
return undefined; // Error | ||
~~~~~~ | ||
!!! error TS2322: Type 'undefined' is not assignable to type 'HelperCond<I, string, T | undefined, RegExp, SettingComposedValue<T>[]>'. |
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.
It's too bad we don't give any elaboration here.
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.
Yes, I don't really know how we could provide more feedback when we can't narrow a return type. It's already not 100% clear whether the user is trying to narrow the return type or not, and even when we think they are, I can't think of a good mechanism to explain why we didn't narrow.
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.
Excellent write-up. I have a couple questions.
@@ -19081,14 +19098,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
if (checkType === wildcardType || extendsType === wildcardType) { | |||
return wildcardType; | |||
} | |||
const effectiveCheckType = forNarrowing && isNarrowingSubstitutionType(checkType) |
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.
Why do you need both forNarrowing
and to check the flags of the check type?
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.
forNarrowing = true
is passed by getConditionalTypeInstantiation
when we validate the conditional type is safe to narrow. It could be that the check type is a narrowing substitution type, but we don't want to narrow the conditional type because it has an invalid shape.
I think I did this because it was inconvenient and a bit confusing for me to call the conditional type validation function from inside getConditionalType
every time we loop to a new conditional type. I found it simpler to validate the whole conditional type in getConditionalTypeInstantiation
and have getConditionalType
loop on that type.
(t: Type) => | ||
getConditionalType( | ||
root, | ||
prependTypeMapping(checkType, getSubstitutionType(narrowingBaseType, t, /*isNarrowed*/ true), newMapper), |
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 think the changes here aren't mentioned in your implementation notes. Why do we need custom logic for distribution when we're narrowing?
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.
Good question.
We are using substitution types (which can be thought of as T & A
, where T
is the base type and A
the constraint) to convey a type parameter's narrowing. When instantiating a distributive conditional type in getConditionalTypeInstantiation
, if the conditional's check type is a substitution type like T & (A | B)
, the old logic would not distribute over this type, because it's a substitution type and not a union type. So, for distribution to happen, we have to take apart the T & (A | B)
into (T & A) | (T & B)
, and distribute over that.
The other special thing that we have to do is to take the intersection of the distribution result, as opposed to the union. This is because, if we narrow a type parameter T
to A | B
, and we have a conditional return type T extends A ? R1 : T extends B ? R2 : T extends C ? R3 : never
, then we don't know which branch of the conditional return to pick, if branch T extends A ? R1
, or branch T extends B ? R2
, so we have to check whether the return expression's type is assignable to both, i.e. assignable to R1 & R2
.
A semi-related note is that validating whether the conditional type has the right shape to be narrowed happens on-demand (and also at first as an optimization in checkReturnExpression
when we're deciding whether to narrow the return type). This is because, as we instantiate a type, we may produce new conditional types that we need to then decide are safe to narrow or not (see nested types case in dependentReturnType6.ts
test).
There's a bunch of details I haven't included in the implementation notes because I don't have a full list of them 😅. I'll add those.
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.
Would it make sense to always normalize union-containing substitution types into unions of substitution types before distributing?
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 haven't thought about it, but I assume it would break something somewhere in weird ways.
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.
something akin to that could help with some NoInfer
problems: #60271 (comment) :p
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 have this vague sense that it would be nice if the conditional-narrowing-specific logic didn’t affect the implementation of getConditionalTypeInstantiation
, but even if it always normalized substitution types, I don’t have a suggestion for how to extract mapTypeToIntersection
, so I guess it’s moot.
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.
something akin to that could help with some
NoInfer
problems: #60271 (comment) :p
I think it's worth investigating what would happen if we normalize substitution types with union constraints like we do for intersections with unions, especially if it fixes some problems. But I'll leave that for the future.
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.
Just one question and one small suggestion - this looks good~
// A narrowable conditional type is one that has the following shape: | ||
// `T extends A ? TrueBranch<T> : FalseBranch<T>`, in other words: | ||
// (0) The conditional type is distributive; | ||
// (1) The conditional type has no `infer` type parameters; |
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.
Why's that? Is something like
function f<T extends [number, string] | [string, number]>(arg: T): T extends [number, infer U] ? U : never {
if (typeof arg[0] === "number") {
return arg[1];
}
throw new Error("Got non numeric tuple");
}
for some reason not feasible to check? Is it just because we'd have to do inference between the true
branch type and the candidate return type to fill in the infer
type variable, and that's not implemented yet? Because I could equivalently write
function f<T extends [number, U] | [string, number], U extends string>(arg: T): T extends [number, U] ? U : never {
if (typeof arg[0] === "number") {
return arg[1];
}
throw new Error("Got non numeric tuple");
}
which afaik is valid under these rules. I think all infer
type parameters in types satisfying the rest of these rules can be rewritten like this pretty easily, which seems to imply the infer
itself aughta work, but I could be wrong.
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.
Basically I don't know how inference would work safely between the substitution type I'm using to represent a narrowed type parameter and the infer type parameters, given how inference works today.
In T extends [number, infer U] ? U : never
, if we instantiate T
with substitution type T & [number, string]
, we would infer from T & [number, string]
to [number, infer U]
.
Now, we can't simply infer string
for U
, because it would not be sound to narrow the return type to string
:
the caller can specify that T
is [number, "foo"]
and then U
is inferred to be "foo"
, not string
.
We'd have to infer something like T[1] & string
or simply T[1]
for U
for this to be safe, and I think we don't have the ability to do that.
@typescript-bot test it |
@gabritto Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Fixes #33912.
Fixes #33014.
Motivation
Sometimes we want to write functions whose return type is picked between different options, depending on the type of a parameter. For instance:
If we want to precisely express this dependency between the return type and the type of
nameOrId
, we have a few options.The first one is to use overloads:
However, if you make a mistake in the implementation of the function and return the wrong type, TypeScript will not warn you. For instance, if instead you implement the function like this:
then your function implementation doesn't respect the overload signatures, but TypeScript will not error.
The alternative to overloads is to use conditional types, like so:
However, while everything works out for the callers of
getObject
, in the implementation TypeScript errors on the return statements, because it compares the type of the return expression to the annotated conditional return type, andRecord<string, string[]>
is not assignable toT extends undefined ? Record<string, string[]> : never
.Solution: conditional return type narrowing
For this PR, I propose a way of checking return statements that understands cases like above.
The idea is that, for the function above:
when checking the return statement
return record
, TS will know thatgroup
has typeundefined
. TS will also know that type parameterT
corresponds exactly to the type ofgroup
. Combining those two pieces of information, TS will know that, inside that branch, the expected return type has to beRecord<string, string[]>
(or a supertype). Then, instead of checkingreturn record
's type againstT extends string ? string[] : T extends undefined ? Record<string, string[]> : never
, it will checkreturn record
's type against theRecord<string, string[]>
branch of the conditional, i.e. a narrowed version of the conditional type. Then there will be no error on the return statement. In the same manner, when checking the return statementreturn array
, TS will know thatgroup
has typestring
, and that therefore it can check the type of the return statement against thestring[]
branch of the conditional return type.For now, we can think of it like this: when we check return statement
return record
, we see thatgroup
has narrowed typeundefined
. Then, we plug that information back into the return type by instantiatingT extends string ? string[] : T extends undefined ? Record<string, string[]> : never
withT
replaced byundefined
.Restrictions
Conditional types
Reasoning about conditional types is pretty tricky. In general, given a function
f(...args): SomeConditionalType
whose return type is some (generic) conditional type, we are not able to do the special check proposed above, because it wouldn't be safe.We need to place some restrictions on what the conditional return type looks like in order for TS to safely analyze it.
We can safely analyze a conditional return type that has the shape
T extends A ? AType : T extends B ? BType : never
.This means the conditional type needs to be distributive (i.e. its check type is a naked type parameter
T
) and havenever
as its false-most type, and it cannot haveinfer
type parameters (e.g. it cannot beT extends [infer A] ? AType : T extends B ? BType : never
).Intuitively, we can think of this conditional type shape as reflecting the kind of code one would write in the implementation of such a function.
In addition to the previous restrictions, the type parameter constraint has to be a union type, and the extends types of the conditional (
A
andB
above) have to be constituents of the type parameter's union constraint (T
above), like so:This is because, to narrow the return type, we first need to narrow the type of
param
(more on that below). When we narrow the type ofparam
, in a lot of scenarios, we will start from its type,T
, or in this case its type constraint,A | B
. Then, we will further narrow that type based on information from control flow analysis, e.g. to pick eitherA
orB
(seegetNarrowableTypeForReference
inchecker.ts
).Therefore, in this typical case, narrowing
param
means we will end up with a type that is eitherA
,B
, or a subtype of those. In turn, when we plug this narrowed type back into the conditional return type, this means we will be able to pick a branch of the conditional type and resolve it. e.g. if the narrowed type ofparam
isA
, the conditional type will resolve toAType
.These additional restrictions are a heuristic meant to capture the "happy path" of narrowing in TS. For instance, if the type parameter's constraint is not a union, then we might have a case like the below:
(Note
unknown
is conceptually equivalent to{} | null | undefined
, but writingT
s constraint as a union instead ofunknown
makes narrowing work.)Aside: why
never
A common way of trying to write a conditional return type is like the following:
This example works fine and it would be safe for TS to allow that function implementation. However, in general, it is not safe to allow this pattern of conditional return type. Consider this case:
The problem boils down to the fact that, when a conditional return type resolves to its false branch, we can't know if the check type is related or not to the extends type. For the example above, when we plug in
{ a: undefined }
forT
inT extends { a: string } ? string : number
, then we fall into the false branch of the conditional, which is desired because{ a: undefined }
does not overlap{ a: string }
. However, when we plug in{ a: string | undefined }
forT
inT extends { a: string } ? string : number
, we fall into the false branch of the conditional, but this is not desired because{ a: string | undefined }
overlaps{ a: string }
, and therefore the return type could actually bestring
.Resolving a conditional type to its false-most branch of a conditional type doesn't provide TS with enough information to safely determine what the return type should be, and because of that, narrowing a conditional return type requires the false-most branch of the conditional to be
never
.Type parameter references
As hinted at above, to narrow a conditional return type, we first need to narrow a parameter, and we need to know that the type of that parameter uniquely corresponds to a type parameter.
Revisiting our example:
To narrow the return type
T extends string ? string[] : T extends undefined ? Record<string, string[]> : never
, we first narrow the type ofgroup
inside theif
branch, and then we can use that information to reason about what typeT
could be replaced with. This only works because the declared type ofgroup
is exactlyT
, and also because there are no other parameters that use typeT
. So in the following cases, TS would not be able to narrow the return type, because there is no unique parameter to whichT
is linked:Indexed access types
The reasoning explained above for conditional types applies in a similar manner to indexed access types that look like this:
So cases like this, where the return type is an indexed access type with a type parameter index, are also supported by this PR. The thinking is similar: in the code above, when we are in the
if (str === "t") { ... }
branch, we knowstr
has type"t"
, and we can plug that information back into the return typeF[T]
which resolves to typenumber
, and similarly for theelse
branch.Implementation
The implementation works roughly like this:
When checking a return statement expression:
SomeType[T]
orT extends A ? AType : T extends B ? BType : never
, and what parameters the type parameters are uniquely linked to, among other requirements. If any of those requirements is not met, we don't continue with narrowing.T
that is uniquely linked to parameterparam
. To narrow the return type, we first need to obtain the narrowed type forparam
at the return statement position. Because, in the source code, there might not be an occurrence ofparam
at the return statement position, we create a synthetic reference toparam
at that position and obtain its narrowed type via regular control-flow analysis. We then obtain a narrowed typeN
forparam
. (If we don't, we'll just ignore that type parameter).T extends A ? AType : T extends B ? BType : never
,T
is linked toparam
, andparam
has narrowed typeN
, we will instantiate the return type withT
replaced byT & N
(as a substitution type).Instantiation
As mentioned above, the process of narrowing a return type is implemented as instantiating that return type with the narrowed type parameters replaced by substitution types. Substitution types can be thought of as
T & A
, whereT
is the base type andA
the constraint type. There are a few changes made to instantiation to make this work:ObjectFlags.IsNarrowingType
flag, which corresponds to substitution types created by return type narrowing.getConditionalTypeInstantiation
, andgetConditionalType
.getConditionalTypeInstantiation
is responsible for distributing a conditional type over its check type. When instantiating a distributive conditional type ingetConditionalTypeInstantiation
, if the conditional's check type is a substitution type likeT & (A | B)
, the usual logic would not distribute over this type, because it's a substitution type and not a union type. So, for distribution to happen, we have to take apart theT & (A | B)
into(T & A) | (T & B)
, and distribute over that.getConditionalTypeInstantiation
is to take the intersection of the distribution result, as opposed to the union. This is because, if we narrow a type parameterT
toA | B
, and we have a conditional return typeT extends A ? R1 : T extends B ? R2 : T extends C ? R3 : never
, then we don't know which branch of the conditional return to pick, if branchT extends A ? R1
, or branchT extends B ? R2
, so we have to check whether the return expression's type is assignable to both, i.e. assignable toR1 & R2
.getConditionalTypeInstantiation
(and also at first as an optimization incheckReturnExpression
when we're deciding whether to narrow the return type). This is because, as we instantiate a type, we may produce new conditional types that we need to then decide are safe to narrow or not (see nested types case independentReturnType6.ts
test).Conditional expression checking
To support conditional expression checking in return statements, this PR changes how we check a conditional expression in a return statement. Before this PR, when checking a return statement
return cond ? exp1 : exp2
, we obtain the type of the whole expressioncond ? exp1 : exp2
, and then compare that type to the return type. With this PR, we now separately check each branch: we first obtain the type ofexp1
, and compare that type to the return type, then obtain the type ofexp2
and compare that type to the return type. This allows us to properly check a conditional expression when return type narrowing is needed.This a breaking change, and the only change that affects existing code, but this change finds bugs. Analysis of extended tests changes: #56941 (comment).
This change also slightly affects performance because we do more checks. Latest perf results here: #56941 (comment).
Performance results
This feature is opt-in. Currently, virtually no code has functions whose return types are conditional or indexed access types that satisfy the restrictions above, so no code goes down the new code path for narrowing return types. This means for existing code, there's no performance impact from the return type narrowing. The only current performance impact is from the conditional expression checking change (see a version of this PR without the change for conditional expressions: #60268 (comment)).
Assessing the performance of the new code path is tricky, as there are no baselines. The existing alternative to conditional return types is to use overloads. In one scenario I tested, checking a function written with conditional return types with this PR took ~+16% check time compared to the same function using overloads and the main branch. However, that's for checking the function declaration. In a different scenario where I included a lot of function calls though, the version with conditional return types + this PR took ~-15% compared to overloads + main branch. So I'd say the performance is acceptable, especially considering you get stronger checks when using conditional return types, and also only a small number of functions in a codebase should be written using this feature.
Unsupported things
Inference:
TS will not infer a conditional return type or an indexed access type for any function or expression. Inferring such a type is more complicated than checking, and inferring such a type could also be surprising for users. This is out of scope.
Contextually-typed anonymous functions:
This is because, if your function does not have an explicitly annotated return type, we will infer one from the returns.
This could be supported in the future.