-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Add noCheck API option
#57934
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
Changes from 4 commits
79961d4
540ce28
3f89f5c
bf82544
a3d4f73
ce65c79
736eac2
5c5e8e0
b33afe7
1a05d96
051a1d1
1231be1
3e29a42
2bda238
7332613
41f82f1
5c9c333
72bd93c
e0d7b4e
8c667f4
e0dfe56
2f65540
0f1b4c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -845,8 +845,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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also need to add forceDtsEmit check here? shouldnt signature generation behave like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK it already does - the |
||
| // Checker wont collect the linked aliases since thats only done when declaration is enabled and checking is performed. | ||
| // Do that here when emitting only dts files | ||
| filesForEmit.forEach(collectLinkedAliases); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4347,6 +4347,15 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg | |
| } | ||
| } | ||
|
|
||
| if (options.noCheck) { | ||
| if (options.noEmit) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "noCheck", "noEmit"); | ||
| } | ||
| if (!options.emitDeclarationOnly) { | ||
| createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "noCheck", "emitDeclarationOnly"); | ||
| } | ||
jakebailey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in I should add an error for using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's just setting
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need test cases:
You can add these testcases by using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We.... don't have the harness for this right now. At least not a harness that then feeds into the existing So I'm not going to. This is gonna be a normal command line option, accepted on both the CLI and via extraValidation(value) {
return [Diagnostics.Unknown_compiler_option_0, "noCheck"];
},so anything that runs validation (API methods that take |
||
|
|
||
| if ( | ||
| options.emitDecoratorMetadata && | ||
| !options.experimentalDecorators | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9858,6 +9858,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 || | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not... quite. More in a bit.
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.) |
||
| host.isSourceOfProjectReferenceRedirect(sourceFile.fileName); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "compilerOptions": { | ||
| "noCheck": true | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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).