Skip to content

fix(55899): Indexing/element access on super avoids instance property checks #55903

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

Closed
wants to merge 3 commits into from

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #55899

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Sep 28, 2023
@DanielRosenwasser
Copy link
Member

If my memory serves me right, indexed accesses might've been an escape hatch to opt out of visibility modifiers. Maybe @RyanCavanaugh remembers better.

That said, let's see how this works

@typescript-bot perf test this
@typescript-bot test top200

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 28, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 28, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
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 (v18.15.0, x64)
Memory used 295,004k (± 0.01%) 295,027k (± 0.01%) ~ 294,978k 295,070k p=0.298 n=6
Parse Time 2.64s (± 0.37%) 2.64s (± 0.31%) ~ 2.63s 2.65s p=0.862 n=6
Bind Time 0.84s (± 0.90%) 0.84s (± 1.30%) ~ 0.83s 0.85s p=0.865 n=6
Check Time 8.07s (± 0.33%) 8.07s (± 0.41%) ~ 8.02s 8.11s p=0.935 n=6
Emit Time 7.04s (± 0.36%) 7.04s (± 0.42%) ~ 7.01s 7.08s p=0.809 n=6
Total Time 18.59s (± 0.11%) 18.59s (± 0.28%) ~ 18.51s 18.65s p=0.872 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,600k (± 1.66%) 191,662k (± 1.24%) ~ 190,652k 196,535k p=0.471 n=6
Parse Time 1.34s (± 0.99%) 1.35s (± 1.15%) ~ 1.34s 1.38s p=0.491 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.15s (± 0.57%) 9.18s (± 0.82%) ~ 9.10s 9.31s p=0.872 n=6
Emit Time 2.64s (± 0.65%) 2.63s (± 0.34%) ~ 2.62s 2.64s p=0.284 n=6
Total Time 13.87s (± 0.41%) 13.89s (± 0.61%) ~ 13.82s 14.04s p=1.000 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,238k (± 0.01%) 347,217k (± 0.01%) ~ 347,193k 347,249k p=0.335 n=6
Parse Time 2.46s (± 0.50%) 2.45s (± 0.31%) ~ 2.44s 2.46s p=0.796 n=6
Bind Time 0.94s (± 0.67%) 0.94s (± 0.43%) ~ 0.94s 0.95s p=0.673 n=6
Check Time 6.86s (± 0.53%) 6.86s (± 0.38%) ~ 6.83s 6.90s p=1.000 n=6
Emit Time 4.01s (± 0.34%) 4.02s (± 0.34%) ~ 4.01s 4.05s p=0.241 n=6
Total Time 14.28s (± 0.24%) 14.28s (± 0.27%) ~ 14.23s 14.34s p=1.000 n=6
TFS - node (v18.15.0, x64)
Memory used 302,550k (± 0.01%) 302,542k (± 0.01%) ~ 302,509k 302,611k p=1.000 n=6
Parse Time 1.98s (± 0.21%) 2.00s (± 1.07%) ~ 1.97s 2.02s p=0.077 n=6
Bind Time 1.01s (± 0.80%) 1.01s (± 1.20%) ~ 0.99s 1.02s p=0.354 n=6
Check Time 6.26s (± 0.45%) 6.26s (± 0.48%) ~ 6.22s 6.30s p=0.935 n=6
Emit Time 3.52s (± 0.66%) 3.55s (± 0.74%) ~ 3.53s 3.59s p=0.090 n=6
Total Time 12.78s (± 0.20%) 12.82s (± 0.50%) ~ 12.74s 12.90s p=0.257 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,501k (± 0.01%) 470,474k (± 0.00%) ~ 470,454k 470,493k p=0.091 n=6
Parse Time 2.57s (± 0.43%) 2.56s (± 0.48%) ~ 2.55s 2.58s p=0.731 n=6
Bind Time 1.00s (± 0.98%) 0.99s (± 0.82%) ~ 0.98s 1.00s p=0.498 n=6
Check Time 16.57s (± 0.33%) 16.64s (± 0.47%) ~ 16.57s 16.79s p=0.147 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.14s (± 0.26%) 20.20s (± 0.36%) ~ 20.12s 20.33s p=0.146 n=6
xstate - node (v18.15.0, x64)
Memory used 512,570k (± 0.02%) 512,595k (± 0.02%) ~ 512,455k 512,686k p=0.630 n=6
Parse Time 3.27s (± 0.36%) 3.27s (± 0.32%) ~ 3.26s 3.29s p=1.000 n=6
Bind Time 1.54s (± 0.35%) 1.55s (± 0.53%) ~ 1.54s 1.56s p=0.859 n=6
Check Time 2.85s (± 0.96%) 2.85s (± 0.97%) ~ 2.81s 2.89s p=0.809 n=6
Emit Time 0.08s (± 6.73%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.174 n=6
Total Time 7.74s (± 0.42%) 7.75s (± 0.35%) ~ 7.71s 7.78s p=0.936 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)
  • xstate - node (v18.15.0, 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 (v18.15.0, x64)
Req 1 - updateOpen 2,371ms (± 1.09%) 2,354ms (± 1.21%) ~ 2,326ms 2,392ms p=0.297 n=6
Req 2 - geterr 5,350ms (± 1.51%) 5,373ms (± 1.78%) ~ 5,278ms 5,473ms p=1.000 n=6
Req 3 - references 326ms (± 0.27%) 327ms (± 0.55%) ~ 325ms 330ms p=0.363 n=6
Req 4 - navto 276ms (± 0.90%) 277ms (± 1.49%) ~ 271ms 282ms p=1.000 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 83ms (± 9.69%) 82ms (±10.23%) ~ 74ms 90ms p=0.557 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,478ms (± 1.45%) 2,470ms (± 0.88%) ~ 2,447ms 2,508ms p=0.810 n=6
Req 2 - geterr 4,104ms (± 1.98%) 4,103ms (± 2.00%) ~ 4,041ms 4,224ms p=0.810 n=6
Req 3 - references 338ms (± 1.52%) 338ms (± 1.49%) ~ 331ms 344ms p=0.935 n=6
Req 4 - navto 284ms (± 0.29%) 284ms (± 0.22%) ~ 283ms 285ms p=0.432 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 80ms (± 7.32%) 80ms (± 7.36%) ~ 75ms 87ms p=1.000 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,594ms (± 0.42%) 2,592ms (± 0.41%) ~ 2,583ms 2,607ms p=0.628 n=6
Req 2 - geterr 1,700ms (± 2.41%) 1,699ms (± 2.41%) ~ 1,633ms 1,737ms p=0.810 n=6
Req 3 - references 119ms (± 8.16%) 112ms (± 7.95%) ~ 105ms 125ms p=0.124 n=6
Req 4 - navto 360ms (± 0.41%) 360ms (± 0.60%) ~ 358ms 364ms p=0.196 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 305ms (± 2.20%) 310ms (± 1.68%) ~ 302ms 316ms p=0.198 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, 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 (v18.15.0, x64)
Execution time 152.06ms (± 0.17%) 152.03ms (± 0.17%) ~ 151.00ms 156.33ms p=0.256 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.87ms (± 0.16%) 227.78ms (± 0.17%) -0.09ms (- 0.04%) 226.46ms 234.23ms p=0.002 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.89ms (± 0.17%) 228.90ms (± 0.19%) ~ 227.09ms 235.33ms p=0.977 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.97ms (± 0.19%) 228.85ms (± 0.15%) -0.12ms (- 0.05%) 227.50ms 232.46ms p=0.011 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

Something interesting changed - please have a look.

Details

apollographql/apollo-client

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

tsconfig.json

discordjs/discord.js

37 of 57 projects failed to build with the old tsc and were ignored

packages/voice/tsconfig.docs.json

packages/voice/tsconfig.json

Eugeny/tabby

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

tabby-community-color-schemes/tsconfig.json

tabby-core/tsconfig.json

tabby-core/tsconfig.typings.json

tabby-electron/tsconfig.json

tabby-linkifier/tsconfig.json

tabby-local/tsconfig.json

tabby-plugin-manager/tsconfig.json

tabby-serial/tsconfig.json

tabby-settings/tsconfig.json

tabby-ssh/tsconfig.json

tabby-telnet/tsconfig.json

tabby-terminal/tsconfig.json

tabby-web-demo/tsconfig.json

tabby-web/tsconfig.json

immich-app/immich

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

server/tsconfig.json

microsoft/vscode

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

src/tsconfig.monaco.json

src/tsconfig.tsec.json

@DanielRosenwasser
Copy link
Member

Oh my

@jakebailey
Copy link
Member

So I guess a lot of people accidentally stumbled on this hole and used it to violate accessible rules?

@Jack-Works
Copy link
Contributor

yeah, this has been repeatedly found as a "bug" but actually is a feature (#42360).

maybe the team should add some comment in the checker, or emit a Message (hint) in IDE that this is violating visibility so people won't repeatedly report/fix this.

@jakebailey
Copy link
Member

Personally, I don't think this is a good hole to have. If someone wants to violate visibility rules, they can as any. I have never thought to try and skip past visibility via this["..."]. The list of breaks isn't that long.

@rubiesonthesky
Copy link

This is really nice way to access class properties in test while, for example, when testing angular components. as any will lose all other type checking and that is in most cases not wanted. The other way is to use ts-ignore but then again you are hiding a lot other mistakes. I understand that you would not want to have this type hole in production code but fixing it will make writing some tests much harder. (Easiest fix is to change component code where property will no longer be private, but that is not desirable in most cases).

I wish there was some legit way to side step private in test files without losing type information. :/

@DanielRosenwasser
Copy link
Member

I don't think we can remove the accessibility checks without a broader discussion. If we want to reduce the scope to just super["..."] for instance property checks, that's fine, but we would need to bring this to a design meeting

@weswigham
Copy link
Member

Personally, I don't think this is a good hole to have.

I don't think we can remove the accessibility checks without a broader discussion.

We could always put it behind a strictness option if we're worried about it being a break - it's pretty in-line with the existing indexed-access relaxing option, suppressImplicitAnyIndexErrors, though admittedly not implicit-any-related, so maybe we shouldn't bundle it into that option specifically.

@sandersn
Copy link
Member

sandersn commented Oct 9, 2023

We could always put it behind a strictness option

If the option joins the --strict family, then we would need some additional way to disable it for just test files, since otherwise-strict code bases might still want to use it for tests. Actually, that would be nice whether or not it's part of the --strict.

@weswigham
Copy link
Member

@DanielRosenwasser didn't we bring this up in a design meeting? Did we come to a conclusion on if we should merge this as-is or flag it as a strictness improvement?

@a-tarasyuk
Copy link
Contributor Author

@DanielRosenwasser @RyanCavanaugh @weswigham @sandersn This PR has been open for a while., should these changes be under the suppressImplicitAnyIndexErrors option, or should the current approach remain unchanged?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 8, 2024

This didn't come up at a design meeting yet - we should discuss it.

That said, the affected test has a comment explaining the behavior:

    // Indexed access expressions have always permitted access to private and protected members.
    // For consistency we also permit such access in indexed access types.

So it probably makes more sense to have under a --strict flag if we're set on changing things. What the flag name is, I don't know.

@a-tarasyuk
Copy link
Contributor Author

This didn't come up at a design meeting yet - we should discuss it.

@DanielRosenwasser thank you for your feedback. I’d appreciate it if you could add it to the design meeting queue when you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexing/element access on super avoids instance property checks
8 participants