Skip to content

Fix issue with optional chaining and type inference in type guard #55613

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

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Sep 3, 2023

Fixes #34974

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 3, 2023
@jakebailey
Copy link
Member

@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 3, 2023

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 1fddcab. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 3, 2023

Heya @jakebailey, I've started to run the regular perf test suite on this PR at 1fddcab. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 3, 2023

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 1fddcab. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 3, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 1fddcab. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/55613/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 2 instances of "Package install failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 300,271k (± 0.01%) 300,263k (± 0.01%) ~ 300,246k 300,305k p=0.336 n=6
Parse Time 3.01s (± 0.27%) 3.01s (± 0.28%) ~ 3.00s 3.02s p=0.718 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=1.000 n=6
Check Time 9.33s (± 0.24%) 9.31s (± 0.23%) ~ 9.28s 9.34s p=0.164 n=6
Emit Time 7.63s (± 0.20%) 7.62s (± 0.27%) ~ 7.59s 7.64s p=0.324 n=6
Total Time 20.90s (± 0.19%) 20.87s (± 0.13%) ~ 20.84s 20.90s p=0.145 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 193,923k (± 0.02%) 193,939k (± 0.02%) ~ 193,860k 193,988k p=0.689 n=6
Parse Time 1.58s (± 0.00%) 1.58s (± 0.00%) ~ 1.58s 1.58s p=1.000 n=6
Bind Time 0.79s (± 0.00%) 0.80s (± 0.65%) +0.01s (+ 0.84%) 0.79s 0.80s p=0.025 n=6
Check Time 9.92s (± 0.26%) 9.95s (± 0.50%) ~ 9.88s 10.03s p=0.145 n=6
Emit Time 2.73s (± 0.31%) 2.74s (± 0.36%) ~ 2.73s 2.75s p=0.734 n=6
Total Time 15.03s (± 0.23%) 15.06s (± 0.26%) ~ 15.01s 15.13s p=0.107 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,199k (± 0.00%) 347,205k (± 0.00%) ~ 347,191k 347,229k p=0.199 n=6
Parse Time 2.69s (± 0.38%) 2.69s (± 0.33%) ~ 2.68s 2.70s p=0.673 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.00%) ~ 0.99s 0.99s p=1.000 n=6
Check Time 7.92s (± 0.22%) 7.93s (± 0.25%) ~ 7.90s 7.96s p=0.567 n=6
Emit Time 4.27s (± 0.12%) 4.25s (± 0.44%) ~ 4.23s 4.28s p=0.079 n=6
Total Time 15.86s (± 0.17%) 15.86s (± 0.11%) ~ 15.83s 15.88s p=0.627 n=6
TFS - node (v16.17.1, x64)
Memory used 301,174k (± 0.00%) 301,176k (± 0.00%) ~ 301,157k 301,195k p=1.000 n=6
Parse Time 2.17s (± 0.56%) 2.16s (± 0.63%) ~ 2.15s 2.19s p=0.170 n=6
Bind Time 1.09s (± 5.33%) 1.11s (± 0.37%) ~ 1.11s 1.12s p=0.673 n=6
Check Time 7.25s (± 0.56%) 7.25s (± 0.23%) ~ 7.22s 7.26s p=1.000 n=6
Emit Time 3.98s (± 0.40%) 3.98s (± 0.40%) ~ 3.97s 4.01s p=0.675 n=6
Total Time 14.49s (± 0.75%) 14.50s (± 0.23%) ~ 14.46s 14.55s p=0.376 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,470k (± 0.00%) 479,469k (± 0.00%) ~ 479,448k 479,483k p=1.000 n=6
Parse Time 3.15s (± 0.16%) 3.15s (± 0.31%) ~ 3.14s 3.17s p=0.386 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 17.77s (± 0.19%) 17.84s (± 0.34%) ~ 17.77s 17.91s p=0.065 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 21.82s (± 0.16%) 21.90s (± 0.25%) +0.07s (+ 0.34%) 21.84s 21.96s p=0.034 n=6
xstate - node (v16.17.1, x64)
Memory used 542,826k (± 0.01%) 542,851k (± 0.01%) ~ 542,804k 542,925k p=0.936 n=6
Parse Time 3.69s (± 0.22%) 3.70s (± 0.20%) ~ 3.69s 3.71s p=0.383 n=6
Bind Time 1.40s (± 4.59%) 1.38s (± 4.42%) ~ 1.33s 1.46s p=0.801 n=6
Check Time 3.25s (± 2.68%) 3.28s (± 2.55%) ~ 3.16s 3.34s p=0.746 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.405 n=6
Total Time 8.42s (± 0.44%) 8.43s (± 0.34%) ~ 8.38s 8.46s p=0.376 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,489ms (± 0.15%) 2,491ms (± 0.07%) ~ 2,488ms 2,493ms p=0.681 n=6
Req 2 - geterr 5,940ms (± 0.23%) 5,946ms (± 0.35%) ~ 5,915ms 5,966ms p=0.575 n=6
Req 3 - references 344ms (± 0.48%) 344ms (± 0.40%) ~ 342ms 346ms p=0.867 n=6
Req 4 - navto 277ms (± 0.37%) 278ms (± 0.62%) ~ 276ms 281ms p=0.737 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 86ms (±10.32%) 82ms (± 7.59%) ~ 76ms 93ms p=0.510 n=6
CompilerTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,624ms (± 0.84%) 2,612ms (± 0.55%) ~ 2,601ms 2,640ms p=0.170 n=6
Req 2 - geterr 4,776ms (± 0.19%) 4,770ms (± 0.15%) ~ 4,762ms 4,784ms p=0.195 n=6
Req 3 - references 354ms (± 2.22%) 350ms (± 0.21%) ~ 349ms 351ms p=0.150 n=6
Req 4 - navto 269ms (± 0.19%) 270ms (± 0.19%) ~ 269ms 270ms p=0.311 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 79ms (± 0.52%) 79ms (± 0.52%) ~ 78ms 79ms p=0.218 n=6
xstateTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,710ms (± 0.18%) 2,711ms (± 0.13%) ~ 2,705ms 2,714ms p=0.629 n=6
Req 2 - geterr 1,965ms (± 1.03%) 1,970ms (± 0.52%) ~ 1,957ms 1,988ms p=0.936 n=6
Req 3 - references 139ms (± 1.87%) 137ms (± 3.04%) ~ 131ms 142ms p=0.687 n=6
Req 4 - navto 352ms (± 0.34%) 352ms (± 0.34%) ~ 351ms 354ms p=1.000 n=6
Req 5 - completionInfo count 2,071 (± 0.00%) 2,071 (± 0.00%) ~ 2,071 2,071 p=1.000 n=6
Req 5 - completionInfo 323ms (± 1.62%) 321ms (± 1.62%) ~ 314ms 326ms p=0.572 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • CompilerTSServer - node (v16.17.1, x64)
  • Compiler-UnionsTSServer - node (v16.17.1, x64)
  • xstateTSServer - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v16.17.1, x64)
Execution time 156.36ms (± 0.15%) 156.40ms (± 0.15%) +0.04ms (+ 0.03%) 154.96ms 158.64ms p=0.006 n=600
tsserver-startup - node (v16.17.1, x64)
Execution time 230.44ms (± 0.14%) 231.19ms (± 0.13%) +0.75ms (+ 0.33%) 229.75ms 235.79ms p=0.000 n=600
tsserverlibrary-startup - node (v16.17.1, x64)
Execution time 235.25ms (± 0.12%) 236.10ms (± 0.17%) +0.86ms (+ 0.36%) 234.66ms 249.11ms p=0.000 n=600
typescript-startup - node (v16.17.1, x64)
Execution time 236.17ms (± 0.15%) 236.03ms (± 0.15%) -0.13ms (- 0.06%) 234.32ms 243.19ms p=0.000 n=600
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • tsc-startup - node (v16.17.1, x64)
  • tsserver-startup - node (v16.17.1, x64)
  • tsserverlibrary-startup - node (v16.17.1, x64)
  • typescript-startup - node (v16.17.1, 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-repos suite comparing main and refs/pull/55613/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.

};

const getName2 = (person?: Person): string => {
return isString(person?.name) ? person?.name : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be changed to this?

Suggested change
return isString(person?.name) ? person?.name : '';
return isString(person?.name) ? person.name : '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both should be fine. If I recall correctly, optional chaining does not alter the type of the expression it is applied to.

Copy link
Contributor

@Andarist Andarist Sep 3, 2023

Choose a reason for hiding this comment

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

Ye, right - i thought that previously the error was also raised within the truthy branch of the conditional expression (as in that the obj wasnt being proven to be non-nullable) but it seems that this part was already working ok

type Person = { name: string; }

const getName1 = (person?: Person): string => {
return typeof person?.name === 'string' ? person?.name : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly here:

Suggested change
return typeof person?.name === 'string' ? person?.name : '';
return typeof person?.name === 'string' ? person.name : '';

@fatcerberus
Copy link

It must be getting too close to spooky season because I read the issue title as "Fix issue with ... type gourd" 🎃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Optional chaining, proper infer in type guard
6 participants