-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add noCheck
API option
#57934
Conversation
…/declarations without full checking
…sentagling NodeCheckFlags and .referenced from the checkers full traversal)
…king, accept baselines with extra output due to less errors, or union order changes
…ly, refine noCheck test conditions to match
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. |
src/compiler/checker.ts
Outdated
@@ -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 |
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).
|
But |
This is not really |
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. |
Specifically: The |
if (!options.emitDeclarationOnly) { | ||
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "noCheck", "emitDeclarationOnly"); | ||
} | ||
} |
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 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 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
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.
As in I should add an error for using noCheck
alongside incremental
?
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.
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?
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.
"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 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.
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.
We need test cases:
- Run through API with
--no-check
, followed bytsc
.(with and without incremental). - Run through API with
--no-check
, followed bytsc --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
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.
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.
…`noCheck`, it is serialized correctly
@@ -4357,6 +4357,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 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.
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.
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 }, |
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.
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)
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.
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.
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 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.
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.
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
?
src/compiler/builder.ts
Outdated
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; |
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.
This should be set only if (!outputFile)
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.
As mentioned somewhere below, you want this condition to be useOldState && !!oldState!.compilerOptions.noCheck !== !!compilerOptions.noCheck
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.
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) { |
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.
Also need to add forceDtsEmit check here? shouldnt signature generation behave like noCheck
?
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.
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 || |
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.
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 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:: |
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.
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 }, |
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 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.
Per our last design meeting discussion on this, this PR enables |
@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. |
The new baselines show that enabling |
|
||
|
||
|
||
!!!! File all.d.ts missing from original emit, but present in noCheck emit |
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.
In general, how are we ending up with these messages? Are they all cases where we don't emit due to errors?
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.
Yea, a checker error blocks the .d.ts emit without noCheck
on.
*** Supplied discrepancy explanation but didnt file any difference |
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.
This seems like it needs fixing?
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.
Ah, there are other files with this (and other threads), forgot.
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.
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).
Yeah, this can happen in |
affectsSemanticDiagnostics: true, | ||
affectsBuildInfo: true, | ||
extraValidation() { | ||
return [Diagnostics.Unknown_compiler_option_0, "noCheck"]; |
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 and showConfig
, plus makes it easier to test as though it were CLI-allowed, like is done in the noCheck
unittest.
@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. |
|
For additional context, And |
Per out-of-band discussions, I've opened #58336 to formally track future work with |
Will this land in 5.5? |
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 underskipLibCheck
, 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
requiresemitDeclarationOnly
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 thegetNodeCheckFlags
calls in the JS transforms - which would be required to do #50699 and for this to fulfill #29651) Additionally, passingnoCheck
alongsidenoEmit
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.