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

Add noCheck API option #57934

Merged
merged 23 commits into from
Apr 26, 2024
Merged

Add noCheck API option #57934

merged 23 commits into from
Apr 26, 2024

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Mar 25, 2024

This PR adds noCheck, a compiler option which instructs the compiler to skip the semantic diagnostics phase. This means the compiler skips right form program construction, parsing, and binding, to emit. Implementation-wise, the basically just considers every file the same way we do declaration files under skipLibCheck, or project reference redirects.

As-is, there are some limitations on this - declaration emit works wonderfully already - every test with declaration emit on also tests declaration emit with noCheck, while JS emit needs some functionality disentangled from the checker's full-tree-error-walk to consistently work with this. As such, noCheck requires emitDeclarationOnly for now. (This limitation could be lifted with some additional work to make a lazy version of the .referenced node links calculations in checker, and by making lazy versions of all the calculations implied by the getNodeCheckFlags calls in the JS transforms - which would be required to do #50699 and for this to fulfill #29651) Additionally, passing noCheck alongside noEmit is an error - there's nothing wrong with this combination of options, it just seems like it doesn't do anything.

noCheck is not currently exposed on the commandline or to tsconfig files, and is an internal API, as there was some concern at the design meeting that it might need to change a bit when we add support for JS emit to it.

weswigham and others added 4 commits March 23, 2024 22:42
…sentagling NodeCheckFlags and .referenced from the checkers full traversal)
…king, accept baselines with extra output due to less errors, or union order changes
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@@ -9038,6 +9038,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function serializeSymbol(symbol: Symbol, isPrivate: boolean, propertyAsAlias: boolean): void {
void getPropertiesOfType(getTypeOfSymbol(symbol)); // resolve symbol's type and properties, which should trigger any required merges
Copy link
Member Author

@weswigham weswigham Mar 25, 2024

Choose a reason for hiding this comment

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

The exhaustive testing in the harness revealed that some JS declaration emit cases were a bit broken with out-of-order declaration emit (and were emitting declarations twice) - these two changes fix them by ensuring the checker work to unify weird JS merges is always done before we look at the symbol's identity (so the merge symbol should always exist if it will exist).

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 25, 2024

skipDefaultLibCheck
skipLibCheck
skipCheck

@weswigham
Copy link
Member Author

But //@ts-nocheck and --noEmit 🤔

@sheetalkamat
Copy link
Member

sheetalkamat commented Mar 25, 2024

This is not really --no-check its just dont report diagnostics right? Because getEmitResolver still checks sourceFile and ensures that file is typechecked

@weswigham
Copy link
Member Author

This is not really --no-check its just dont report diagnostics right? Because getEmitResolver still checks sourceFile and ensures that file is typechecked

It doesn't request any diagnostics from the checker, so it skips the whole exhaustive check traversal - on our own codebase this skips basically all checker time - the bits of the checker declaration emit actually pulls on to produce output are minimal. In theory, yes, it could cascade into a complete program type check. In practice, it never does, for the same reason completions basically never results in a whole program typecheck - the checker's lazy.

@weswigham
Copy link
Member Author

Specifically: The skipTypeChecking function is used inside checkSourceFile, so even though createEmitResolver tries to call getDiagnostics, with noCheck on, it does nothing~

@weswigham weswigham changed the title Add --noCheck Add noCheck API option Mar 27, 2024
if (!options.emitDeclarationOnly) {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "noCheck", "emitDeclarationOnly");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

for now we should disable this for incremental as the build info generated will not handle this correctly.

Copy link
Member

Choose a reason for hiding this comment

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

or we need to make sure noCheck recalculates semantic diagnostics and stores the that flag into buildinfo

Copy link
Member Author

Choose a reason for hiding this comment

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

As in I should add an error for using noCheck alongside incremental?

Copy link
Member Author

Choose a reason for hiding this comment

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

or we need to make sure noCheck recalculates semantic diagnostics and stores the that flag into buildinfo

That's just setting affectsSemanticDiagnostics in the option descriptor when we add it back to being enabled for cli/tsconfig in the future, right?

Copy link
Member

Choose a reason for hiding this comment

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

"in option" yes but now it needs to be handled in builder or reported as error to have incremental and nocheck as buildinfo written will not be correct. It will show no errors and next time you run tsc it will not report errors . (without storing noCheck value in buildInfo and checking for that in builder)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is 051a1d1 along the lines of what you were thinking? Since it's not in the commonOptionsWithBuild, commandOptionsWithoutBuild, or optionDeclarations lists, it's never included in help or valid on the CLI/in a config, but since it's built into the OptionsNameMap, all the normal option serializing/deserializing machinery works with it - build info included.

Copy link
Member

Choose a reason for hiding this comment

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

We need test cases:

  1. Run through API with --no-check , followed by tsc .(with and without incremental).
  2. Run through API with --no-check , followed by tsc --b (with and without incremental) - have a feeling that tsc - in non incremental is going to find that everything is uptodate (there is no buildinfo to check, only output file stamps) and will not report errors?

You can add these testcases by using verifyTsc where the initial fs is generated through running API with --no-check

Copy link
Member Author

Choose a reason for hiding this comment

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

Run through API with --no-check

We.... don't have the harness for this right now. At least not a harness that then feeds into the existing verifyTsc stuff (obviously, all the compiler-style tests build against the API without using the executeCommandLine entrypoint). This is quite a lot to put together for something that's only supposed to be API-only temporarily... I started putting it together, and such a harness is looking like it's going to be more than the entire rest of the PR. 😵

So I'm not going to. This is gonna be a normal command line option, accepted on both the CLI and via tsconfig, except with a

        extraValidation(value) {
            return [Diagnostics.Unknown_compiler_option_0, "noCheck"];
        },

so anything that runs validation (API methods that take CompilerOptions obviously do not) reports that it doesn't exist, but otherwise, it uses all the usual pipelines for things. And then in the test where I'd like to consider it a real option, I'll override that validator with a noop temporarily, which I can do for the tsc -b style tests, to validate it will work correctly with -b once it's actually public and usable via.... anything other than the API.

PR Backlog automation moved this from Not started to Needs merge Apr 11, 2024
@@ -4357,6 +4357,15 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
}
}

if (options.noCheck) {
if (options.noEmit) {
Copy link
Member

Choose a reason for hiding this comment

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

i think noEmitOnError and noCheck should also be combo that does not make sense.

Copy link
Member Author

@weswigham weswigham Apr 12, 2024

Choose a reason for hiding this comment

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

noEmitOnError blocks emit on the presence of options, syntactic, global, semantic, and declaration diagnostics. noCheck only blocks semantic from being generated, so noEmitOnError does still have meaning alongside noCheck, though it certainly will block emit less.

loadProjectFromFiles({
"/src/a.ts": getATsContent(declAText),
"/src/tsconfig.json": jsonToReadableText({
compilerOptions: { noCheck: true, emitDeclarationOnly: true, declaration: true },
Copy link
Member

Choose a reason for hiding this comment

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

Having this in json and not reporting error is a bug because then it doesnt become API only option per design meeting.,

The scenario i am talking is missed here because:
modifying tsconfig.json will check that config file is newer than output and hence program will be rebuilt. The problem with doing this with API is that output will be updated but not inputs which means the build will report that everything is uptodate

Here is how to write this test case
1 load project files (without noCheck option) in your test case as is . No errors
2. As part of first edit: Do these steps to emulate API usage: Change a.ts to introduce error, run API (by creating program and emitting with noCheck on, use sys= new vfs.sys(fs)) to create compiler host
3. This will fail because after edit, test framework will run tsc --build again and it will find that everything is UpToDate and not report error. (test will not create discrepancy baseline because unfortunately we don't check discrepancies in error reporting just file emit validation)

Copy link
Member Author

Choose a reason for hiding this comment

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

Having this in json and not reporting error is a bug because then it doesnt become API only option per design meeting.,

And it does normally report an error, which is what the other test block that doesn't override the validator function verifies (and naturally, these collapse into one test once it's actually exposed via CLI)~

The problem with doing this with API is that output will be updated but not inputs which means the build will report that everything is uptodate

If you build via API and pass options not in your on-disk config file (as you would need to, since noCheck is API only right now), your up-to-dateness checks for configs are going to all be invalidated anyway, since your API-fueled build will have different options applied than a commandline one. Precisely

The problem with doing this with API is that output will be updated but not inputs which means the build will report that everything is uptodate

is in no way unique to this new flag - any time you do a API build with different flags than what your on-disk configs specify this'll happen, and I'm pretty sure how -b handles that is just entirely out of scope, unless we're talking specifically incremental output, which just comes down to the options serialized in the .buildinfo which is why:

This will fail because after edit, test framework will run tsc --build again and it will find that everything is UpToDate and not report error. (test will not create discrepancy baseline because unfortunately we don't check discrepancies in error reporting just file emit validation)

It both will and won't. If --incremental isn't on, yeah, it won't build again because all the timestamps say it's newer, and build doesn't know that your last build was with different settings. Again, that's pretty out-of-scope and isn't going to get fixed, since there's no incremental artifact. Nothing new there, that issue has nothing to do with noCheck - you can replicate that with any semantic diagnostic affecting option. For --incremental, that won't be the case, because the .buildinfo emitted by the API build will have different compiler options than the new build (semantic diagnostic affecting ones), which'll force a recheck (though not reemit), and is precisely what this test verifies without needing all the API-layering hoopla by temporarily enabling it in configs/cli (since an incremental build via config is going to produce the same buildinfo as one via api). AFAIK, the only critical thing to test is that noCheck is serialized into the buildinfo just like every other semantic diagnostic-affecting option, when it's allowed, which this test verifies.

As an aside "via API" is ambiguous, since we expose executeCommandLine now, but I've taken that to mean "only allowed where we take CompilerOptions-type objects" for now, which just means any API that's post-option-parsing/validating. Absolutely none of those APIs (eg, createProgram) are even going to emit a .buildinfo file on their own without extra instrumentation on behalf of the caller of the API, so it is quite literally up to the API user to keep things up-to-date, provided our emitBuildInfo function does serialize the option for them when they call it to do that, which, again, this test does verify.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with writing this test case this way is that it is modifying timestamp of tsconfig files and hence build will take place. To check this correctly, you want to use API emit as edit as mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or I could allow --noCheck as a build option (why not? seems like a reasonable thing to want to turn on/off for the whole project graph) and pass it down from the top-level CLI invocation, rather than by editing the tsconfig?

PR Backlog automation moved this from Needs merge to Waiting on author Apr 12, 2024
else if (!canCopySemanticDiagnostics) {
// No emit being done, but semantic diagnostics must be recalculated - since these (and the options that change these)
// are in the build info file, we need to emit it to keep the info on-disk up-to-date
state.buildInfoEmitPending = true;
Copy link
Member

Choose a reason for hiding this comment

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

This should be set only if (!outputFile)

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned somewhere below, you want this condition to be useOldState && !!oldState!.compilerOptions.noCheck !== !!compilerOptions.noCheck

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no clue why this would be noCheck-specific, though? noCheck seems to me to be no different from any other compiler option that varies the errors we produce.

@@ -848,8 +848,8 @@ export function emitFiles(resolver: EmitResolver, host: EmitHost, targetSourceFi
const filesForEmit = forceDtsEmit ? sourceFiles : filter(sourceFiles, isSourceFileNotJson);
// Setup and perform the transformation to retrieve declarations from the input files
const inputListOrBundle = compilerOptions.outFile ? [factory.createBundle(filesForEmit)] : filesForEmit;
if (emitOnly && !getEmitDeclarations(compilerOptions)) {
// Checker wont collect the linked aliases since thats only done when declaration is enabled.
if ((emitOnly && !getEmitDeclarations(compilerOptions)) || compilerOptions.noCheck) {
Copy link
Member

Choose a reason for hiding this comment

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

Also need to add forceDtsEmit check here? shouldnt signature generation behave like noCheck?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK it already does - the || compilerOptions.noCheck being tacked on here is so that noCheck builds do the same data collection signature generation runs already do.

@@ -9938,6 +9938,7 @@ export function skipTypeChecking(sourceFile: SourceFile, options: CompilerOption
// '/// <reference no-default-lib="true"/>' directive.
return (options.skipLibCheck && sourceFile.isDeclarationFile ||
options.skipDefaultLibCheck && sourceFile.hasNoDefaultLib) ||
options.noCheck ||
Copy link
Member

Choose a reason for hiding this comment

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

Adding this here also skips grammar checks .. May be i am remembering it incorrectly but i thought we wanted to do grammar checks ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this here also skips grammar checks

Not... quite. More in a bit.

May be i am remembering it incorrectly but i thought we wanted to do grammar checks ?

I wanna say "no". We ruminated on it briefly, but "grammar checks" aren't a defined subset of the diagnostics we publicly recognize (and some things you'd think are in that category are emitted in the parser, binder, or checker passes over the AST - roll a d6 to find out where), so while are somewhat well-defined in ECMA-262, are incredibly poorly defined for us. Additionally, what diagnostics we skip vs emit isn't in any way what's important for this flag, what is is that we skip the checker's full tree walk (and all the work it does) for diagnostics and jump right to emit (which is what inclusion in this function does). Doing a walk specifically for grammar errors would run pretty counter to that goal, though I admit if you're in a checker context and already traversing an AST most grammar-style errors are usually trivial to emit. Again, not terribly important for this, though - we just want to skip the big walk that does all the expensive work. (Usually so it can be done asynchronously.)

@@ -115,127 +115,4 @@ IncrementalBuild:
]
},
"version": "FakeTSVersion"
}
TsBuild info text without affectedFilesPendingEmit:: /src/project2/src/tsconfig.tsbuildinfo.readable.baseline.txt::
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrectly emitting buildInfo because "emitDeclarationsOnly" does not affect semantic diagnostics. The condition in builder to check for this is:

 const canCopySemanticDiagnostics = useOldState && oldState!.semanticDiagnosticsPerFile && !!state.semanticDiagnosticsPerFile &&
        !compilerOptionsAffectSemanticDiagnostics(compilerOptions, oldCompilerOptions!);

where as you want it to be compilerOptionsAffectSemanticDiagnostics(compilerOptions, oldCompilerOptions!); when emitting buildInfo again.

I think you just want to check if "noCheck" is changed and then mark the buildInfo emit pending.

loadProjectFromFiles({
"/src/a.ts": getATsContent(declAText),
"/src/tsconfig.json": jsonToReadableText({
compilerOptions: { noCheck: true, emitDeclarationOnly: true, declaration: true },
Copy link
Member

Choose a reason for hiding this comment

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

The problem with writing this test case this way is that it is modifying timestamp of tsconfig files and hence build will take place. To check this correctly, you want to use API emit as edit as mentioned above.

@weswigham
Copy link
Member Author

Per our last design meeting discussion on this, this PR enables noCheck for transpileDeclaration by default. On this workstation, without noCheck, a simple transpileDeclaration on each file in our src directory (single thread, I/O time not included) takes 27144ms, with noCheck, that is cut to 8404ms. A substantial improvement, considering semantic diagnostics aren't even available via the transpile APIs!

@weswigham
Copy link
Member Author

@sheetalkamat Since the option isn't available on the CLI yet, nor does it have any special behavior W.R.T. build mode compared to any other semantic diagnostics, I'm not sure there's much to do in this PR. As we said at the last design meeting we likely want to clean up how build mode handles changing flags that aren't from tsconfig files before we make this available via CLI (since a primary usecase is toggling it for a whole project graph), but that's not something that's going to happen in this PR, nor likely for 5.5. I could revert the builder.ts change here just so we can keep all that cleanup together, if you'd like.

@robpalme
Copy link

The new baselines show that enabling noCheck can re-order union items compared to checked declaration emit. Do we know of any other declaration emit differences?




!!!! File all.d.ts missing from original emit, but present in noCheck emit
Copy link
Member

Choose a reason for hiding this comment

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

In general, how are we ending up with these messages? Are they all cases where we don't emit due to errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, a checker error blocks the .d.ts emit without noCheck on.

*** Supplied discrepancy explanation but didnt file any difference
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it needs fixing?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, there are other files with this (and other threads), forgot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the small builder change in this PR - we can look into it separately. The issue there isn't related to the new noCheck flag, but rather applies to any semantic-diagnostic-affecting option you build-via-api with a different setting than you build-via-cli with, which, as we discussed, needs some more extensive builder changes to properly support (and to support passing builder-wide semantic settings via CLI generally).

@weswigham
Copy link
Member Author

The new baselines show that enabling noCheck can re-order union items compared to checked declaration emit. Do we know of any other declaration emit differences?

Yeah, this can happen in incremental mode, too. It's a pretty well-known issue that we haven't found a great way to fix, but usually doesn't have semantic implications. Other stuff that depends on check order is regarded as a bug, since it could make diagnostics in the editor inconsistent.

affectsSemanticDiagnostics: true,
affectsBuildInfo: true,
extraValidation() {
return [Diagnostics.Unknown_compiler_option_0, "noCheck"];
Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone reading, this validation is what blocks this option from being passed on the CLI. This is a nicer way to do it than our usual method (just adding an entry to the CompilerOptions interface), since it still provides metadata for .buildinfo serialization and showConfig, plus makes it easier to test as though it were CLI-allowed, like is done in the noCheck unittest.

@sheetalkamat
Copy link
Member

@weswigham Will review soon probably tonight or tomrorow morning, but i think we need to make sure buildInfo is not written when "noCheck" is on accidently. This can be done by changing "getTsBuildInfoEmitOutputFilePath" to return undefined if noCheck is true.

@weswigham
Copy link
Member Author

weswigham commented Apr 24, 2024

but i think we need to make sure buildInfo is not written when "noCheck" is on accidently. This can be done by changing "getTsBuildInfoEmitOutputFilePath" to return undefined if noCheck is true.

noCheck isn't any different from any other option that affects checker diagnostics; I don't see why we'd do a one-off fix for it that doesn't apply to every other semantic-diagnostic-affecting option. noCheck isn't in any way special, other than it always adding an option parsing diagnostic when you pass it through the command line helper (which is also what any non-extant flag would do).

@weswigham
Copy link
Member Author

For additional context, allowNonTsExtensions and suppressOutputPathCheck are other internal-only compiler option that definitely affect diagnostics, which we don't do any special handling for at all. I assume our approach for internal only options is "use at your own risk", since it's not like they can be passed through normal build/cli/config pipelines.

And noCheck shouldn't even be internal only for long. 🤷‍♂️

@weswigham
Copy link
Member Author

Per out-of-band discussions, I've opened #58336 to formally track future work with --build and --noCheck to help clarify the long-term goals for this. As-is, I'm going to merge this, and any other changes we want to make can be taken as follow-ups.

@weswigham weswigham enabled auto-merge (squash) April 26, 2024 20:18
@weswigham weswigham disabled auto-merge April 26, 2024 20:21
@weswigham weswigham merged commit f76727d into microsoft:main Apr 26, 2024
25 checks passed
PR Backlog automation moved this from Waiting on author to Done Apr 26, 2024
@zanminkian
Copy link

Will this land in 5.5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants