Skip to content

Conversation

@gabritto
Copy link
Member

@gabrittogabritto commented Jan 6, 2022

Fixes#45762

Background

In 4.3, we improved control-flow narrowing of generic types (#43183). When we are about to go into control-flow narrowing, we call a function getNarrowableTypeForReference, that returns the type we are going to use in narrowing.
As of 4.3, when we call getNarrowableTypeForReference on something of type T extends C, we often get type C (the constraint) instead of the generic type T, which allows us to do narrowing when we couldn't. As an example:

typeC={kind: "a",a: string}|{kind: "b",b: string}functionfunc<TextendsC>(value: T){if(value.kind==="a"){value.a;// In 4.3: value.a: string}else{value.b;// In 4.3: value.b: string}}

Inside the if branch above, to get the type of value.a, we first need the type of value. So we call getNarrowableTypeForReference(value), and get type C (instead of the generic T, which we can't narrow). Then we can further narrow C into {kind: "a", a: string } due to the if condition. Finally, we get the type of property a of type {kind: "a", a: string }, which is type string, and that is the type of value.a.

In 4.3, the same thing happened for binding elements in binding patterns:

typeC={kind: "a",a: string}|{kind: "b",b: string}functionfunc<TextendsC>(value: T){if(value.kind==="a"){const{ a }=value;// In 4.3: a: string}else{const{ b }=value;// In 4.3: b: string}}

However, that meant other things broke, like #43941:

typeParams={foo: string;}&({tag: 'a';type: number}|{tag: 'b';type: string});constgetType=<PextendsParams>(params: P)=>{const{ foo, ...rest// <4.3: rest: Omit<P, "foo"> -- 4.3: rest:{tag: 'a'; type: number } |{tag: 'b'; type: string }}=params;returnrest;};declareconstparams: Params;switch(params.tag){case'a': {constresult=getType(params).type;// <4.3: result: number -- 4.3: result: string | numberbreak;}case'b': {constresult=getType(params).type;// <4.3: result: string -- 4.3: result: string | numberbreak;}}

In the example above, to determine the type of ...rest, we once again call getNarrowableTypeForReference(params), determine that type, and then compute whatever properties are left after we remove foo.
Before 4.3, getNarrowableTypeForReference(params) returned type P, so rest would have generic type Omit<P, "foo">. In 4.3, getNarrowableTypeForReference(params)returned typeParams, so restwould have non-generic type{tag: 'a'; type: number } |{tag: 'b'; type: string }`.

That distinction between rest having a generic versus non-generic type means we can or can't narrow other uses of rest.
The problem in the example above surfaces in the switch statement, when we try to call the function getType, that returns rest.
If we keep rest generic, we can "flow" the narrowing of the switch statement into the return type of getType.
If rest is instead the non-generic type, we can't do that narrowing, because there's no longer a connection between getType's type parameter P and the type of rest.

Change in 4.4

To fix the issue #43941 above, we reverted the getNarrowableTypeForReference(ref) behavior, making it return the type parameter instead of the type parameter's constraint when the reference ref was contextually typed by binding patterns.
So the result was like this:

typeC={kind: "a",a: string}|{kind: "b",b: string}functionfunc<TextendsC>(value: T){if(value.kind==="a"){value.a;// In 4.4: value.a: stringconst{ a }=value;// In 4.4: error, "Property 'a' does not exist on type 'C'".}else{value.b;// In 4.4: value.b: stringconst{ b }=value;// In 4.4: error, "Property 'a' does not exist on type 'C'".}}

Issue

It seems we are at odds on whether we should have getNarrowableTypeForReference return the generic type or the constraint when we have binding patterns/destructuring, because the most useful behavior can go either way depending on the code in question.

In some cases, we really expect the binding pattern access to behave like the regular property access:

if(value.kind==="a"){value.a;// stringconst{ a }=value;// error, but should be string???}

On the other hand, in this case:

constgetType=<PextendsParams>(params: P)=>{const{ foo, ...rest}=params;returnrest;};

when rest has the generic type Omit<P, "foo">, it becomes possibly more useful for callers of that function, because we can narrow the return type based on the argument type.

Proposed fix

A compromise for this issue with binding patterns is to have getNarrowableTypeForReference return the generic type if we are retrieving the type in order to use it for a rest binding element, and to have getNarrowableTypeForReference return the constraint type for non-rest binding elements. Like this:

typeC={kind: "a",a: string}|{kind: "b",b: string}functionfunc<TextendsC>(value: T){if(value.kind==="a"){const{ a }=value;// getNarrowableTypeForReference(value) = C, a: stringconst{ ...rest}=value;// getNarrowableTypeForReference(value) = T, rest: T (or Omit<T, something>)}}

Implementation

To implement this special casing for binding elements, we need a way to know *inside getNarrowableTypeForReference if we are returning a type to be used for determining the type of an object rest binding element.
This PR changes getNarrowableTypeForReference (and some other checker functions) to take in a checkMode parameter.
We also have a new CheckMode flag, CheckMode.ObjectRestBindingElement, that indicates precisely this.

Questions

  • Do we want to move forward with this fix? It's a bit weird and seems like we're stacking a special case on top of a special case, but there might not be a better way to solve this, because an argument can be made for using a generic type's constraint, and an argument can also be made for keeping the generic type in the situations above.

  • Is there a better way to implement this, without passing around CheckMode?

  • Do we want this special, generic-preserving behavior for rest binding elements in array binding patterns too?

@typescript-bottypescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 6, 2022
@typescript-bottypescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 10, 2022
@gabrittogabritto changed the title Gabritto/issue45762Fix destructuring and narrowing interactionJan 10, 2022
function getTypeForVariableLikeDeclaration(declaration: ParameterDeclaration | PropertyDeclaration | PropertySignature | VariableDeclaration | BindingElement | JSDocPropertyLikeTag, includeOptionality: boolean): Type | undefined{
function getTypeForVariableLikeDeclaration(
declaration: ParameterDeclaration | PropertyDeclaration | PropertySignature | VariableDeclaration | BindingElement | JSDocPropertyLikeTag,
includeOptionality: boolean, // >> TODO: change this to use a flag in check mode; figure out how this interacts with caching
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to move forward with this PR, I'm going to replace the use of includeOptionality parameter by a new check mode flag.

functionfunc2<TextendsZ>(value: T){
if(value.kind==="f"){
const{f: f1}=value;
const{f: { a, ...spread}}=value;
Copy link
MemberAuthor

@gabrittogabrittoJan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this case is any more weird than this whole thing already is, but... ...spread here is not going to have a generic type, since we first get the non-generic type of value, then find out the non-generic type of property f on that type, and that's the type we use to compute the type of spread.

Comment on lines +29 to +30
const{ kind, ...r1}=t;
constr2=(({ kind, ...rest})=>rest)(t);
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an inconsistency that was already present. We have:
r1 : Omit<T, "kind">, but r2 :{a: string}.

@gabritto
Copy link
MemberAuthor

@typescript-bot perf test this

@gabrittogabritto marked this pull request as ready for review January 11, 2022 23:16
@orta
Copy link
Contributor

orta commented Jan 21, 2022

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022

Heya @orta, I've started to run the perf test suite on this PR at 23c38be. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@orta
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..47337

Metricmain47337DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used355,936k (± 0.02%)355,949k (± 0.03%)+13k (+ 0.00%)355,786k356,217k
Parse Time1.97s (± 0.48%)1.97s (± 0.66%)+0.00s (+ 0.05%)1.93s1.99s
Bind Time0.86s (± 1.02%)0.86s (± 0.65%)-0.01s (- 0.58%)0.85s0.87s
Check Time5.57s (± 0.52%)5.58s (± 0.42%)+0.01s (+ 0.18%)5.53s5.64s
Emit Time5.93s (± 0.58%)5.91s (± 0.43%)-0.02s (- 0.32%)5.86s5.95s
Total Time14.32s (± 0.48%)14.31s (± 0.29%)-0.01s (- 0.10%)14.21s14.40s
Compiler-Unions - node (v10.16.3, x64)
Memory used204,215k (± 0.02%)204,203k (± 0.02%)-11k (- 0.01%)204,141k204,300k
Parse Time0.79s (± 1.11%)0.80s (± 0.84%)+0.00s (+ 0.50%)0.78s0.81s
Bind Time0.53s (± 1.32%)0.52s (± 1.00%)-0.01s (- 1.51%)0.51s0.53s
Check Time7.86s (± 0.46%)7.88s (± 0.82%)+0.01s (+ 0.18%)7.75s8.05s
Emit Time2.46s (± 0.56%)2.46s (± 0.73%)-0.00s (- 0.08%)2.43s2.52s
Total Time11.65s (± 0.37%)11.65s (± 0.71%)+0.01s (+ 0.06%)11.51s11.86s
Monaco - node (v10.16.3, x64)
Memory used342,654k (± 0.01%)342,598k (± 0.02%)-56k (- 0.02%)342,442k342,734k
Parse Time1.49s (± 0.41%)1.49s (± 0.58%)-0.00s (- 0.20%)1.48s1.52s
Bind Time0.76s (± 0.45%)0.76s (± 0.68%)+0.00s (+ 0.26%)0.75s0.77s
Check Time5.55s (± 0.74%)5.55s (± 0.68%)+0.00s (+ 0.02%)5.43s5.64s
Emit Time3.20s (± 1.01%)3.23s (± 1.28%)+0.03s (+ 0.87%)3.14s3.35s
Total Time11.00s (± 0.41%)11.04s (± 0.49%)+0.03s (+ 0.29%)10.89s11.13s
TFS - node (v10.16.3, x64)
Memory used305,702k (± 0.02%)305,684k (± 0.04%)-19k (- 0.01%)305,492k306,094k
Parse Time1.20s (± 0.77%)1.21s (± 0.37%)+0.01s (+ 0.50%)1.20s1.22s
Bind Time0.71s (± 0.96%)0.72s (± 1.03%)+0.01s (+ 1.12%)0.70s0.73s
Check Time5.04s (± 0.74%)5.07s (± 0.27%)+0.03s (+ 0.60%)5.05s5.10s
Emit Time3.38s (± 0.91%)3.40s (± 1.42%)+0.02s (+ 0.50%)3.31s3.48s
Total Time10.34s (± 0.45%)10.39s (± 0.52%)+0.06s (+ 0.57%)10.31s10.51s
material-ui - node (v10.16.3, x64)
Memory used471,728k (± 0.01%)471,756k (± 0.01%)+28k (+ 0.01%)471,696k471,859k
Parse Time1.78s (± 0.53%)1.78s (± 0.50%)+0.00s (+ 0.11%)1.77s1.80s
Bind Time0.67s (± 1.43%)0.66s (± 1.59%)-0.00s (- 0.30%)0.65s0.69s
Check Time14.25s (± 0.52%)14.29s (± 0.61%)+0.05s (+ 0.34%)14.13s14.48s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time16.70s (± 0.46%)16.74s (± 0.58%)+0.05s (+ 0.28%)16.56s16.94s
xstate - node (v10.16.3, x64)
Memory used577,149k (± 1.84%)570,076k (± 0.02%)-7,073k (- 1.23%)569,877k570,320k
Parse Time2.56s (± 0.37%)2.56s (± 0.39%)-0.00s (- 0.00%)2.54s2.58s
Bind Time1.01s (± 0.36%)1.02s (± 0.80%)+0.01s (+ 0.59%)1.00s1.04s
Check Time1.51s (± 0.63%)1.50s (± 0.44%)-0.00s (- 0.13%)1.49s1.52s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.14s (± 0.30%)5.15s (± 0.18%)+0.01s (+ 0.16%)5.12s5.16s
Angular - node (v12.1.0, x64)
Memory used333,741k (± 0.02%)333,720k (± 0.02%)-21k (- 0.01%)333,508k333,825k
Parse Time1.95s (± 0.65%)1.96s (± 0.70%)+0.00s (+ 0.26%)1.93s1.98s
Bind Time0.84s (± 0.59%)0.84s (± 0.90%)-0.00s (- 0.12%)0.83s0.86s
Check Time5.38s (± 0.41%)5.39s (± 0.42%)+0.00s (+ 0.06%)5.34s5.45s
Emit Time6.16s (± 0.56%)6.13s (± 0.34%)-0.04s (- 0.58%)6.08s6.19s
Total Time14.35s (± 0.28%)14.32s (± 0.27%)-0.03s (- 0.20%)14.22s14.39s
Compiler-Unions - node (v12.1.0, x64)
Memory used191,739k (± 0.07%)191,599k (± 0.14%)-140k (- 0.07%)190,750k191,981k
Parse Time0.78s (± 0.79%)0.78s (± 0.61%)+0.00s (+ 0.51%)0.77s0.79s
Bind Time0.53s (± 0.63%)0.53s (± 1.12%)+0.00s (+ 0.94%)0.52s0.55s
Check Time7.31s (± 0.56%)7.40s (± 0.32%)+0.09s (+ 1.19%)7.34s7.44s
Emit Time2.49s (± 0.99%)2.48s (± 0.92%)-0.02s (- 0.60%)2.42s2.51s
Total Time11.11s (± 0.44%)11.19s (± 0.30%)+0.08s (+ 0.76%)11.11s11.26s
Monaco - node (v12.1.0, x64)
Memory used325,619k (± 0.03%)325,615k (± 0.04%)-4k (- 0.00%)325,411k326,080k
Parse Time1.48s (± 0.46%)1.48s (± 1.05%)+0.01s (+ 0.61%)1.46s1.52s
Bind Time0.74s (± 0.63%)0.74s (± 0.70%)+0.00s (+ 0.14%)0.73s0.75s
Check Time5.40s (± 0.48%)5.41s (± 0.42%)+0.00s (+ 0.09%)5.35s5.46s
Emit Time3.22s (± 0.52%)3.25s (± 0.71%)+0.03s (+ 1.06%)3.20s3.29s
Total Time10.83s (± 0.36%)10.88s (± 0.39%)+0.05s (+ 0.47%)10.77s10.99s
TFS - node (v12.1.0, x64)
Memory used290,345k (± 0.02%)290,386k (± 0.01%)+41k (+ 0.01%)290,310k290,498k
Parse Time1.21s (± 0.71%)1.23s (± 0.97%)+0.02s (+ 1.32%)1.20s1.26s
Bind Time0.70s (± 0.71%)0.70s (± 0.83%)+0.01s (+ 1.01%)0.69s0.71s
Check Time4.99s (± 0.54%)4.97s (± 0.61%)-0.02s (- 0.40%)4.92s5.03s
Emit Time3.42s (± 0.68%)3.45s (± 0.88%)+0.02s (+ 0.64%)3.40s3.54s
Total Time10.32s (± 0.29%)10.34s (± 0.48%)+0.02s (+ 0.23%)10.24s10.49s
material-ui - node (v12.1.0, x64)
Memory used450,445k (± 0.01%)450,372k (± 0.06%)-73k (- 0.02%)449,238k450,585k
Parse Time1.78s (± 0.37%)1.79s (± 0.43%)+0.01s (+ 0.51%)1.77s1.81s
Bind Time0.64s (± 0.53%)0.65s (± 1.08%)+0.01s (+ 0.93%)0.64s0.67s
Check Time12.81s (± 0.81%)12.89s (± 0.79%)+0.08s (+ 0.62%)12.64s13.17s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.24s (± 0.68%)15.33s (± 0.63%)+0.09s (+ 0.62%)15.08s15.59s
xstate - node (v12.1.0, x64)
Memory used539,525k (± 1.44%)536,032k (± 0.02%)-3,493k (- 0.65%)535,897k536,324k
Parse Time2.49s (± 0.64%)2.49s (± 0.55%)0.00s ( 0.00%)2.46s2.53s
Bind Time1.05s (± 0.67%)1.05s (± 0.89%)+0.00s (+ 0.19%)1.04s1.08s
Check Time1.44s (± 0.89%)1.45s (± 0.70%)+0.00s (+ 0.21%)1.42s1.47s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.06s (± 0.42%)5.06s (± 0.30%)-0.00s (- 0.02%)5.01s5.08s
Angular - node (v14.15.1, x64)
Memory used332,065k (± 0.00%)332,092k (± 0.00%)+27k (+ 0.01%)332,052k332,132k
Parse Time1.96s (± 0.56%)1.96s (± 0.60%)-0.00s (- 0.05%)1.93s1.98s
Bind Time0.88s (± 0.88%)0.88s (± 0.85%)+0.00s (+ 0.23%)0.87s0.90s
Check Time5.43s (± 0.48%)5.42s (± 0.60%)-0.00s (- 0.06%)5.36s5.50s
Emit Time6.22s (± 0.67%)6.23s (± 0.60%)+0.01s (+ 0.16%)6.15s6.29s
Total Time14.48s (± 0.43%)14.49s (± 0.40%)+0.01s (+ 0.07%)14.36s14.61s
Compiler-Unions - node (v14.15.1, x64)
Memory used193,280k (± 0.37%)192,547k (± 0.63%)-734k (- 0.38%)189,417k193,714k
Parse Time0.81s (± 0.71%)0.81s (± 0.76%)-0.00s (- 0.12%)0.80s0.83s
Bind Time0.56s (± 0.60%)0.56s (± 0.53%)-0.00s (- 0.89%)0.55s0.56s
Check Time7.46s (± 0.66%)7.48s (± 0.94%)+0.02s (+ 0.29%)7.38s7.68s
Emit Time2.47s (± 0.34%)2.49s (± 1.15%)+0.03s (+ 1.09%)2.44s2.58s
Total Time11.29s (± 0.46%)11.34s (± 0.77%)+0.04s (+ 0.39%)11.21s11.63s
Monaco - node (v14.15.1, x64)
Memory used324,443k (± 0.00%)324,440k (± 0.01%)-3k (- 0.00%)324,396k324,473k
Parse Time1.51s (± 0.51%)1.51s (± 0.75%)+0.00s (+ 0.07%)1.49s1.54s
Bind Time0.77s (± 0.75%)0.77s (± 0.86%)-0.00s (- 0.00%)0.76s0.79s
Check Time5.36s (± 0.85%)5.38s (± 0.42%)+0.02s (+ 0.39%)5.32s5.41s
Emit Time3.28s (± 0.75%)3.30s (± 0.69%)+0.02s (+ 0.67%)3.26s3.34s
Total Time10.92s (± 0.56%)10.96s (± 0.39%)+0.05s (+ 0.42%)10.88s11.05s
TFS - node (v14.15.1, x64)
Memory used289,220k (± 0.00%)289,222k (± 0.01%)+2k (+ 0.00%)289,146k289,261k
Parse Time1.24s (± 0.90%)1.23s (± 0.60%)-0.01s (- 0.49%)1.21s1.24s
Bind Time0.75s (± 1.19%)0.74s (± 0.64%)-0.00s (- 0.40%)0.74s0.76s
Check Time4.97s (± 0.46%)4.99s (± 0.63%)+0.02s (+ 0.36%)4.93s5.05s
Emit Time3.55s (± 0.72%)3.56s (± 0.50%)+0.01s (+ 0.39%)3.51s3.60s
Total Time10.50s (± 0.49%)10.52s (± 0.42%)+0.02s (+ 0.23%)10.39s10.60s
material-ui - node (v14.15.1, x64)
Memory used448,666k (± 0.00%)448,541k (± 0.06%)-125k (- 0.03%)447,459k448,698k
Parse Time1.84s (± 0.67%)1.84s (± 0.76%)0.00s ( 0.00%)1.82s1.88s
Bind Time0.68s (± 0.54%)0.69s (± 0.84%)+0.01s (+ 1.17%)0.68s0.70s
Check Time12.88s (± 0.68%)12.90s (± 0.79%)+0.03s (+ 0.22%)12.67s13.08s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.40s (± 0.56%)15.43s (± 0.70%)+0.03s (+ 0.21%)15.18s15.62s
xstate - node (v14.15.1, x64)
Memory used533,709k (± 0.00%)533,719k (± 0.01%)+10k (+ 0.00%)533,669k533,793k
Parse Time2.55s (± 0.40%)2.56s (± 0.57%)+0.01s (+ 0.39%)2.52s2.59s
Bind Time1.17s (± 0.91%)1.17s (± 0.81%)+0.01s (+ 0.86%)1.16s1.20s
Check Time1.48s (± 0.57%)1.49s (± 0.59%)+0.01s (+ 0.61%)1.47s1.51s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.27s (± 0.25%)5.30s (± 0.37%)+0.03s (+ 0.57%)5.26s5.33s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory7 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
BenchmarkNameIterations
Current4733710
Baselinemain10

Developer Information:

Download Benchmark

@gabritto
Copy link
MemberAuthor

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022

Heya @gabritto, I've started to run the tarball bundle task on this PR at 23c38be. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022

Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{"devDependencies":{"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/118069/artifacts?artifactName=tgz&fileId=5E7679E65EC7843039E23306BDAFB943423F455FE7823C179CC050430C6167E202&fileName=/typescript-4.6.0-insiders.20220121.tgz" } } 

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

RestBindingElement = 1 << 5, // Checking a type that is going to be used to determine the type of a rest binding element
// e.g. in `const{a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`,
// we need to preserve generic types instead of substituting them for constraints
IncludeOptionality = 1 << 6, // >> TODO: description, replace bool param with flag
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this flag instead of the includeOptionality parameter in another commit, once we settle on the other changes of this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely get this change in the PR as well.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since replacing the includeOptionality by a CheckMode flag affects our caching behavior in checkExpressionCached (or we'll have to add an exception for that flag only), I'm gonna merge this without that part for the 4.6 RC, and I can add this change in another PR if we really want this.

@RyanCavanaugh
Copy link
Member

@ahejlsberg any concerns? 4.6 RC is fast approaching and we'd like to have this in

Copy link
Member

@ahejlsbergahejlsberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, but would like to see a few changes first.

RestBindingElement = 1 << 5, // Checking a type that is going to be used to determine the type of a rest binding element
// e.g. in `const{a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`,
// we need to preserve generic types instead of substituting them for constraints
IncludeOptionality = 1 << 6, // >> TODO: description, replace bool param with flag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely get this change in the PR as well.

}
const symbol = getSymbolOfNode(node);
return symbol && getSymbolLinks(symbol).type || getTypeForVariableLikeDeclaration(node, /*includeOptionality*/ false);
return symbol && getSymbolLinks(symbol).type || getTypeForVariableLikeDeclaration(node, /*includeOptionality*/ false, checkMode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should definitely fold the includeOptionality into CheckMode.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4.4 regression: in operator does not narrow types in generic constraints when destructuring

6 participants

@gabritto@orta@typescript-bot@RyanCavanaugh@ahejlsberg