Uh oh!
There was an error while loading. Please reload this page.
Allow --noCheck to be commandLine option - #58839
Conversation
Uh oh!
There was an error while loading. Please reload this page.
6dc2e31 to
b641a43CompareJake Bailey (jakebailey)
commented
Jun 13, 2024
This will close #29651, right? |
Sheetal Nandi (sheetalkamat)
commented
Jun 13, 2024
Yes. We would report ssyntax errors though while emitting. |
Bruce Pascoe (fatcerberus)
commented
Jun 13, 2024
Oh no, I hadn't realized some of the TypeScript devs were snakes |
b641a43 to
7397f7bCompare7397f7b to
5a84e20CompareSheetal Nandi (sheetalkamat)
commented
Jun 13, 2024
TypeScript Bot (@typescript-bot) test it |
TypeScript Bot (typescript-bot)
commented
Jun 13, 2024
Hey Sheetal Nandi (@sheetalkamat), the results of running the DT tests are ready. Everything looks the same! |
Uh oh!
There was an error while loading. Please reload this page.
TypeScript Bot (typescript-bot)
commented
Jun 13, 2024
Sheetal Nandi (@sheetalkamat) Here are the results of running the user tests with tsc comparing Everything looks good! |
| cancellationToken: CancellationToken | undefined, | ||
| semanticDiagnosticsPerFile?: BuilderProgramState["semanticDiagnosticsPerFile"], | ||
| ): readonly Diagnostic[] { | ||
| if (state.compilerOptions.noCheck) return emptyArray; |
There was a problem hiding this comment.
Isn't it funky that a noCheck with -b fetches different diagnostics than a noCheck normally does? This is going to skip all the program construction errors that noCheck would normally still report, no?
There was a problem hiding this comment.
No. If typechecking is skipped the semantic and program diagnostics are skipped for that file too.. (Look at getBindAndCheckDiagnostics as well as getProgramDiagnostics in program)
There was a problem hiding this comment.
Oh, so this early bail here just skips the per-file iteration that'll internally bail on every individual file anyway; got it. It's not really clear that that's guaranteed to be the case.
TypeScript Bot (typescript-bot)
commented
Jun 13, 2024
Sheetal Nandi (@sheetalkamat) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Because this is only output file in the program, we dont need fileId to deduplicate name | ||
| latestChangedDtsFile?: string | undefined; | ||
| errors: true | undefined; | ||
| checkPending: true | undefined; |
There was a problem hiding this comment.
Isn't this exactly derived from the options? Like, isn't this just options.noCheck? Eveywhere I'm looking in this PR, this is either set if options.noCheck in the buildinfo is present, or unset if it isn't. I guess NonIncrementalBuildInfo doesn't have options stored, but honestly, we should probably just always store them all.
There was a problem hiding this comment.
No its not same as options.noCheck since it takes into account run with --noCheck, one without noCheck and then --noCheck without any updates in between does not update tsbuildInfo to mark it as pending. So its really if we havent done semanticdiagnostics calculation
There was a problem hiding this comment.
Right, it just seems like that's the same check we do for differing diagnostics-affecting options anyway? Like this is kinda redundant? (Though technically isn't since it's not using that mechanism with this PR.)
You run once without <flag> set, those options get cached into the .buildinfo, then you do a run with <flag> set - we compare the old <flag> value (nonextant) with the new <flag> value (present), and if <flag> affects diagnostics, mark the build as needing a .buildinfo update, minimally, and some subset of diagnostics and emit updated (as appropriate for the flag). At least for --incremental. Adding noCheck would, then, trigger a typecheck, yes, but it'd be a basically free no-op typecheck (since the flag's function is to skip it). Removing the flag then, would, naturally, also trigger a new typecheck (and you'd get the errors back). Just seems weird we wouldn't reuse that logic for non-incremental, and instead special-case the option like this. Doesn't seem like it's actually skipping anything beyond a few function calls that'd skip their work internally with the flag set anyway, and is duplicating what already happens inside createBuilderProgramState so long as the options from the old non-incremental build are actually serialized - which is why it seems so much like we're just hoisting the option out of the options list and into the root buildinfo.
There was a problem hiding this comment.
That’s because we don’t want to cached diagnostics that happened as part of checking when running with no check..
This helps with keeping the cache .. if you see the diff of second commit you should see that we are now able to retain diagnostics of unchanged files
we do exactly same thing as part of pending emit so we don’t have to emit already emitted files
There was a problem hiding this comment.
I think I get what we're doing here that's special - we're trying to reuse the .buildinfo from an old non-noCheck build for another non-noCheck without a noCheck build in the middle clobbering it.
That feels kinda weird, since it's basically like we're ignoring the noCheck build even happened and not updating the state, but I kinda get why you'd want that - so long as the state on disk is from the non-noCheck build, you can start a noCheck (for emit) and non-noCheck (for checking) build in parallel off it.
Personally, I assumed you'd want separate .buildinfos for you non-noCheck and noCheck build parts if you were trying to ensure they didn't clobber one another as you parallelized things. This kinda feels like it's trying to enable running the two builds with different options collaboratively off of a single .buildinfo (since if the noCheck runs first, it'll leave the diagnostics part to the next build, while if the noEmit part runs first, it'll leave the emit part for the next build to fill in), which feels kinda odd. Both orders work in serial under this scheme, but if you actually parallelize it, one very likely could clobber the other's .buildinfo output regardless. So I guess it's kinda neat that we can do this, but I'm not sure why we'd need to. If you are running them in serial, the options themselves are going to skip the relevant work, so skipping them at the orchestrator level is a bit redundant and just lets you run them in arbitrary order repeatedly without being penalized with a rebuild when you toggle emit/check on and then off again, but in parallel, you need separate buildinfos to prevent race conditions between the processes anyway, which were never going to trigger a rebuild unless there was an actual substantive change in the first place.
I guess the TL;DR is:
Do we actually care if
tsc -b . --noCheck
tsc -b . --noEmit
tsc -b . --noCheck
tsc -b . --noEmit
reuses diagnostics from the first --noEmit run on the second --noEmit line? These kind of partial output builds are only useful in parallel, and in parallel, you need separate .buildinfos for safety anyway. I'd expect we'd wanna enable and recommend something like
tsc -b . --noCheck
tsc -b . --noEmit --tsBuildInfoFile ${configDir}/noemit.buildinfo
tsc -b . --noCheck
tsc -b . --noEmit --tsBuildInfoFile ${configDir}/noemit.buildinfo
instead, since that'll work even if you spawn the differently-option'd tsc -bs in parallel.
There was a problem hiding this comment.
We do care about that and there is buildInfo fidelity built into it. Eg currenly we do support tsc -b --noEmit followed by tsc -b so it doesnt typecheck again.
There was a problem hiding this comment.
We are not doing this for parallel builds but subsequent builds. Eg imagine running tsc -b --noCheck -w while debugging and once done last running tsc -b
There was a problem hiding this comment.
I guess it's fine to have this support, I'm just not sure it's gonna help many people in ways that aren't gonna lead them to a pit of failure. Like you see the sequential builds with different options smartly don't trigger rebuilds, so then naturally if you're a watch user you spawn one window with tsc -b --noCheck -w and one with tsc -b --noEmit -w as your super simple easy top-level parallelization, and that bricks the state, since the two builds will repeatedly clobber one another, rather than cooperate to build a single more complete state like the sequential builds do.
TypeScript Bot (typescript-bot)
commented
Jun 13, 2024
Sheetal Nandi (@sheetalkamat) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
| // Because this is only output file in the program, we dont need fileId to deduplicate name | ||
| latestChangedDtsFile?: string | undefined; | ||
| errors: true | undefined; | ||
| checkPending: true | undefined; |
There was a problem hiding this comment.
I guess it's fine to have this support, I'm just not sure it's gonna help many people in ways that aren't gonna lead them to a pit of failure. Like you see the sequential builds with different options smartly don't trigger rebuilds, so then naturally if you're a watch user you spawn one window with tsc -b --noCheck -w and one with tsc -b --noEmit -w as your super simple easy top-level parallelization, and that bricks the state, since the two builds will repeatedly clobber one another, rather than cooperate to build a single more complete state like the sequential builds do.
Works on top of #58626 and #58838
Adds
checkPendingto buildInfo so as to handle if there was noCheck pending. This does not add "noCheck" as semantic affecting diagnostics because builder can handle that and save the diagnostics between runs with and without "noCheck"