Skip to content
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

Control flow analysis for element access with variable constant-like index in for statements #60715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Dec 9, 2024

This is a small extension of #57847
closes #58803

let variables in for loops are special, despite them being mutable and "shared" for the loop - each iteration gets its own unique copy of that variable. A mutation in the incrementor has no effect on the loop's body so, to the best of my understanding, it can safely be ignored.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 9, 2024
Comment on lines +29635 to +29645
function isSymbolAssignedInForStatementBody(symbol: Symbol) {
const forStatement = getRootDeclaration(symbol.valueDeclaration!).parent.parent;
Debug.assert(isForStatement(forStatement));
return !isPastLastAssignment(symbol, forStatement.statement);
}

function isUsedInForStatementBody(symbol: Symbol, location: Node) {
const forStatement = getRootDeclaration(symbol.valueDeclaration!).parent.parent;
Debug.assert(isForStatement(forStatement));
return location.pos >= forStatement.statement.pos && location.end <= forStatement.statement.end;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to combine those 2 but I've failed so far to figure out an elegant function name for a function that would do both 😅

@jakebailey
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 9, 2024

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

Command Status Results
test top400 ✅ Started 👀 Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/60715/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

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

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 34 34 ~ ~ ~ p=1.000 n=6
Symbols 62,363 62,363 ~ ~ ~ p=1.000 n=6
Types 50,395 50,395 ~ ~ ~ p=1.000 n=6
Memory used 194,235k (± 0.97%) 193,705k (± 0.76%) ~ 193,012k 196,693k p=0.936 n=6
Parse Time 1.31s (± 0.84%) 1.31s (± 0.75%) ~ 1.29s 1.32s p=0.718 n=6
Bind Time 0.72s 0.72s ~ ~ ~ p=1.000 n=6
Check Time 9.77s (± 0.48%) 9.76s (± 0.33%) ~ 9.72s 9.81s p=0.573 n=6
Emit Time 2.74s (± 0.44%) 2.73s (± 0.95%) ~ 2.69s 2.76s p=0.806 n=6
Total Time 14.54s (± 0.40%) 14.52s (± 0.30%) ~ 14.44s 14.57s p=0.468 n=6
angular-1 - node (v18.15.0, x64)
Errors 37 38 +1 (+ 2.70%) ~ ~ p=0.001 n=6
Symbols 947,936 947,936 ~ ~ ~ p=1.000 n=6
Types 410,955 410,965 +10 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 1,225,832k (± 0.00%) 1,225,923k (± 0.00%) +92k (+ 0.01%) 1,225,883k 1,225,985k p=0.005 n=6
Parse Time 6.61s (± 0.62%) 6.63s (± 0.63%) ~ 6.59s 6.71s p=0.257 n=6
Bind Time 1.89s (± 0.33%) 1.89s (± 0.43%) ~ 1.88s 1.90s p=0.432 n=6
Check Time 31.96s (± 0.31%) 32.05s (± 0.28%) ~ 31.89s 32.14s p=0.121 n=6
Emit Time 15.14s (± 0.50%) 15.16s (± 0.23%) ~ 15.11s 15.20s p=0.936 n=6
Total Time 55.60s (± 0.36%) 55.73s (± 0.18%) ~ 55.60s 55.85s p=0.378 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,447,028 2,447,028 ~ ~ ~ p=1.000 n=6
Types 896,160 896,160 ~ ~ ~ p=1.000 n=6
Memory used 2,318,460k (± 0.00%) 2,318,452k (± 0.00%) ~ 2,318,387k 2,318,517k p=1.000 n=6
Parse Time 9.44s (± 0.29%) 9.44s (± 0.23%) ~ 9.40s 9.46s p=0.625 n=6
Bind Time 2.23s (± 0.37%) 2.23s (± 0.46%) ~ 2.21s 2.24s p=1.000 n=6
Check Time 73.29s (± 0.41%) 73.65s (± 0.45%) ~ 73.19s 74.03s p=0.092 n=6
Emit Time 0.29s (± 2.61%) 0.29s (± 3.68%) ~ 0.27s 0.30s p=0.611 n=6
Total Time 85.24s (± 0.36%) 85.60s (± 0.40%) ~ 85.13s 85.99s p=0.093 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,328 1,225,338 +10 (+ 0.00%) ~ ~ p=0.001 n=6
Types 266,569 266,575 +6 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 2,414,691k (± 6.13%) 2,354,565k (± 0.03%) ~ 2,353,602k 2,355,589k p=0.689 n=6
Parse Time 5.24s (± 0.88%) 5.21s (± 1.41%) ~ 5.13s 5.32s p=0.468 n=6
Bind Time 1.77s (± 1.09%) 1.77s (± 1.27%) ~ 1.75s 1.80s p=1.000 n=6
Check Time 35.13s (± 0.56%) 35.24s (± 0.15%) ~ 35.18s 35.31s p=0.173 n=6
Emit Time 2.95s (± 0.76%) 2.93s (± 2.51%) ~ 2.81s 2.99s p=0.688 n=6
Total Time 45.11s (± 0.39%) 45.19s (± 0.33%) ~ 44.94s 45.38s p=0.230 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,328 1,225,338 +10 (+ 0.00%) ~ ~ p=0.001 n=6
Types 266,569 266,575 +6 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 2,908,034k (±12.85%) 3,030,024k (± 9.75%) ~ 2,426,400k 3,151,558k p=0.128 n=6
Parse Time 6.95s (± 1.84%) 7.00s (± 1.47%) ~ 6.85s 7.10s p=0.423 n=6
Bind Time 2.17s (± 1.19%) 2.15s (± 0.68%) ~ 2.13s 2.17s p=0.125 n=6
Check Time 42.74s (± 0.59%) 42.84s (± 0.54%) ~ 42.43s 43.08s p=0.630 n=6
Emit Time 3.51s (± 2.96%) 3.50s (± 1.67%) ~ 3.44s 3.59s p=0.873 n=6
Total Time 55.37s (± 0.73%) 55.50s (± 0.50%) ~ 55.00s 55.71s p=0.688 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 262,267 262,277 +10 (+ 0.00%) ~ ~ p=0.001 n=6
Types 106,628 106,634 +6 (+ 0.01%) ~ ~ p=0.001 n=6
Memory used 439,879k (± 0.01%) 440,070k (± 0.01%) +191k (+ 0.04%) 439,994k 440,163k p=0.005 n=6
Parse Time 3.54s (± 0.39%) 3.54s (± 1.02%) ~ 3.49s 3.59s p=0.745 n=6
Bind Time 1.32s (± 1.34%) 1.32s (± 0.78%) ~ 1.30s 1.33s p=0.805 n=6
Check Time 18.94s (± 0.74%) 18.96s (± 0.38%) ~ 18.88s 19.06s p=0.810 n=6
Emit Time 1.53s (± 0.76%) 1.53s (± 0.87%) ~ 1.52s 1.55s p=1.000 n=6
Total Time 25.32s (± 0.65%) 25.35s (± 0.26%) ~ 25.22s 25.40s p=1.000 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 70 70 ~ ~ ~ p=1.000 n=6
Symbols 226,062 226,062 ~ ~ ~ p=1.000 n=6
Types 94,488 94,491 +3 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 371,615k (± 0.01%) 371,741k (± 0.03%) +126k (+ 0.03%) 371,638k 371,926k p=0.020 n=6
Parse Time 2.91s (± 1.45%) 2.88s (± 0.58%) ~ 2.87s 2.91s p=0.119 n=6
Bind Time 1.59s (± 1.79%) 1.59s (± 0.86%) ~ 1.57s 1.61s p=1.000 n=6
Check Time 16.50s (± 0.39%) 16.53s (± 0.21%) ~ 16.50s 16.59s p=0.574 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 21.00s (± 0.17%) 20.99s (± 0.13%) ~ 20.96s 21.04s p=0.871 n=6
vscode - node (v18.15.0, x64)
Errors 3 3 ~ ~ ~ p=1.000 n=6
Symbols 3,197,783 3,197,783 ~ ~ ~ p=1.000 n=6
Types 1,099,256 1,099,258 +2 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 3,271,546k (± 0.01%) 3,272,164k (± 0.01%) +618k (+ 0.02%) 3,271,726k 3,272,545k p=0.031 n=6
Parse Time 14.14s (± 0.31%) 14.23s (± 0.56%) ~ 14.14s 14.37s p=0.054 n=6
Bind Time 4.53s (± 0.55%) 4.52s (± 0.69%) ~ 4.47s 4.56s p=0.808 n=6
Check Time 86.56s (± 0.42%) 86.43s (± 0.59%) ~ 85.79s 87.34s p=0.298 n=6
Emit Time 28.13s (± 2.01%) 27.91s (± 2.97%) ~ 27.25s 29.27s p=0.748 n=6
Total Time 133.36s (± 0.44%) 133.10s (± 0.91%) ~ 131.69s 134.66s p=0.471 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 288,747 288,747 ~ ~ ~ p=1.000 n=6
Types 117,158 117,158 ~ ~ ~ p=1.000 n=6
Memory used 440,915k (± 0.02%) 441,098k (± 0.04%) +183k (+ 0.04%) 440,914k 441,429k p=0.045 n=6
Parse Time 4.10s (± 0.78%) 4.06s (± 1.33%) ~ 3.99s 4.15s p=0.228 n=6
Bind Time 1.76s (± 1.70%) 1.76s (± 1.05%) ~ 1.74s 1.79s p=0.871 n=6
Check Time 18.84s (± 0.39%) 18.86s (± 0.53%) ~ 18.79s 19.06s p=0.688 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.70s (± 0.54%) 24.69s (± 0.55%) ~ 24.55s 24.94s p=0.518 n=6
xstate-main - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 552,389 552,389 ~ ~ ~ p=1.000 n=6
Types 185,093 185,093 ~ ~ ~ p=1.000 n=6
Memory used 492,423k (± 0.02%) 492,524k (± 0.01%) +101k (+ 0.02%) 492,404k 492,598k p=0.045 n=6
Parse Time 3.41s (± 1.03%) 3.41s (± 0.51%) ~ 3.38s 3.43s p=0.627 n=6
Bind Time 1.17s (± 1.71%) 1.17s (± 1.17%) ~ 1.15s 1.19s p=0.869 n=6
Check Time 19.59s (± 1.26%) 19.46s (± 0.21%) ~ 19.40s 19.52s p=0.196 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.17s (± 1.01%) 24.06s (± 0.19%) ~ 24.00s 24.11s p=0.335 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/60715/merge:

Something interesting changed - please have a look.

Details

snabbdom/snabbdom

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

tsconfig.json

  • error TS2345: Argument of type 'string | number' is not assignable to parameter of type 'string | undefined'.

@Andarist
Copy link
Contributor Author

The snabbdom break isn't harmful. It boils down to a code like this:

declare function isPrimitive(s: any): s is string | number;

export function test() {
  let c: any;
  for (let i = 0; i < c.length; ++i) {
    if (isPrimitive(c[i])) {
      const target: string | undefined = c[i];
    }
  }
}

The problem is that c is any and thus, before this change, c[i] stayed as any too so it was, naturally, assignable to string | undefined. But isPrimitive is a type predicate and it can narrow down any:

declare function isPrimitive(s: any): s is string | number;
declare const foo: any;
if (isPrimitive(foo)) {
  foo; // string | number
}

Since now this constant-like c[i] can be narrowed down it breaks because string | number isn't assignable to string | undefined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

Narrowing for constant indexed access fails on array getter
3 participants