Skip to content

Fix a logic-changing typo in getRecursionIdentity #54321

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

Conversation

angryzor
Copy link
Contributor

@angryzor angryzor commented May 19, 2023

There is currently a typo bug in the getRecursionIdentity function that changes the logic of the function in an unexpected way. This PR fixes that bug.

Please verify that:

  • There is an associated issue in the Backlog milestone (required)
  • Code is up-to-date with the main branch
  • You've successfully run hereby runtests locally
  • There are new or updated unit tests validating the change - there is no new functionality added, this PR fixes a small (1 character) logic mistake in the existing code

Fixes #54320

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 19, 2023
@jakebailey
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 19, 2023

Heya @jakebailey, I've started to run the extended test suite on this PR at 8e185f5. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 19, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..54321

Metric main 54321 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 365,139k (± 0.01%) 365,066k (± 0.02%) ~ 365,002k 365,160k p=0.066 n=6
Parse Time 3.56s (± 0.86%) 3.55s (± 0.15%) ~ 3.55s 3.56s p=0.797 n=6
Bind Time 1.18s (± 0.69%) 1.19s (± 0.98%) ~ 1.17s 1.20s p=0.401 n=6
Check Time 9.62s (± 0.38%) 9.59s (± 0.57%) ~ 9.50s 9.65s p=0.520 n=6
Emit Time 8.00s (± 0.65%) 7.94s (± 0.87%) ~ 7.86s 8.02s p=0.128 n=6
Total Time 22.36s (± 0.44%) 22.27s (± 0.52%) ~ 22.08s 22.37s p=0.107 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 192,916k (± 0.04%) 192,957k (± 0.04%) ~ 192,859k 193,020k p=0.470 n=6
Parse Time 1.60s (± 0.97%) 1.61s (± 1.04%) ~ 1.58s 1.63s p=0.171 n=6
Bind Time 0.83s (± 0.98%) 0.83s (± 0.00%) ~ 0.83s 0.83s p=0.405 n=6
Check Time 10.25s (± 0.54%) 10.21s (± 0.72%) ~ 10.08s 10.28s p=0.334 n=6
Emit Time 3.01s (± 0.80%) 3.02s (± 1.02%) ~ 2.98s 3.07s p=0.629 n=6
Total Time 15.70s (± 0.29%) 15.67s (± 0.62%) ~ 15.53s 15.81s p=0.627 n=6
Monaco - node (v16.17.1, x64)
Memory used 345,884k (± 0.01%) 345,866k (± 0.00%) ~ 345,841k 345,884k p=0.377 n=6
Parse Time 2.73s (± 0.83%) 2.73s (± 0.38%) ~ 2.72s 2.75s p=0.100 n=6
Bind Time 1.08s (± 0.50%) 1.09s (± 0.82%) ~ 1.08s 1.10s p=0.341 n=6
Check Time 7.84s (± 0.29%) 7.85s (± 0.41%) ~ 7.83s 7.91s p=0.869 n=6
Emit Time 4.49s (± 1.09%) 4.45s (± 0.48%) ~ 4.43s 4.48s p=0.075 n=6
Total Time 16.14s (± 0.49%) 16.12s (± 0.23%) ~ 16.08s 16.18s p=0.872 n=6
TFS - node (v16.17.1, x64)
Memory used 299,958k (± 0.01%) 299,960k (± 0.01%) ~ 299,935k 299,982k p=1.000 n=6
Parse Time 2.17s (± 0.68%) 2.17s (± 0.82%) ~ 2.15s 2.20s p=1.000 n=6
Bind Time 1.24s (± 0.88%) 1.24s (± 0.79%) ~ 1.23s 1.25s p=0.859 n=6
Check Time 7.29s (± 0.40%) 7.29s (± 0.47%) ~ 7.25s 7.33s p=0.808 n=6
Emit Time 4.33s (± 1.04%) 4.37s (± 0.66%) ~ 4.33s 4.41s p=0.147 n=6
Total Time 15.02s (± 0.31%) 15.06s (± 0.42%) ~ 14.96s 15.14s p=0.298 n=6
material-ui - node (v16.17.1, x64)
Memory used 480,937k (± 0.01%) 480,940k (± 0.01%) ~ 480,895k 480,980k p=0.810 n=6
Parse Time 3.25s (± 0.45%) 3.26s (± 0.60%) ~ 3.24s 3.28s p=0.188 n=6
Bind Time 0.94s (± 0.43%) 0.94s (± 0.55%) ~ 0.94s 0.95s p=0.595 n=6
Check Time 17.83s (± 0.58%) 17.93s (± 0.83%) ~ 17.76s 18.11s p=0.423 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.02s (± 0.52%) 22.14s (± 0.72%) ~ 21.94s 22.34s p=0.173 n=6
xstate - node (v16.17.1, x64)
Memory used 560,512k (± 0.01%) 560,517k (± 0.03%) ~ 560,390k 560,817k p=0.378 n=6
Parse Time 4.01s (± 0.41%) 4.03s (± 0.16%) ~ 4.02s 4.04s p=0.073 n=6
Bind Time 1.77s (± 0.46%) 1.78s (± 0.55%) ~ 1.77s 1.79s p=0.065 n=6
Check Time 3.05s (± 0.86%) 3.07s (± 0.63%) ~ 3.04s 3.09s p=0.259 n=6
Emit Time 0.09s (± 6.44%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=0.071 n=6
Total Time 8.92s (± 0.46%) 8.97s (± 0.27%) +0.05s (+ 0.50%) 8.93s 8.99s p=0.045 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-148-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
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 54321 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

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

Otherwise...

Everything looks good!

@fatcerberus
Copy link

There are new or updated unit tests validating the change - there is no new functionality added, this PR fixes a small (1 character) logic mistake in the existing code

Of course this is a bug either way, but ideally there would be a regression test… does this observably change the behavior of any TS code?

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/54321/merge:

Everything looks good!

@angryzor
Copy link
Contributor Author

There are new or updated unit tests validating the change - there is no new functionality added, this PR fixes a small (1 character) logic mistake in the existing code

Of course this is a bug either way, but ideally there would be a regression test… does this observably change the behavior of any TS code?

The problem is that I stumbled upon this completely by accident while reading the source trying to understand how the infinite recursion prevention mechanism works (because my fork was infinitely recursing 😅). In other words I currently do not understand this part of the code base well enough to know with certainty what functionality this bug may affect. If there is a noticeable effect for the end user then it was in any case not covered by existing unit tests as these pass without any modification.

I can tell you however that the bug is currently mostly masked by the additional condition right after it: && (type as TypeReference).node. This extra check significantly narrows down the types that could end up in this conditional branch. After looking at the existing Type classes there are only 3 that include this property and thus could possibly end up in this branch:

  • TypeReference
  • DeferredTypeReference
  • InstantiationExpressionType

The first 2 are obviously fine as they are the types that are supposed to end up in that branch, just the last one needs to be verified.

@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.

@RyanCavanaugh RyanCavanaugh merged commit 0bfc9f5 into microsoft:main May 22, 2023
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.

getRecursionIdentity currently runs a boolean AND instead of a bitwise AND on the objectFlags of its input
5 participants