-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 all 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 |
---|---|---|
|
@@ -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) { | ||
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 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 |
---|---|---|
|
@@ -4424,6 +4424,15 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg | |
} | ||
} | ||
|
||
if (options.noCheck) { | ||
if (options.noEmit) { | ||
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 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
|
||
} | ||
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. 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 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 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 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) 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 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 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 |
---|---|---|
|
@@ -9967,6 +9967,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 || | ||
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 ? 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,86 @@ | ||
import { | ||
CommandLineOption, | ||
optionDeclarations, | ||
} from "../../_namespaces/ts"; | ||
import { jsonToReadableText } from "../helpers"; | ||
import { | ||
noChangeRun, | ||
verifyTsc, | ||
} from "../helpers/tsc"; | ||
import { loadProjectFromFiles } from "../helpers/vfs"; | ||
|
||
function verifyNoCheckFlag(variant: string) { | ||
function verifyNoCheckWorker(subScenario: string, declAText: string, commandLineArgs: readonly string[]) { | ||
verifyTsc({ | ||
scenario: variant, | ||
subScenario, | ||
fs: () => | ||
loadProjectFromFiles({ | ||
"/src/a.ts": getATsContent(declAText), | ||
"/src/tsconfig.json": jsonToReadableText({ | ||
compilerOptions: { noCheck: true, emitDeclarationOnly: true, declaration: true }, | ||
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. 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: Here is how to write this test case 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.
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)~
If you build via API and pass options not in your on-disk config file (as you would need to, since
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
It both will and won't. If As an aside "via API" is ambiguous, since we expose 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. 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 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 I could allow |
||
}), | ||
}), | ||
commandLineArgs, | ||
edits: [ | ||
noChangeRun, | ||
{ | ||
caption: "Fix `a` error", | ||
edit: fs => fs.writeFileSync("/src/a.ts", getATsContent(`const a = "hello"`)), | ||
}, | ||
noChangeRun, | ||
{ | ||
caption: "Disable noCheck", | ||
edit: fs => | ||
fs.writeFileSync( | ||
"/src/tsconfig.json", | ||
jsonToReadableText({ | ||
compilerOptions: { emitDeclarationOnly: true, declaration: true }, | ||
}), | ||
), | ||
}, | ||
noChangeRun, | ||
], | ||
baselinePrograms: true, | ||
}); | ||
|
||
function getATsContent(declAText: string) { | ||
return `const err: number = "error"; | ||
${declAText}`; | ||
} | ||
} | ||
|
||
function verifyNoCheck(subScenario: string, aTsContent: string) { | ||
verifyNoCheckWorker(subScenario, aTsContent, ["--b", "/src/tsconfig.json", "-v"]); | ||
verifyNoCheckWorker(`${subScenario} with incremental`, aTsContent, ["--b", "/src/tsconfig.json", "-v", "--incremental"]); | ||
} | ||
|
||
verifyNoCheck("syntax errors", `const a = "hello`); | ||
verifyNoCheck("semantic errors", `const a: number = "hello"`); | ||
} | ||
|
||
describe("unittests:: tsbuild:: noCheck", () => { | ||
// Enable the `noCheck` option on the CLI for testing purposes, to ensure it works with incremental/build | ||
let validate: CommandLineOption["extraValidation"]; | ||
before(() => { | ||
for (const opt of optionDeclarations) { | ||
if (opt.name === "noCheck") { | ||
validate = opt.extraValidation; | ||
opt.extraValidation = () => undefined; | ||
} | ||
} | ||
}); | ||
after(() => { | ||
for (const opt of optionDeclarations) { | ||
if (opt.name === "noCheck") { | ||
opt.extraValidation = validate; | ||
} | ||
} | ||
}); | ||
|
||
verifyNoCheckFlag("noCheck"); | ||
}); | ||
|
||
describe("unittests:: tsbuild:: noCheck:: errors", () => { | ||
verifyNoCheckFlag("noCheck-errors"); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"compilerOptions": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
//// [tests/cases/compiler/noCheckDoesNotReportError.ts] //// | ||
|
||
//// [noCheckDoesNotReportError.ts] | ||
export const a: number = "not ok"; | ||
|
||
|
||
|
||
|
||
//// [noCheckDoesNotReportError.d.ts] | ||
export declare const a: number; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
//// [tests/cases/compiler/noCheckDoesNotReportError.ts] //// | ||
|
||
=== noCheckDoesNotReportError.ts === | ||
export const a: number = "not ok"; | ||
>a : Symbol(a, Decl(noCheckDoesNotReportError.ts, 0, 12)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
//// [tests/cases/compiler/noCheckDoesNotReportError.ts] //// | ||
|
||
=== noCheckDoesNotReportError.ts === | ||
export const a: number = "not ok"; | ||
>a : number | ||
> : ^^^^^^ | ||
>"not ok" : "not ok" | ||
> : ^^^^^^^^ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
error TS5053: Option 'emitDeclarationOnly' cannot be specified with option 'noEmit'. | ||
error TS5053: Option 'noCheck' cannot be specified with option 'noEmit'. | ||
|
||
|
||
!!! error TS5053: Option 'emitDeclarationOnly' cannot be specified with option 'noEmit'. | ||
!!! error TS5053: Option 'noCheck' cannot be specified with option 'noEmit'. | ||
==== noCheckNoEmit.ts (0 errors) ==== | ||
export const a: number = "not ok"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
//// [tests/cases/compiler/noCheckNoEmit.ts] //// | ||
|
||
=== noCheckNoEmit.ts === | ||
export const a: number = "not ok"; | ||
>a : Symbol(a, Decl(noCheckNoEmit.ts, 0, 12)) | ||
|
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.
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 andshowConfig
, plus makes it easier to test as though it were CLI-allowed, like is done in thenoCheck
unittest.