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

Experiment: report functions with multiple returns that could be type predicates #58173

Closed
wants to merge 5 commits into from

Conversation

danvk
Copy link
Contributor

@danvk danvk commented Apr 12, 2024

This is a side experiment for #58154. That PR extends type predicate inference to functions with multiple returns. It seems to have minimal perf impact and also minimal breakages. This makes you wonder… are there any such functions?

This fork of the PR should answer that question by reporting errors on all functions with 2+ return statements where it can infer a type predicate. To be clear, these aren't real errors! They're just identifying multi-return predicates in the wild.

There are three such function in TypeScript itself:

  1. tryAddPropertyAssignment in nodeFactory.ts:
function tryAddPropertyAssignment(properties: PropertyAssignment[], propertyName: string, expression: Expression | undefined) {
    if (expression) {
        properties.push(createPropertyAssignment(propertyName, expression));
        return true;
    }
    return false;
}

This gets an inferred return type of expression is Expression. This is accurate, though not consequential for any of the code that calls this function.

  1. charIsPunctuation in patternMatcher.ts:
function charIsPunctuation(ch: number) {
    switch (ch) {
        case CharacterCodes.exclamation:
        case CharacterCodes.doubleQuote:
        // ...
        case CharacterCodes._:
        case CharacterCodes.openBrace:
        case CharacterCodes.closeBrace:
            return true;
    }

    return false;
}

This gets an inferred return type of ch is CharacterCodes._ | CharacterCodes.ampersand | CharacterCodes.asterisk | ... | CharacterCodes.slash. Again, this is accurate but not consequential for any of the calling code.

  1. An anonymous function in getLinkedEditingRangeAtPosition in services.ts:
const tag = findAncestor(token.parent, n => {
    if (isJsxOpeningElement(n) || isJsxClosingElement(n)) {
        return true;
    }
    return false;
});
if (!tag) return undefined;
Debug.assert(isJsxOpeningElement(tag) || isJsxClosingElement(tag), "tag should be opening or closing element");

The arrow function becomes a type predicate with #58154 which changes the type of tag. This is a positive change that eliminates the need for the Debug.assert statement.

Of course, the function could have been written more succinctly as n => isJsxOpeningElement(n) || isJsxClosingElement(n) and the existing code would infer a type predicate.

I'm curious what other errors this reports on the user/top/etc suites. This will characterize whether this is a common pattern in the wild.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 12, 2024
@RyanCavanaugh
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 12, 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

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

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the user tests comparing main and refs/pull/58173/merge:

Something interesting changed - please have a look.

Details

pyright

/mnt/ts_downloads/_/m/pyright/build.sh

  • [NEW] error TS1499: Function with multiple returns is implicitly a type predicate.
    • /mnt/ts_downloads/_/m/pyright/pyright: ../pyright-internal/src/analyzer/parseTreeUtils.ts(2588,17)
    • /mnt/ts_downloads/_/m/pyright/pyright-internal: src/analyzer/parseTreeUtils.ts(2588,17)
    • /mnt/ts_downloads/_/m/pyright/vscode-pyright: ../pyright-internal/src/analyzer/parseTreeUtils.ts(2588,17)

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh
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
Angular - node (v18.15.0, x64)
Memory used 296,989k (± 0.00%) 297,001k (± 0.01%) ~ 296,971k 297,013k p=0.298 n=6
Parse Time 2.69s (± 0.45%) 2.69s (± 0.38%) ~ 2.67s 2.70s p=0.408 n=6
Bind Time 0.82s (± 0.50%) 0.82s (± 0.00%) ~ 0.82s 0.82s p=0.405 n=6
Check Time 8.33s (± 0.42%) 8.35s (± 0.37%) ~ 8.32s 8.40s p=0.469 n=6
Emit Time 7.05s (± 0.31%) 7.05s (± 0.32%) ~ 7.01s 7.07s p=1.000 n=6
Total Time 18.89s (± 0.20%) 18.90s (± 0.26%) ~ 18.83s 18.95s p=0.629 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,371k (± 0.74%) 194,332k (± 0.94%) +1,961k (+ 1.02%) 191,962k 195,646k p=0.013 n=6
Parse Time 2.01s (± 1.53%) 2.02s (± 0.60%) ~ 2.01s 2.04s p=0.416 n=6
Bind Time 1.07s (± 0.92%) 1.07s (± 0.97%) ~ 1.05s 1.08s p=0.788 n=6
Check Time 14.04s (± 0.18%) 14.06s (± 0.50%) ~ 13.96s 14.13s p=0.574 n=6
Emit Time 3.89s (± 2.32%) 3.87s (± 1.04%) ~ 3.79s 3.90s p=0.292 n=6
Total Time 21.01s (± 0.49%) 21.02s (± 0.42%) ~ 20.88s 21.09s p=0.747 n=6
Monaco - node (v18.15.0, x64)
Memory used 348,863k (± 0.01%) 348,866k (± 0.00%) ~ 348,847k 348,882k p=0.688 n=6
Parse Time 3.85s (± 0.86%) 3.85s (± 1.12%) ~ 3.79s 3.90s p=0.936 n=6
Bind Time 1.32s (± 0.62%) 1.32s (± 0.80%) ~ 1.30s 1.33s p=0.865 n=6
Check Time 10.26s (± 0.42%) 10.27s (± 0.49%) ~ 10.18s 10.31s p=0.872 n=6
Emit Time 5.99s (± 0.52%) 6.00s (± 0.61%) ~ 5.94s 6.03s p=0.809 n=6
Total Time 21.42s (± 0.32%) 21.42s (± 0.23%) ~ 21.35s 21.50s p=1.000 n=6
TFS - node (v18.15.0, x64)
Memory used 302,975k (± 0.01%) 302,982k (± 0.00%) ~ 302,964k 303,001k p=0.471 n=6
Parse Time 2.04s (± 0.51%) 2.02s (± 0.67%) ~ 2.01s 2.04s p=0.161 n=6
Bind Time 0.98s (± 0.77%) 0.98s (± 0.77%) ~ 0.97s 0.99s p=1.000 n=6
Check Time 6.47s (± 0.49%) 6.45s (± 0.51%) ~ 6.41s 6.51s p=0.519 n=6
Emit Time 3.58s (± 0.23%) 3.58s (± 0.51%) ~ 3.56s 3.60s p=1.000 n=6
Total Time 13.07s (± 0.27%) 13.04s (± 0.31%) ~ 12.97s 13.08s p=0.170 n=6
material-ui - node (v18.15.0, x64)
Memory used 511,624k (± 0.01%) 511,617k (± 0.00%) ~ 511,579k 511,646k p=0.575 n=6
Parse Time 4.01s (± 0.66%) 4.02s (± 1.04%) ~ 3.99s 4.09s p=0.681 n=6
Bind Time 1.46s (± 1.42%) 1.45s (± 2.02%) ~ 1.42s 1.50s p=0.872 n=6
Check Time 25.32s (± 0.24%) 25.27s (± 0.18%) ~ 25.22s 25.35s p=0.148 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 30.79s (± 0.23%) 30.75s (± 0.22%) ~ 30.69s 30.88s p=0.335 n=6
mui-docs - node (v18.15.0, x64)
Memory used 1,748,693k (± 0.00%) 1,748,679k (± 0.00%) ~ 1,748,618k 1,748,755k p=0.471 n=6
Parse Time 9.96s (± 0.38%) 10.03s (± 0.88%) ~ 9.93s 10.12s p=0.261 n=6
Bind Time 3.35s (± 0.27%) 3.36s (± 0.87%) ~ 3.32s 3.40s p=0.370 n=6
Check Time 81.76s (± 0.27%) 82.04s (± 0.60%) ~ 81.29s 82.68s p=0.230 n=6
Emit Time 0.19s (± 2.81%) 0.20s (± 5.07%) ~ 0.19s 0.22s p=0.138 n=6
Total Time 95.26s (± 0.21%) 95.63s (± 0.47%) ~ 95.00s 96.17s p=0.229 n=6
self-build-src - node (v18.15.0, x64)
Memory used 2,306,333k (± 0.06%) 2,307,414k (± 0.03%) ~ 2,306,245k 2,308,046k p=0.093 n=6
Parse Time 6.04s (± 0.68%) 6.03s (± 0.69%) ~ 5.97s 6.08s p=0.872 n=6
Bind Time 2.23s (± 1.05%) 2.24s (± 1.28%) ~ 2.20s 2.29s p=0.520 n=6
Check Time 39.84s (± 0.28%) 39.90s (± 0.39%) ~ 39.72s 40.17s p=0.423 n=6
Emit Time 3.17s (± 3.58%) 3.19s (± 3.58%) ~ 3.04s 3.35s p=1.000 n=6
Total Time 51.30s (± 0.43%) 51.38s (± 0.38%) ~ 51.15s 51.67s p=0.688 n=6
self-build-src-public-api - node (v18.15.0, x64)
Memory used 2,380,781k (± 0.04%) 2,381,898k (± 0.03%) ~ 2,381,032k 2,382,892k p=0.066 n=6
Parse Time 7.63s (± 0.36%) 7.66s (± 0.82%) ~ 7.56s 7.74s p=0.230 n=6
Bind Time 2.52s (± 1.24%) 2.51s (± 1.12%) ~ 2.48s 2.56s p=0.368 n=6
Check Time 49.80s (± 0.92%) 49.52s (± 0.53%) ~ 49.22s 49.97s p=0.298 n=6
Emit Time 3.92s (± 1.70%) 3.96s (± 1.99%) ~ 3.83s 4.03s p=0.261 n=6
Total Time 63.87s (± 0.70%) 63.65s (± 0.40%) ~ 63.29s 64.03s p=0.575 n=6
self-compiler - node (v18.15.0, x64)
Memory used 419,069k (± 0.01%) 419,192k (± 0.01%) +123k (+ 0.03%) 419,131k 419,245k p=0.005 n=6
Parse Time 3.40s (± 0.76%) 3.37s (± 2.43%) ~ 3.21s 3.44s p=0.808 n=6
Bind Time 1.31s (± 1.01%) 1.33s (± 4.80%) ~ 1.26s 1.45s p=0.466 n=6
Check Time 18.04s (± 0.30%) 18.02s (± 0.41%) ~ 17.94s 18.14s p=0.686 n=6
Emit Time 1.37s (± 1.36%) 1.37s (± 2.62%) ~ 1.33s 1.41s p=0.872 n=6
Total Time 24.11s (± 0.20%) 24.10s (± 0.32%) ~ 24.01s 24.22s p=0.748 n=6
vscode - node (v18.15.0, x64)
Memory used 2,912,926k (± 0.00%) 2,912,839k (± 0.00%) ~ 2,912,683k 2,912,925k p=0.066 n=6
Parse Time 13.39s (± 0.21%) 13.42s (± 0.34%) ~ 13.35s 13.49s p=0.255 n=6
Bind Time 4.06s (± 0.40%) 4.10s (± 2.29%) ~ 4.05s 4.29s p=0.807 n=6
Check Time 72.46s (± 0.41%) 72.48s (± 0.27%) ~ 72.33s 72.80s p=0.689 n=6
Emit Time 19.48s (± 0.82%) 20.14s (± 7.49%) ~ 19.37s 23.22s p=0.172 n=6
Total Time 109.39s (± 0.39%) 110.15s (± 1.31%) ~ 109.31s 113.06s p=0.199 n=6
webpack - node (v18.15.0, x64)
Memory used 409,375k (± 0.01%) 409,385k (± 0.01%) ~ 409,324k 409,429k p=0.689 n=6
Parse Time 3.26s (± 0.82%) 3.27s (± 1.19%) ~ 3.24s 3.32s p=0.466 n=6
Bind Time 1.37s (± 0.75%) 1.38s (± 0.76%) ~ 1.37s 1.40s p=0.098 n=6
Check Time 14.38s (± 0.15%) 14.39s (± 0.29%) ~ 14.35s 14.45s p=0.872 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 19.01s (± 0.23%) 19.04s (± 0.21%) ~ 18.99s 19.08s p=0.191 n=6
xstate - node (v18.15.0, x64)
Memory used 671,977k (± 0.01%) 672,047k (± 0.02%) ~ 671,934k 672,265k p=0.689 n=6
Parse Time 4.12s (± 0.84%) 4.12s (± 0.74%) ~ 4.08s 4.17s p=1.000 n=6
Bind Time 1.87s (± 0.83%) 1.88s (± 0.87%) ~ 1.86s 1.90s p=0.511 n=6
Check Time 3.43s (± 0.24%) 3.45s (± 0.57%) ~ 3.42s 3.47s p=0.081 n=6
Emit Time 0.03s (±19.35%) 0.03s (±14.39%) ~ 0.02s 0.03s p=0.595 n=6
Total Time 9.46s (± 0.48%) 9.47s (± 0.58%) ~ 9.40s 9.56s p=0.746 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-build-src-public-api - 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)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@jakebailey
Copy link
Member

jakebailey commented Apr 12, 2024

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the top 400 repos comparing main and refs/pull/58173/merge:

Something interesting changed - please have a look.

Details

facebook/lexical

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

tsconfig.json

Tencent/omi

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

packages/reactive-signal/tsconfig.json

packages/reactive-signal/examples/tsconfig.json

@jakebailey
Copy link
Member

I feel like it's telling that we're seeing so few of these, and that when we do, the code still compiles as it did before. That makes it seem like this isn't very valuable, and that the initial design was good enough.

@danvk
Copy link
Contributor Author

danvk commented Apr 12, 2024

Closing this out since we've learned what we're going to from it.

@danvk danvk closed this Apr 12, 2024
@jakebailey
Copy link
Member

BTW thank you for sending these PRs in exactly the format we need to let the infra report something, super helpful.

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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants