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

Allow some per file compiler options #49886

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jul 12, 2022

In this PR, we allow certain compiler options to be configured via comments within a file for the scope of that file using a comment of the form // @ts-option [value]. This means you can write, for example:
compiler-option-pragmas

Currently this supports everything in the "strict type checks" and "additional checks" segments of options, which includes:

  • strict
  • noImplicitAny
  • strictNullChecks
  • strictFunctionTypes
  • strictBindCallApply
  • strictPropertyInitialization
  • noImplicitThis
  • useUnknownInCatchVariables
  • alwaysStrict
  • noUnusedLocals
  • noUnusedParameters
  • exactOptionalPropertyTypes
  • noImplicitReturns
  • noFallthroughCasesInSwitch
  • noUncheckedIndexedAccess
  • noImplicitOverride
  • noPropertyAccessFromIndexSignature

Adding more options to this list is just a matter of adding the allowedAsPragma flag to the definition of the option in commandLineParser.ts and using getFileLocalCompilerOption to look up the option at relevant locations, so we can easily add support for more local options as needed (by far the hardest was strictNullChecks).

Fixes #31035

I've wanted to look into this since I originally added the generic pragma-parsing infrastructure for @ts-check and related comments.

To start off the discussion, in the linked issue, @RyanCavanaugh asked:

It's not really even clear what this means - if you have a type declared in file A, another type in file B, and you try to relate them in file C, whose value of strictFunctionTypes is in play?

Currently in this PR, whichever is stricter. If either file the types originated from has it set, it's related strictly. Could go the other way and relate loosely if either is explicitly non-strict if looser checking is more preferable. Honestly, most of these "which context controls the flag" already have fairly canonical answers, either because we already look up a context to check JS-y-ness (for JS-file-specific-behaviors or error ignoring), or to issue an error in (in which case the error node location is a pretty logical context).

Anyways, I'm hoping this implementation can jumpstart discussion on the value of in-file pragmas for compiler options, specifically with the goal of bikeshedding the syntax and what options we should ultimately expose on a per-file basis. (Technically, I've tried to make the internal APIs for adding new per-file pragmas very type-safe and very easy to use.)

I strongly recommend reviewing this by-commit if possible. Specifically, the changes to strictNullChecks to support this are extensive and are contained in f5134a5.

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 12, 2022
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 12, 2022

Heya @weswigham, I've started to run the tarball bundle task on this PR at 459196b. You can monitor the build here.

@weswigham
Copy link
Member Author

@typescript-bot pack this now that baselines should be green again :)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2022

Heya @weswigham, I've started to run the tarball bundle task on this PR at 97bba40. You can monitor the build here.

@MartinJohns
Copy link
Contributor

Is this opt-in only, or will it be possible to opt-out as well?

@weswigham
Copy link
Member Author

You should be able to write things like // @ts-strict false and the like. Parameters get parsed the same way they do on the command line.

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2022

Heya @weswigham, I've started to run the tarball bundle task on this PR at ef3813c. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2022

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/129711/artifacts?artifactName=tgz&fileId=07F2813C225AA6CC371299076DC3BF08FFE79A5C45BC00165AB9D270F70FC3FB02&fileName=/typescript-4.8.0-insiders.20220713.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

WIP 2

Less WIP - Only exactOptionalPropertyTypes and strictNullChecks left

Pragmas are case-insensitive

Fix harness types

Fixup baselines

File-local support for exactOptionalPropertyTypes

Fix lints

Cache getSourceFileOfNode within the checker
@weswigham weswigham marked this pull request as ready for review August 31, 2022 18:09
@weswigham
Copy link
Member Author

This should be cleaned up and ready to go, so let's get the bot to do some tests on this iteration (not the much has changed implementation-wise since I ran them on the strict null checks-dedicated draft pr).

@typescript-bot pack this
@typescript-bot user test this inline
@typescript-bot test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 31, 2022

Heya @weswigham, I've started to run the extended test suite on this PR at 4bde2e7. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 31, 2022

Heya @weswigham, I've started to run the perf test suite on this PR at 4bde2e7. You can monitor the build here.

Update: The results are in!

@weswigham
Copy link
Member Author

@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 21, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..49886

Metric main 49886 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 339,137k (± 0.01%) 343,109k (± 0.01%) +3,972k (+ 1.17%) 343,079k 343,167k
Parse Time 2.07s (± 0.56%) 2.06s (± 0.57%) -0.01s (- 0.39%) 2.04s 2.09s
Bind Time 0.80s (± 0.70%) 0.80s (± 0.62%) -0.00s (- 0.25%) 0.79s 0.81s
Check Time 5.86s (± 0.41%) 6.36s (± 0.40%) +0.50s (+ 8.55%) 6.29s 6.43s
Emit Time 6.25s (± 0.48%) 6.27s (± 0.74%) +0.02s (+ 0.38%) 6.18s 6.35s
Total Time 14.97s (± 0.28%) 15.49s (± 0.40%) +0.51s (+ 3.43%) 15.30s 15.58s
Compiler-Unions - node (v14.15.1, x64)
Memory used 190,085k (± 0.14%) 194,044k (± 0.01%) +3,960k (+ 2.08%) 193,973k 194,101k
Parse Time 0.86s (± 0.98%) 0.86s (± 0.68%) -0.00s (- 0.23%) 0.85s 0.87s
Bind Time 0.48s (± 1.03%) 0.49s (± 1.01%) +0.00s (+ 0.21%) 0.48s 0.50s
Check Time 6.73s (± 0.34%) 7.29s (± 0.41%) +0.56s (+ 8.26%) 7.23s 7.36s
Emit Time 2.40s (± 0.63%) 2.41s (± 0.85%) +0.01s (+ 0.46%) 2.37s 2.46s
Total Time 10.48s (± 0.25%) 11.05s (± 0.34%) +0.57s (+ 5.44%) 10.99s 11.14s
Monaco - node (v14.15.1, x64)
Memory used 326,573k (± 0.01%) 333,793k (± 0.01%) +7,220k (+ 2.21%) 333,741k 333,831k
Parse Time 1.59s (± 0.79%) 1.58s (± 0.60%) -0.01s (- 0.57%) 1.56s 1.60s
Bind Time 0.73s (± 0.50%) 0.72s (± 0.55%) -0.01s (- 0.69%) 0.71s 0.73s
Check Time 5.71s (± 0.29%) 6.37s (± 0.46%) +0.66s (+11.49%) 6.29s 6.43s
Emit Time 3.35s (± 0.30%) 3.36s (± 0.92%) +0.01s (+ 0.33%) 3.32s 3.47s
Total Time 11.38s (± 0.24%) 12.03s (± 0.47%) +0.65s (+ 5.74%) 11.92s 12.18s
TFS - node (v14.15.1, x64)
Memory used 289,660k (± 0.01%) 295,357k (± 0.01%) +5,697k (+ 1.97%) 295,292k 295,395k
Parse Time 1.29s (± 0.64%) 1.29s (± 0.82%) 0.00s ( 0.00%) 1.28s 1.32s
Bind Time 0.79s (± 0.92%) 0.79s (± 0.78%) -0.00s (- 0.13%) 0.78s 0.81s
Check Time 5.37s (± 0.58%) 5.90s (± 0.45%) +0.53s (+ 9.79%) 5.85s 5.97s
Emit Time 3.60s (± 0.77%) 3.60s (± 0.72%) +0.00s (+ 0.08%) 3.53s 3.64s
Total Time 11.06s (± 0.52%) 11.59s (± 0.44%) +0.53s (+ 4.83%) 11.47s 11.71s
material-ui - node (v14.15.1, x64)
Memory used 436,848k (± 0.00%) 439,061k (± 0.00%) +2,213k (+ 0.51%) 439,018k 439,106k
Parse Time 1.88s (± 0.51%) 1.87s (± 0.47%) -0.01s (- 0.27%) 1.85s 1.89s
Bind Time 0.58s (± 0.96%) 0.58s (± 0.99%) +0.00s (+ 0.35%) 0.57s 0.59s
Check Time 12.94s (± 0.71%) 14.20s (± 0.62%) +1.25s (+ 9.68%) 13.99s 14.35s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.61%) 16.65s (± 0.54%) +1.25s (+ 8.11%) 16.45s 16.80s
xstate - node (v14.15.1, x64)
Memory used 547,672k (± 0.01%) 549,075k (± 0.00%) +1,402k (+ 0.26%) 549,026k 549,112k
Parse Time 2.62s (± 0.48%) 2.62s (± 0.60%) +0.00s (+ 0.11%) 2.60s 2.67s
Bind Time 0.98s (± 1.55%) 0.98s (± 1.23%) -0.01s (- 0.71%) 0.96s 1.00s
Check Time 1.51s (± 0.64%) 1.60s (± 0.46%) +0.09s (+ 5.89%) 1.58s 1.62s
Emit Time 0.07s (± 0.00%) 0.07s (± 3.14%) +0.00s (+ 1.43%) 0.07s 0.08s
Total Time 5.19s (± 0.41%) 5.27s (± 0.45%) +0.08s (+ 1.58%) 5.23s 5.33s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49886 10
Baseline main 10

Developer Information:

Download Benchmark

@weswigham
Copy link
Member Author

@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 21, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..49886

Metric main 49886 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 339,137k (± 0.01%) 343,063k (± 0.01%) +3,926k (+ 1.16%) 343,018k 343,104k
Parse Time 2.07s (± 0.56%) 2.08s (± 1.17%) +0.02s (+ 0.73%) 2.05s 2.17s
Bind Time 0.80s (± 0.70%) 0.80s (± 0.59%) +0.00s (+ 0.25%) 0.79s 0.81s
Check Time 5.86s (± 0.41%) 6.38s (± 0.34%) +0.53s (+ 8.98%) 6.34s 6.43s
Emit Time 6.25s (± 0.48%) 6.30s (± 0.74%) +0.05s (+ 0.80%) 6.21s 6.43s
Total Time 14.97s (± 0.28%) 15.56s (± 0.49%) +0.59s (+ 3.94%) 15.45s 15.76s
Compiler-Unions - node (v14.15.1, x64)
Memory used 190,085k (± 0.14%) 193,913k (± 0.13%) +3,828k (+ 2.01%) 192,934k 194,067k
Parse Time 0.86s (± 0.98%) 0.86s (± 0.65%) -0.00s (- 0.12%) 0.85s 0.87s
Bind Time 0.48s (± 1.03%) 0.49s (± 0.61%) +0.00s (+ 0.62%) 0.48s 0.49s
Check Time 6.73s (± 0.34%) 7.32s (± 0.45%) +0.59s (+ 8.70%) 7.21s 7.39s
Emit Time 2.40s (± 0.63%) 2.41s (± 0.88%) +0.01s (+ 0.33%) 2.37s 2.48s
Total Time 10.48s (± 0.25%) 11.07s (± 0.40%) +0.60s (+ 5.69%) 10.95s 11.17s
Monaco - node (v14.15.1, x64)
Memory used 326,573k (± 0.01%) 333,806k (± 0.01%) +7,233k (+ 2.21%) 333,761k 333,858k
Parse Time 1.59s (± 0.79%) 1.59s (± 0.84%) +0.01s (+ 0.57%) 1.56s 1.63s
Bind Time 0.73s (± 0.50%) 0.73s (± 0.82%) 0.00s ( 0.00%) 0.71s 0.74s
Check Time 5.71s (± 0.29%) 6.37s (± 0.42%) +0.66s (+11.56%) 6.31s 6.43s
Emit Time 3.35s (± 0.30%) 3.38s (± 1.04%) +0.03s (+ 0.84%) 3.33s 3.50s
Total Time 11.38s (± 0.24%) 12.07s (± 0.44%) +0.70s (+ 6.12%) 11.97s 12.24s
TFS - node (v14.15.1, x64)
Memory used 289,660k (± 0.01%) 295,349k (± 0.01%) +5,689k (+ 1.96%) 295,298k 295,389k
Parse Time 1.29s (± 0.64%) 1.30s (± 0.43%) +0.00s (+ 0.31%) 1.28s 1.31s
Bind Time 0.79s (± 0.92%) 0.79s (± 0.75%) +0.00s (+ 0.25%) 0.78s 0.81s
Check Time 5.37s (± 0.58%) 5.90s (± 0.38%) +0.53s (+ 9.79%) 5.84s 5.94s
Emit Time 3.60s (± 0.77%) 3.60s (± 0.53%) +0.00s (+ 0.03%) 3.57s 3.65s
Total Time 11.06s (± 0.52%) 11.60s (± 0.18%) +0.54s (+ 4.87%) 11.54s 11.64s
material-ui - node (v14.15.1, x64)
Memory used 436,848k (± 0.00%) 439,032k (± 0.01%) +2,184k (+ 0.50%) 438,969k 439,073k
Parse Time 1.88s (± 0.51%) 1.88s (± 0.64%) +0.01s (+ 0.43%) 1.85s 1.91s
Bind Time 0.58s (± 0.96%) 0.58s (± 0.81%) +0.00s (+ 0.35%) 0.57s 0.59s
Check Time 12.94s (± 0.71%) 14.24s (± 0.48%) +1.29s (+ 9.97%) 14.10s 14.38s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.61%) 16.70s (± 0.44%) +1.30s (+ 8.46%) 16.54s 16.86s
xstate - node (v14.15.1, x64)
Memory used 547,672k (± 0.01%) 549,070k (± 0.00%) +1,398k (+ 0.26%) 549,029k 549,114k
Parse Time 2.62s (± 0.48%) 2.63s (± 0.63%) +0.01s (+ 0.50%) 2.58s 2.66s
Bind Time 0.98s (± 1.55%) 0.99s (± 1.20%) +0.00s (+ 0.41%) 0.96s 1.02s
Check Time 1.51s (± 0.64%) 1.61s (± 0.68%) +0.10s (+ 6.55%) 1.59s 1.63s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.19s (± 0.41%) 5.30s (± 0.49%) +0.11s (+ 2.04%) 5.22s 5.35s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49886 10
Baseline main 10

Developer Information:

Download Benchmark

Copy link

@neolectron neolectron left a comment

Choose a reason for hiding this comment

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

lgtm

@abdatta
Copy link

abdatta commented Jan 10, 2023

Hi, any updates on this? Now that it is approved, can we expect it to be merged anytime soon?

@abdatta
Copy link

abdatta commented Jan 11, 2023

@typescript-bot pack this

@abdatta
Copy link

abdatta commented Jan 13, 2023

Hey @weswigham, if you don't mind, can you please ask the @typescript-bot to pack this latest iteration? It seems to listen to the PR author. I wanted to try it out in some local project. Thanks!

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 13, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5b77328. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 13, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/142539/artifacts?artifactName=tgz&fileId=84F197BAEF8A6E106505CC1E9EE470B6D941DF8AA3E6B9D2787B60D6817BEDF002&fileName=/typescript-4.9.0-insiders.20230113.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@abdatta
Copy link

abdatta commented Jan 13, 2023

Thanks @jakebailey!

@xiaoxiangmoe
Copy link
Contributor

There is only a week before ts 5.0 beta. Maybe this feature will be avaliable after 5.1

@abdatta
Copy link

abdatta commented Jan 15, 2023

I'm seeing these kinds of errors despite turning off the check. Is this expected? Ideally it should not be imo as turning off SNC from tsconfig would not generate this error.

image

Playground Link

@eyelidlessness
Copy link

I was writing up a much longer background for this, but I’m exhausted on Saturday, and don’t want to lose the context before I bail. I’m happy to come back and fill in details of why I’m asking if it seems really bonkers to ask.

Is there any possibility this could allow specifying:

  • tsconfig path rather than individual config items
  • a range of lines it applies to versus a whole module

The use case prompting me to ask involves using Playwright as a library, and would incredibly simplify its usage (and type safety) for a single function proxy as I transition a library from dependence on Node APIs and native dependencies to be usable in browser. It’s definitely not a common use case, but I can imagine others which might benefit from specifying a given callback is being shuttled off to be executed in an entirely different environment.

@DanielRosenwasser
Copy link
Member

Hey all, the future of this feature is somewhat uncertain. Any time the type system has to make a decision based on strictness, it now has to walk up to the top of the file and check for strictness directives. If not that, every node (or almost every node) of our syntax trees has to get a little bit bigger to hold onto that information.

This has a type-checking performance impact that is higher than we can accept. There might be some optimizations we can make for the more common case of no file-specific options within a project, but that leads to some unpredictable performance cliffs as soon as one is added.

So this won't make its way into TypeScript 5.0, and it's unclear if we'll be able to bring it in in the future either.

@debadree25
Copy link

Hey all, the future of this feature is somewhat uncertain. Any time the type system has to make a decision based on strictness, it now has to walk up to the top of the file and check for strictness directives. If not that, every node (or almost every node) of our syntax trees has to get a little bit bigger to hold onto that information.

This has a type-checking performance impact that is higher than we can accept. There might be some optimizations we can make for the more common case of no file-specific options within a project, but that leads to some unpredictable performance cliffs as soon as one is added.

So this won't make its way into TypeScript 5.0, and it's unclear if we'll be able to bring it in in the future either.

Can this option be not added as an opt in? like an option in compilerOptions something like "enableIncrementalStrictCheckComments" or something? this can be an incredibly helpful option I think most people would be happy to accept a performance penalty to allow for incremental migration to safer and better code.

I came across this issue searching for a way to enable incremental strict checks on a 50k+ line codebase on which enabling strict checks throws up 3k+ errors 🙈 while i understand plugins exist for this having this on a compiler level can be so much better!

I know no nothing about typescripts development process/priorities so if anything i said here is wrong/doesnt make sense I apologise but I sincerely hope you consider the request!

Thank you to the author and typescript team for their time and effort on this!

@weswigham weswigham added the Experiment A fork with an experimental idea which might not make it into master label Aug 13, 2024
@weswigham weswigham marked this pull request as draft August 13, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow per file compilerOptions