Skip to content

Control flow analysis for element access with variable index - #57847

Merged
Anders Hejlsberg (ahejlsberg) merged 6 commits into
mainfrom
fix56389
Mar 26, 2024
Merged

Control flow analysis for element access with variable index#57847
Anders Hejlsberg (ahejlsberg) merged 6 commits into
mainfrom
fix56389

Conversation

@ahejlsberg

@ahejlsbergAnders Hejlsberg (ahejlsberg) commented Mar 19, 2024

Copy link
Copy Markdown
Member

With this PR we perform control flow analysis for element access expressions obj[key] where key is a const variable, or a let variable or parameter that is never targeted in an assignment. The intuition here is that even if the exact name of the property selected by key isn't known, any control flow analysis proof that involves obj[key] should hold in subsequent statements as long as obj and key aren't mutated,

Some examples:

functionf1(obj: Record<string,unknown>,key: string){if(typeofobj[key]==="string"){obj[key].toUpperCase();// Now ok, previously was error}}typeThing={a?: string,b?: number,c?: number};functionf2(obj: Thing,getKey: ()=>keyofThing){constkey=getKey();if(obj[key]!==undefined){if(typeofobj[key]==="string"){obj[key].toUpperCase();// Now ok, previously was error}if(typeofobj[key]==="number"){obj[key].toFixed();// Now ok, previously was error}}}functionf3<Kextendsstring>(obj: Record<K,string|undefined>,key: K){if(obj[key]){obj[key].toUpperCase();// Now ok, previously was error}}

Fixes#56389.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
test top200✅ Started👀 Results
user test this✅ Started👀 Results
run dt✅ Started✅ Results
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing main and refs/pull/57847/merge:

Something interesting changed - please have a look.

Details

webpack

tsconfig.types.json

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,747k (± 0.00%)295,762k (± 0.01%)~295,722k295,797kp=0.198 n=6
Parse Time2.66s (± 0.28%)2.67s (± 0.31%)~2.65s2.67sp=0.432 n=6
Bind Time0.83s (± 0.62%)0.83s (± 0.62%)~0.82s0.83sp=1.000 n=6
Check Time8.21s (± 0.28%)8.19s (± 0.16%)~8.17s8.21sp=0.089 n=6
Emit Time7.14s (± 0.26%)7.13s (± 0.33%)~7.11s7.17sp=0.518 n=6
Total Time18.84s (± 0.15%)18.81s (± 0.09%)~18.79s18.83sp=0.164 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used192,638k (± 0.77%)193,221k (± 0.91%)~191,920k195,508kp=0.378 n=6
Parse Time1.36s (± 1.20%)1.36s (± 0.40%)~1.35s1.36sp=0.360 n=6
Bind Time0.72s (± 0.57%)0.72s (± 0.00%)~0.72s0.72sp=0.405 n=6
Check Time9.52s (± 0.74%)9.48s (± 0.54%)~9.42s9.54sp=0.260 n=6
Emit Time2.65s (± 0.41%)2.66s (± 0.19%)+0.01s (+ 0.50%)2.66s2.67sp=0.020 n=6
Total Time14.26s (± 0.52%)14.22s (± 0.35%)~14.16s14.28sp=0.469 n=6
Monaco - node (v18.15.0, x64)
Memory used347,377k (± 0.01%)347,411k (± 0.00%)~347,381k347,434kp=0.065 n=6
Parse Time2.47s (± 0.51%)2.48s (± 0.54%)~2.46s2.50sp=0.139 n=6
Bind Time0.93s (± 0.44%)0.93s (± 0.56%)~0.92s0.93sp=0.595 n=6
Check Time7.02s (± 0.40%)7.02s (± 0.50%)~6.98s7.08sp=1.000 n=6
Emit Time4.06s (± 0.67%)4.08s (± 0.25%)~4.06s4.09sp=0.415 n=6
Total Time14.47s (± 0.21%)14.50s (± 0.27%)~14.47s14.57sp=0.334 n=6
TFS - node (v18.15.0, x64)
Memory used302,758k (± 0.01%)302,755k (± 0.01%)~302,734k302,782kp=0.936 n=6
Parse Time2.01s (± 0.88%)2.00s (± 0.68%)~1.98s2.02sp=0.805 n=6
Bind Time1.01s (± 0.81%)1.00s (± 0.82%)~0.99s1.01sp=0.077 n=6
Check Time6.34s (± 0.36%)6.32s (± 0.38%)~6.29s6.36sp=0.373 n=6
Emit Time3.61s (± 0.21%)3.60s (± 0.91%)~3.56s3.65sp=0.566 n=6
Total Time12.96s (± 0.17%)12.93s (± 0.38%)~12.86s13.01sp=0.225 n=6
material-ui - node (v18.15.0, x64)
Memory used511,373k (± 0.00%)511,377k (± 0.01%)~511,349k511,407kp=1.000 n=6
Parse Time2.66s (± 0.46%)2.67s (± 0.28%)~2.66s2.68sp=0.062 n=6
Bind Time0.98s (± 1.19%)0.98s (± 1.35%)~0.96s1.00sp=0.737 n=6
Check Time17.34s (± 0.68%)17.31s (± 0.44%)~17.21s17.41sp=0.936 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.97s (± 0.53%)20.96s (± 0.33%)~20.88s21.04sp=1.000 n=6
mui-docs - node (v18.15.0, x64)
Memory used1,719,135k (± 0.00%)1,719,188k (± 0.00%)+54k (+ 0.00%)1,719,155k1,719,222kp=0.020 n=6
Parse Time6.54s (± 0.21%)6.53s (± 0.51%)~6.50s6.59sp=0.373 n=6
Bind Time2.35s (± 0.23%)2.36s (± 0.69%)~2.35s2.39sp=0.342 n=6
Check Time56.29s (± 0.65%)56.21s (± 0.28%)~56.01s56.41sp=0.575 n=6
Emit Time0.13s (± 0.00%)0.13s (± 0.00%)~0.13s0.13sp=1.000 n=6
Total Time65.31s (± 0.56%)65.23s (± 0.25%)~65.07s65.49sp=0.689 n=6
self-build-src - node (v18.15.0, x64)
Memory used2,395,587k (± 0.03%)2,395,905k (± 0.03%)~2,395,338k2,397,348kp=0.298 n=6
Parse Time4.99s (± 0.96%)5.01s (± 1.19%)~4.96s5.12sp=0.936 n=6
Bind Time1.90s (± 0.87%)1.90s (± 1.20%)~1.88s1.93sp=0.933 n=6
Check Time33.59s (± 0.22%)33.59s (± 0.31%)~33.41s33.67sp=1.000 n=6
Emit Time2.71s (± 1.81%)2.70s (± 1.22%)~2.67s2.75sp=0.422 n=6
Total Time43.21s (± 0.19%)43.23s (± 0.29%)~43.08s43.39sp=0.810 n=6
self-compiler - node (v18.15.0, x64)
Memory used416,099k (± 0.00%)416,158k (± 0.01%)+59k (+ 0.01%)416,114k416,200kp=0.005 n=6
Parse Time2.82s (± 1.20%)2.81s (± 0.94%)~2.76s2.83sp=0.747 n=6
Bind Time1.07s (± 0.70%)1.07s (± 0.92%)~1.06s1.08sp=1.000 n=6
Check Time15.28s (± 0.23%)15.37s (± 0.22%)+0.08s (+ 0.55%)15.32s15.41sp=0.008 n=6
Emit Time1.16s (± 1.01%)1.14s (± 0.66%)-0.02s (- 1.44%)1.13s1.15sp=0.032 n=6
Total Time20.34s (± 0.08%)20.38s (± 0.19%)+0.05s (+ 0.23%)20.34s20.44sp=0.029 n=6
vscode - node (v18.15.0, x64)
Memory used2,883,825k (± 0.00%)2,884,003k (± 0.00%)+178k (+ 0.01%)2,883,812k2,884,096kp=0.031 n=6
Parse Time10.82s (± 0.35%)10.83s (± 0.57%)~10.76s10.91sp=1.000 n=6
Bind Time3.46s (± 0.68%)3.46s (± 0.30%)~3.45s3.48sp=0.494 n=6
Check Time61.55s (± 0.41%)61.79s (± 0.63%)~61.47s62.54sp=0.298 n=6
Emit Time17.06s (± 7.90%)16.99s (± 8.36%)~16.30s19.89sp=0.298 n=6
Total Time92.88s (± 1.66%)93.07s (± 1.97%)~92.07s96.80sp=0.575 n=6
webpack - node (v18.15.0, x64)
Memory used408,036k (± 0.01%)408,075k (± 0.01%)~408,013k408,146kp=0.378 n=6
Parse Time3.23s (± 0.32%)3.23s (± 0.25%)~3.22s3.24sp=0.270 n=6
Bind Time1.38s (± 0.30%)1.37s (± 0.75%)~1.36s1.38sp=0.461 n=6
Check Time14.27s (± 0.41%)14.31s (± 0.30%)~14.27s14.37sp=0.415 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time18.88s (± 0.34%)18.92s (± 0.28%)~18.86s18.99sp=0.521 n=6
xstate - node (v18.15.0, x64)
Memory used513,118k (± 0.02%)513,166k (± 0.02%)~513,081k513,316kp=0.689 n=6
Parse Time3.28s (± 0.32%)3.28s (± 0.31%)~3.26s3.29sp=0.933 n=6
Bind Time1.54s (± 0.34%)1.54s (± 0.41%)~1.53s1.55sp=0.386 n=6
Check Time2.86s (± 0.68%)2.85s (± 0.90%)~2.83s2.90sp=0.517 n=6
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)~0.07s0.07sp=1.000 n=6
Total Time7.75s (± 0.19%)7.75s (± 0.29%)~7.73s7.79sp=0.420 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing main and refs/pull/57847/merge:

Something interesting changed - please have a look.

Details

microsoft/vscode

4 of 54 projects failed to build with the old tsc and were ignored

src/tsconfig.tsec.json

src/tsconfig.json

build/tsconfig.build.json

react-navigation/react-navigation

9 of 14 projects failed to build with the old tsc and were ignored

tsconfig.json

packages/core/tsconfig.build.json

Redocly/redoc

tsconfig.lib.json

tsconfig.json

ueberdosis/tiptap

3 of 8 projects failed to build with the old tsc and were ignored

tests/cypress/tsconfig.json

demos/tsconfig.vue-3.json

demos/tsconfig.vue-2.json

demos/tsconfig.react.json

demos/tsconfig.base.json

vercel/hyper

2 of 3 projects failed to build with the old tsc and were ignored

tsconfig.json

xyflow/xyflow

5 of 6 projects failed to build with the old tsc and were ignored

packages/system/tsconfig.json

  • error TS2339: Property 'handleBounds' does not exist on type '{ z?: number | undefined; handleBounds?: NodeHandleBounds | undefined; isParent?: boolean | undefined; userProvidedNode: NodeBase<Record<string, unknown>, string>; } | undefined'.

@jakebailey

Copy link
Copy Markdown
Member

Weird; reading some of the breaks, there seems to be a bug in this iteration of the PR where unrelated accesses are getting narrowed too.

declareconstkey: string;declareconstobj1: Record<string,number|undefined>;declareconstobj2: Record<string,number|undefined>;if(obj1[key]){constx=obj1[key];// ^?consty=obj2[key];// ^?}

image

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

Argh, I see what the issue is. Will fix.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
test top200✅ Started👀 Results
user test this✅ Started✅ Results
run dt✅ Started✅ Results
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing main and refs/pull/57847/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,750k (± 0.01%)295,777k (± 0.00%)~295,765k295,788kp=0.066 n=6
Parse Time2.66s (± 0.37%)2.66s (± 0.50%)~2.64s2.68sp=0.654 n=6
Bind Time0.83s (± 0.62%)0.83s (± 0.00%)~0.83s0.83sp=0.174 n=6
Check Time8.20s (± 0.26%)8.19s (± 0.15%)~8.17s8.20sp=0.220 n=6
Emit Time7.14s (± 0.23%)7.11s (± 0.23%)-0.03s (- 0.40%)7.09s7.13sp=0.028 n=6
Total Time18.83s (± 0.21%)18.79s (± 0.14%)~18.76s18.82sp=0.075 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used193,151k (± 0.97%)192,675k (± 0.75%)~191,914k195,612kp=0.936 n=6
Parse Time1.36s (± 0.40%)1.36s (± 1.02%)~1.33s1.37sp=0.663 n=6
Bind Time0.72s (± 0.00%)0.72s (± 0.00%)~0.72s0.72sp=1.000 n=6
Check Time9.49s (± 0.56%)9.48s (± 0.58%)~9.41s9.58sp=1.000 n=6
Emit Time2.66s (± 0.66%)2.65s (± 0.34%)~2.64s2.66sp=0.198 n=6
Total Time14.22s (± 0.32%)14.21s (± 0.35%)~14.15s14.30sp=0.936 n=6
Monaco - node (v18.15.0, x64)
Memory used347,386k (± 0.01%)347,403k (± 0.01%)~347,380k347,435kp=0.173 n=6
Parse Time2.47s (± 0.98%)2.49s (± 0.00%)~2.49s2.49sp=0.128 n=6
Bind Time0.94s (± 2.40%)0.93s (± 0.44%)~0.92s0.93sp=0.787 n=6
Check Time7.02s (± 0.32%)7.00s (± 0.33%)~6.98s7.04sp=0.106 n=6
Emit Time4.07s (± 0.41%)4.07s (± 0.30%)~4.06s4.09sp=0.616 n=6
Total Time14.50s (± 0.13%)14.49s (± 0.26%)~14.45s14.55sp=0.332 n=6
TFS - node (v18.15.0, x64)
Memory used302,745k (± 0.01%)302,751k (± 0.01%)~302,722k302,771kp=0.378 n=6
Parse Time2.00s (± 0.81%)2.01s (± 0.75%)~2.00s2.04sp=0.462 n=6
Bind Time1.00s (± 0.98%)1.00s (± 1.03%)~0.99s1.02sp=0.452 n=6
Check Time6.33s (± 0.60%)6.30s (± 0.21%)~6.28s6.32sp=0.221 n=6
Emit Time3.60s (± 0.55%)3.60s (± 0.23%)~3.59s3.61sp=1.000 n=6
Total Time12.93s (± 0.30%)12.92s (± 0.22%)~12.89s12.97sp=0.685 n=6
material-ui - node (v18.15.0, x64)
Memory used511,362k (± 0.00%)511,389k (± 0.00%)+27k (+ 0.01%)511,377k511,417kp=0.030 n=6
Parse Time2.66s (± 0.58%)2.66s (± 0.31%)~2.64s2.66sp=0.242 n=6
Bind Time0.99s (± 1.23%)0.99s (± 1.05%)~0.97s1.00sp=1.000 n=6
Check Time17.33s (± 0.33%)17.29s (± 0.25%)~17.23s17.36sp=0.226 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.98s (± 0.26%)20.93s (± 0.23%)~20.86s21.01sp=0.125 n=6
mui-docs - node (v18.15.0, x64)
Memory used1,719,162k (± 0.00%)1,719,198k (± 0.00%)~1,719,144k1,719,243kp=0.066 n=6
Parse Time6.52s (± 0.49%)6.53s (± 0.43%)~6.50s6.57sp=0.747 n=6
Bind Time2.37s (± 0.56%)2.35s (± 0.35%)~2.35s2.37sp=0.052 n=6
Check Time56.10s (± 0.42%)56.40s (± 0.42%)+0.30s (+ 0.54%)56.12s56.75sp=0.045 n=6
Emit Time0.14s (± 4.05%)0.13s (± 0.00%)~0.13s0.13sp=0.071 n=6
Total Time65.12s (± 0.36%)65.41s (± 0.39%)~65.10s65.79sp=0.093 n=6
self-build-src - node (v18.15.0, x64)
Memory used2,395,113k (± 0.03%)2,395,863k (± 0.02%)~2,395,219k2,396,651kp=0.128 n=6
Parse Time4.98s (± 0.73%)4.98s (± 1.12%)~4.94s5.08sp=0.809 n=6
Bind Time1.89s (± 0.72%)1.91s (± 0.63%)+0.02s (+ 1.06%)1.90s1.93sp=0.042 n=6
Check Time33.64s (± 0.55%)33.64s (± 0.29%)~33.49s33.79sp=0.689 n=6
Emit Time2.70s (± 2.05%)2.71s (± 1.18%)~2.68s2.77sp=1.000 n=6
Total Time43.22s (± 0.55%)43.26s (± 0.21%)~43.13s43.36sp=0.378 n=6
self-compiler - node (v18.15.0, x64)
Memory used416,056k (± 0.00%)416,145k (± 0.01%)+89k (+ 0.02%)416,120k416,178kp=0.005 n=6
Parse Time2.82s (± 0.66%)2.81s (± 0.52%)~2.79s2.83sp=0.368 n=6
Bind Time1.07s (± 0.51%)1.07s (± 0.51%)~1.06s1.07sp=1.000 n=6
Check Time15.27s (± 0.39%)15.37s (± 0.45%)+0.10s (+ 0.64%)15.30s15.48sp=0.029 n=6
Emit Time1.14s (± 1.51%)1.15s (± 0.90%)~1.13s1.16sp=0.459 n=6
Total Time20.30s (± 0.38%)20.39s (± 0.37%)~20.30s20.52sp=0.109 n=6
vscode - node (v18.15.0, x64)
Memory used2,884,099k (± 0.00%)2,884,245k (± 0.00%)+147k (+ 0.01%)2,884,114k2,884,293kp=0.013 n=6
Parse Time10.81s (± 0.27%)10.81s (± 0.23%)~10.77s10.84sp=0.871 n=6
Bind Time3.46s (± 0.47%)3.46s (± 0.24%)~3.45s3.47sp=0.458 n=6
Check Time61.53s (± 0.21%)61.65s (± 0.39%)~61.37s62.08sp=0.297 n=6
Emit Time16.44s (± 0.24%)17.12s (± 8.96%)~16.41s20.25sp=0.126 n=6
Total Time92.24s (± 0.19%)93.05s (± 1.87%)~92.13s96.60sp=0.149 n=6
webpack - node (v18.15.0, x64)
Memory used408,093k (± 0.03%)408,112k (± 0.02%)~408,031k408,214kp=0.689 n=6
Parse Time3.22s (± 0.46%)3.24s (± 0.50%)~3.21s3.25sp=0.139 n=6
Bind Time1.38s (± 0.71%)1.38s (± 0.99%)~1.36s1.40sp=0.607 n=6
Check Time14.31s (± 0.50%)14.31s (± 0.30%)~14.23s14.34sp=1.000 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time18.91s (± 0.40%)18.93s (± 0.19%)~18.88s18.97sp=0.688 n=6
xstate - node (v18.15.0, x64)
Memory used513,109k (± 0.01%)513,091k (± 0.01%)~513,035k513,147kp=0.810 n=6
Parse Time3.28s (± 0.23%)3.27s (± 0.23%)~3.26s3.28sp=0.195 n=6
Bind Time1.54s (± 0.41%)1.54s (± 0.49%)~1.53s1.55sp=0.718 n=6
Check Time2.86s (± 0.64%)2.84s (± 1.02%)~2.79s2.87sp=0.145 n=6
Emit Time0.07s (± 0.00%)0.07s (± 5.69%)~0.07s0.08sp=0.405 n=6
Total Time7.76s (± 0.27%)7.73s (± 0.34%)~7.69s7.76sp=0.198 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) perf test this faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing main and refs/pull/57847/merge:

Something interesting changed - please have a look.

Details

xyflow/xyflow

5 of 6 projects failed to build with the old tsc and were ignored

packages/system/tsconfig.json

  • error TS2339: Property 'handleBounds' does not exist on type '{ z?: number | undefined; handleBounds?: NodeHandleBounds | undefined; isParent?: boolean | undefined; userProvidedNode: NodeBase<Record<string, unknown>, string>; } | undefined'.

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
pack this✅ Started✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), 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/160607/artifacts?artifactName=tgz&fileId=FCCE8FB0B7250696E9C38855366CC2296FF63CC26FF4295ECFB451B5CF87FE3B02&fileName=/typescript-5.5.0-insiders.20240319.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/pr-build@5.5.0-pr-57847-18".;

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

Tests and performance look good. This is ready for a review.

}
if (isElementAccessExpression(source) && isElementAccessExpression(target) && isIdentifier(source.argumentExpression) && isIdentifier(target.argumentExpression)) {
const symbol = getResolvedSymbol(source.argumentExpression);
if (symbol === getResolvedSymbol(target.argumentExpression) && (isConstantVariable(symbol) || isParameterOrMutableLocalVariable(symbol) && !isSymbolAssigned(symbol))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it make more sense to do isPastLastAssignment using a more specific location? How does this PR interact with #56908?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not that simple. We would need to know that all narrowing operations for obj[key] occur past the last assignment to key which would require an additional CFA graph walk. We don't have that issue in #56908 because the only location we care about is the location where the arrow function is created.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense; I really need to prototype the idea I had to make that efficient enough for us to do, but I've lost that brainwave ☹️

Can you add a test in the vein of #56908 in terms of what closures do? Something like: Playground Link (which does behave properly I think)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Can you add a test in the vein of...

Such a test wouldn't show anything. The new analysis in #56908 only affects references to local variables, not references to properties, so there are no changes across closure boundaries with this PR.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) perf test this faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,752k (± 0.00%)295,764k (± 0.01%)~295,727k295,787kp=0.199 n=6
Parse Time2.67s (± 0.28%)2.66s (± 0.39%)~2.65s2.68sp=0.351 n=6
Bind Time0.83s (± 0.76%)0.83s (± 0.76%)~0.82s0.84sp=1.000 n=6
Check Time8.20s (± 0.28%)8.21s (± 0.29%)~8.17s8.24sp=0.459 n=6
Emit Time7.11s (± 0.19%)7.11s (± 0.43%)~7.07s7.15sp=0.745 n=6
Total Time18.81s (± 0.21%)18.82s (± 0.21%)~18.78s18.88sp=0.686 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used192,684k (± 0.79%)193,228k (± 0.93%)~191,955k195,545kp=0.689 n=6
Parse Time1.36s (± 1.29%)1.36s (± 1.10%)~1.34s1.38sp=0.932 n=6
Bind Time0.72s (± 0.57%)0.72s (± 0.00%)~0.72s0.72sp=0.405 n=6
Check Time9.54s (± 0.59%)9.53s (± 0.69%)~9.45s9.60sp=1.000 n=6
Emit Time2.66s (± 0.39%)2.66s (± 0.19%)~2.65s2.66sp=0.794 n=6
Total Time14.28s (± 0.44%)14.27s (± 0.43%)~14.19s14.33sp=0.935 n=6
Monaco - node (v18.15.0, x64)
Memory used347,381k (± 0.01%)347,410k (± 0.00%)+29k (+ 0.01%)347,396k347,428kp=0.020 n=6
Parse Time2.48s (± 0.59%)2.48s (± 0.49%)~2.46s2.49sp=0.867 n=6
Bind Time0.93s (± 0.44%)0.92s (± 0.59%)~0.92s0.93sp=0.282 n=6
Check Time7.02s (± 0.43%)7.03s (± 0.11%)~7.02s7.04sp=0.368 n=6
Emit Time4.07s (± 0.63%)4.07s (± 0.55%)~4.05s4.11sp=0.800 n=6
Total Time14.50s (± 0.42%)14.50s (± 0.18%)~14.48s14.55sp=1.000 n=6
TFS - node (v18.15.0, x64)
Memory used302,745k (± 0.01%)302,759k (± 0.01%)~302,715k302,814kp=0.575 n=6
Parse Time2.01s (± 0.70%)2.00s (± 1.41%)~1.97s2.05sp=0.293 n=6
Bind Time1.01s (± 1.02%)1.00s (± 0.52%)~0.99s1.00sp=0.069 n=6
Check Time6.33s (± 0.45%)6.33s (± 0.60%)~6.28s6.39sp=0.630 n=6
Emit Time3.61s (± 0.29%)3.60s (± 0.46%)~3.58s3.62sp=0.559 n=6
Total Time12.96s (± 0.26%)12.93s (± 0.41%)~12.87s13.01sp=0.261 n=6
material-ui - node (v18.15.0, x64)
Memory used509,905k (± 0.00%)509,929k (± 0.00%)+24k (+ 0.00%)509,910k509,954kp=0.020 n=6
Parse Time2.66s (± 0.73%)2.66s (± 0.44%)~2.64s2.67sp=0.869 n=6
Bind Time0.99s (± 1.18%)0.98s (± 1.23%)~0.97s1.00sp=0.323 n=6
Check Time17.23s (± 0.24%)17.27s (± 0.33%)~17.19s17.33sp=0.296 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.89s (± 0.23%)20.91s (± 0.28%)~20.84s20.98sp=0.372 n=6
mui-docs - node (v18.15.0, x64)
Memory used1,719,252k (± 0.00%)1,719,223k (± 0.00%)~1,719,141k1,719,297kp=0.575 n=6
Parse Time6.54s (± 0.43%)6.55s (± 0.74%)~6.50s6.64sp=0.872 n=6
Bind Time2.36s (± 0.38%)2.38s (± 0.52%)+0.02s (+ 0.64%)2.37s2.40sp=0.025 n=6
Check Time56.41s (± 0.23%)56.32s (± 0.47%)~55.88s56.58sp=0.810 n=6
Emit Time0.13s (± 0.00%)0.13s (± 3.87%)~0.13s0.14sp=0.174 n=6
Total Time65.45s (± 0.21%)65.38s (± 0.34%)~65.03s65.58sp=0.630 n=6
self-build-src - node (v18.15.0, x64)
Memory used2,395,320k (± 0.04%)2,394,952k (± 0.04%)~2,393,869k2,395,740kp=0.575 n=6
Parse Time4.99s (± 0.58%)5.04s (± 1.02%)~4.96s5.10sp=0.077 n=6
Bind Time1.91s (± 0.88%)1.90s (± 0.98%)~1.88s1.93sp=0.516 n=6
Check Time33.69s (± 0.48%)33.71s (± 0.24%)~33.60s33.80sp=0.689 n=6
Emit Time2.72s (± 1.49%)2.69s (± 1.32%)~2.65s2.74sp=0.375 n=6
Total Time43.33s (± 0.40%)43.35s (± 0.17%)~43.26s43.44sp=0.810 n=6
self-compiler - node (v18.15.0, x64)
Memory used416,071k (± 0.01%)416,145k (± 0.01%)+75k (+ 0.02%)416,120k416,177kp=0.005 n=6
Parse Time2.82s (± 0.65%)2.84s (± 0.66%)~2.82s2.87sp=0.122 n=6
Bind Time1.07s (± 0.59%)1.07s (± 0.51%)~1.06s1.07sp=0.201 n=6
Check Time15.28s (± 0.22%)15.33s (± 0.18%)+0.04s (+ 0.27%)15.30s15.36sp=0.043 n=6
Emit Time1.14s (± 1.43%)1.14s (± 1.11%)~1.13s1.16sp=0.567 n=6
Total Time20.32s (± 0.16%)20.36s (± 0.18%)+0.05s (+ 0.23%)20.32s20.42sp=0.034 n=6
vscode - node (v18.15.0, x64)
Memory used2,886,074k (± 0.00%)2,886,262k (± 0.00%)+189k (+ 0.01%)2,886,173k2,886,342kp=0.005 n=6
Parse Time10.86s (± 0.46%)10.84s (± 0.37%)~10.80s10.91sp=0.808 n=6
Bind Time3.47s (± 0.42%)3.46s (± 0.22%)~3.45s3.47sp=0.102 n=6
Check Time61.33s (± 0.32%)61.46s (± 0.32%)~61.25s61.67sp=0.378 n=6
Emit Time16.50s (± 0.79%)16.42s (± 0.26%)~16.35s16.47sp=0.148 n=6
Total Time92.16s (± 0.33%)92.19s (± 0.24%)~91.91s92.45sp=0.936 n=6
webpack - node (v18.15.0, x64)
Memory used408,070k (± 0.02%)408,120k (± 0.02%)~408,023k408,217kp=0.378 n=6
Parse Time3.23s (± 0.51%)3.23s (± 0.46%)~3.21s3.24sp=0.684 n=6
Bind Time1.38s (± 0.00%)1.38s (± 0.75%)~1.36s1.39sp=0.599 n=6
Check Time14.29s (± 0.17%)14.29s (± 0.22%)~14.23s14.32sp=0.935 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time18.90s (± 0.18%)18.90s (± 0.17%)~18.86s18.94sp=1.000 n=6
xstate - node (v18.15.0, x64)
Memory used513,109k (± 0.01%)513,102k (± 0.01%)~512,985k513,143kp=0.810 n=6
Parse Time3.27s (± 0.27%)3.27s (± 0.41%)~3.25s3.29sp=0.801 n=6
Bind Time1.54s (± 0.54%)1.54s (± 0.26%)~1.54s1.55sp=0.527 n=6
Check Time2.85s (± 0.63%)2.85s (± 0.65%)~2.83s2.88sp=0.628 n=6
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)~0.07s0.07sp=1.000 n=6
Total Time7.75s (± 0.21%)7.74s (± 0.18%)~7.73s7.76sp=1.000 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@ipanasenko

Copy link
Copy Markdown

Hi. I'm not sure if it's okay to ask here or I should rather open new discussion, so sorry in advance.

Do you think the following type narrowing using in should work as well?
I have a Record with predefined keys in it, and I expected that if in returns true, then animal can only be of Animal type.
https://www.typescriptlang.org/play/?#code/C4TwDgpgBAggdgSwLYEMA2UC8UDkBjFYHKAH1wBMB7Ac2LJwCMEAncnAbgCg9K4BnYFBSJUaAAooA7nwCyKMAC4oAJQg9WAHnjJ0AGihwArkgYRmAPixQA3pyhQCwBQBZddqFWou39pqyUATG4AvlzcvAJQABaUknJwIBLSVgAUwjpoSgLMCHDUAJRKRiZmWJa29ggAZlBpIuhQuUL14lKy8vk27vbMEMCGzHDNGUntYADa6aIAulz2wZzuvf2DUACScFW5CKBcwUA

I think this might be related to this feature.

@JoostK

Copy link
Copy Markdown
Contributor

Hi. I'm not sure if it's okay to ask here or I should rather open new discussion, so sorry in advance.

Do you think the following type narrowing using in should work as well? I have a Record with predefined keys in it, and I expected that if in returns true, then animal can only be of Animal type. https://www.typescriptlang.org/play/?#code/C4TwDgpgBAggdgSwLYEMA2UC8UDkBjFYHKAH1wBMB7Ac2LJwCMEAncnAbgCg9K4BnYFBSJUaAAooA7nwCyKMAC4oAJQg9WAHnjJ0AGihwArkgYRmAPixQA3pyhQCwBQBZddqFWou39pqyUATG4AvlzcvAJQABaUknJwIBLSVgAUwjpoSgLMCHDUAJRKRiZmWJa29ggAZlBpIuhQuUL14lKy8vk27vbMEMCGzHDNGUntYADa6aIAulz2wZzuvf2DUACScFW5CKBcwUA

I think this might be related to this feature.

That error is correct and desirable: the index signature only specifies that properties in Animal (i.e. 'cat' | 'dog' | 'bird') have a number value, it does not specify a type for any other properties. Since TypeScript does not have strict types it's fine for a Record<Animal, number> to also have other properties, as demonstrated in this playground. If the animal parameter is changed from type string to Animal then TypeScript no longer reports an error for the implementation of howManyPaws, while correctly preventing the monkey from requesting its number of paws.

@ipanasenko

Copy link
Copy Markdown

I see. Big thank you for an explanation

@geakstr

Copy link
Copy Markdown

Hi and thank you for your work! Trying to clarify for myself this statement "The intuition here is that even if the exact name of the property selected by key isn't known, any control flow analysis proof that involves obj[key] should hold in subsequent statements as long as obj and key aren't mutated" from the PR description.

Here I mutate obj and TS 5.5.2 doesn't throw errors:

function f1(obj: Record<string, unknown>, key: string) {
if (typeof obj[key] === "string") {
delete obj[key];
obj[key].toUpperCase(); // ok
}
}

Is this "expected"?

@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Narrow object property when key is a variable

7 participants

@ahejlsberg@typescript-bot@jakebailey@ipanasenko@JoostK@geakstr@gabritto