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.

Sign up for GitHub

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

Jump to bottom

Allow --noCheck to be commandLine option #58839

Merged
merged 3 commits into from
Jun 14, 2024
Merged

Allow --noCheck to be commandLine option #58839

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5360b7b
Initial change to allow noCheck on commandLine (including build) and …
sheetalkamat Jun 7, 2024
5a84e20
Add checkPending for deciding whether to rebuild or report errors
sheetalkamat Jun 13, 2024
c99bbb7
Remove unnecessary comment
sheetalkamat Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions src/compiler/builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ import {
SemanticDiagnosticsBuilderProgram,
SignatureInfo,
skipAlias,
skipTypeChecking,
skipTypeCheckingIgnoringNoCheck,
some,
SourceFile,
sourceFileMayBeEmitted,
Expand Down Expand Up @@ -158,6 +158,8 @@ export interface ReusableBuilderProgramState extends BuilderState {
* emitKind pending for a program with --out
*/
programEmitPending?: BuilderFileEmit;
/** If semantic diagnsotic check is pending */
checkPending?: true;
/*
* true if semantic diagnostics are ReusableDiagnostic instead of Diagnostic
*/
Expand Down Expand Up @@ -329,6 +331,7 @@ function createBuilderProgramState(
}
state.changedFilesSet = new Set();
state.latestChangedDtsFile = compilerOptions.composite ? oldState?.latestChangedDtsFile : undefined;
state.checkPending = state.compilerOptions.noCheck ? true : undefined;

const useOldState = BuilderState.canReuseOldState(state.referencedMap, oldState);
const oldCompilerOptions = useOldState ? oldState!.compilerOptions : undefined;
Expand Down Expand Up @@ -470,9 +473,15 @@ function createBuilderProgramState(
state.programEmitPending | pendingEmitKind :
pendingEmitKind;
}
// if (!state.compilerOptions.noCheck)
sheetalkamat marked this conversation as resolved.
Show resolved Hide resolved
state.buildInfoEmitPending = true;
}
}
if (
useOldState &&
state.semanticDiagnosticsPerFile.size !== state.fileInfos.size &&
oldState!.checkPending !== state.checkPending
) state.buildInfoEmitPending = true;
return state;

function addFileToChangeSet(path: Path) {
Expand Down Expand Up @@ -741,7 +750,7 @@ function removeDiagnosticsOfLibraryFiles(state: BuilderProgramStateWithDefinedPr
const options = state.program.getCompilerOptions();
forEach(state.program.getSourceFiles(), f =>
state.program.isSourceFileDefaultLibrary(f) &&
!skipTypeChecking(f, options, state.program) &&
!skipTypeCheckingIgnoringNoCheck(f, options, state.program) &&
removeSemanticDiagnosticsOf(state, f.resolvedPath));
}
}
Expand Down Expand Up @@ -986,6 +995,7 @@ function getSemanticDiagnosticsOfFile(
cancellationToken: CancellationToken | undefined,
semanticDiagnosticsPerFile?: BuilderProgramState["semanticDiagnosticsPerFile"],
): readonly Diagnostic[] {
if (state.compilerOptions.noCheck) return emptyArray;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

return concatenate(
getBinderAndCheckerDiagnosticsOfFile(state, sourceFile, cancellationToken, semanticDiagnosticsPerFile),
state.program.getProgramDiagnostics(sourceFile),
Expand Down Expand Up @@ -1084,6 +1094,7 @@ export interface IncrementalBuildInfoBase extends BuildInfo {
// 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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@weswigham weswigham Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

/** @internal */
Expand Down Expand Up @@ -1131,6 +1142,7 @@ export function isIncrementalBuildInfo(info: BuildInfo): info is IncrementalBuil
export interface NonIncrementalBuildInfo extends BuildInfo {
root: readonly string[];
errors: true | undefined;
checkPending: true | undefined;
}

/** @internal */
Expand Down Expand Up @@ -1190,6 +1202,7 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
const buildInfo: NonIncrementalBuildInfo = {
root: arrayFrom(rootFileNames, r => relativeToBuildInfo(r)),
errors: state.hasErrors ? true : undefined,
checkPending: state.checkPending,
version,
};
return buildInfo;
Expand Down Expand Up @@ -1223,6 +1236,7 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
false : // Pending emit is same as deteremined by compilerOptions
state.programEmitPending, // Actual value
errors: state.hasErrors ? true : undefined,
checkPending: state.checkPending,
version,
};
return buildInfo;
Expand Down Expand Up @@ -1313,6 +1327,7 @@ function getBuildInfo(state: BuilderProgramStateWithDefinedProgram): BuildInfo {
emitSignatures,
latestChangedDtsFile,
errors: state.hasErrors ? true : undefined,
checkPending: state.checkPending,
version,
};
return buildInfo;
Expand Down Expand Up @@ -1952,7 +1967,13 @@ export function createBuilderProgram(
while (true) {
const affected = getNextAffectedFile(state, cancellationToken, host);
let result;
if (!affected) return undefined; // Done
if (!affected) {
if (state.checkPending && !state.compilerOptions.noCheck) {
state.checkPending = undefined;
state.buildInfoEmitPending = true;
}
return undefined; // Done
}
else if (affected !== state.program) {
// Get diagnostics for the affected file if its not ignored
const affectedSourceFile = affected as SourceFile;
Expand Down Expand Up @@ -1984,6 +2005,7 @@ export function createBuilderProgram(
result = diagnostics || emptyArray;
state.changedFilesSet.clear();
state.programEmitPending = getBuilderFileEmit(state.compilerOptions);
if (!state.compilerOptions.noCheck) state.checkPending = undefined;
state.buildInfoEmitPending = true;
}
return { result, affected };
Expand Down Expand Up @@ -2021,6 +2043,10 @@ export function createBuilderProgram(
for (const sourceFile of state.program.getSourceFiles()) {
diagnostics = addRange(diagnostics, getSemanticDiagnosticsOfFile(state, sourceFile, cancellationToken));
}
if (state.checkPending && !state.compilerOptions.noCheck) {
state.checkPending = undefined;
state.buildInfoEmitPending = true;
}
return diagnostics || emptyArray;
}
}
Expand Down Expand Up @@ -2089,6 +2115,7 @@ export function createBuilderProgramUsingIncrementalBuildInfo(
outSignature: buildInfo.outSignature,
programEmitPending: buildInfo.pendingEmit === undefined ? undefined : toProgramEmitPending(buildInfo.pendingEmit, buildInfo.options),
hasErrors: buildInfo.errors,
checkPending: buildInfo.checkPending,
};
}
else {
Expand Down Expand Up @@ -2125,6 +2152,7 @@ export function createBuilderProgramUsingIncrementalBuildInfo(
latestChangedDtsFile,
emitSignatures: emitSignatures?.size ? emitSignatures : undefined,
hasErrors: buildInfo.errors,
checkPending: buildInfo.checkPending,
};
}

Expand Down
42 changes: 19 additions & 23 deletions src/compiler/commandLineParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,25 @@ export const commonOptionsWithBuild: CommandLineOption[] = [
description: Diagnostics.Include_sourcemap_files_inside_the_emitted_JavaScript,
defaultValueDescription: false,
},
{
name: "noCheck",
type: "boolean",
showInSimplifiedHelpView: false,
category: Diagnostics.Compiler_Diagnostics,
description: Diagnostics.Disable_full_type_checking_only_critical_parse_and_emit_errors_will_be_reported,
transpileOptionValue: true,
defaultValueDescription: false,
// Not setting affectsSemanticDiagnostics or affectsBuildInfo because we dont want all diagnostics to go away, its handled in builder
},
sheetalkamat marked this conversation as resolved.
Show resolved Hide resolved
{
name: "noEmit",
type: "boolean",
showInSimplifiedHelpView: true,
category: Diagnostics.Emit,
description: Diagnostics.Disable_emitting_files_from_a_compilation,
transpileOptionValue: undefined,
defaultValueDescription: false,
},
{
name: "assumeChangesOnlyAffectDirectDependencies",
type: "boolean",
Expand Down Expand Up @@ -772,29 +791,6 @@ const commandOptionsWithoutBuild: CommandLineOption[] = [
defaultValueDescription: false,
description: Diagnostics.Disable_emitting_comments,
},
{
name: "noCheck",
type: "boolean",
showInSimplifiedHelpView: false,
category: Diagnostics.Compiler_Diagnostics,
description: Diagnostics.Disable_full_type_checking_only_critical_parse_and_emit_errors_will_be_reported,
transpileOptionValue: true,
defaultValueDescription: false,
affectsSemanticDiagnostics: true,
affectsBuildInfo: true,
extraValidation() {
return [Diagnostics.Unknown_compiler_option_0, "noCheck"];
},
},
{
name: "noEmit",
type: "boolean",
showInSimplifiedHelpView: true,
category: Diagnostics.Emit,
description: Diagnostics.Disable_emitting_files_from_a_compilation,
transpileOptionValue: undefined,
defaultValueDescription: false,
},
{
name: "importHelpers",
type: "boolean",
Expand Down
11 changes: 8 additions & 3 deletions src/compiler/tsbuildPublic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1535,7 +1535,11 @@ function getUpToDateStatusWorker<T extends BuilderProgram>(state: SolutionBuilde
};
}

if ((buildInfo as IncrementalBuildInfo | NonIncrementalBuildInfo).errors) {
if (
!project.options.noCheck &&
((buildInfo as IncrementalBuildInfo | NonIncrementalBuildInfo).errors || // TODO: syntax errors????
(buildInfo as IncrementalBuildInfo | NonIncrementalBuildInfo).checkPending)
) {
return {
type: UpToDateStatusType.OutOfDateBuildInfoWithErrors,
buildInfoFile: buildInfoPath,
Expand All @@ -1545,8 +1549,9 @@ function getUpToDateStatusWorker<T extends BuilderProgram>(state: SolutionBuilde
if (incrementalBuildInfo) {
// If there are errors, we need to build project again to report it
if (
incrementalBuildInfo.semanticDiagnosticsPerFile?.length ||
(!project.options.noEmit && getEmitDeclarations(project.options) && incrementalBuildInfo.emitDiagnosticsPerFile?.length)
!project.options.noCheck &&
(incrementalBuildInfo.semanticDiagnosticsPerFile?.length ||
(!project.options.noEmit && getEmitDeclarations(project.options) && incrementalBuildInfo.emitDiagnosticsPerFile?.length))
) {
return {
type: UpToDateStatusType.OutOfDateBuildInfoWithErrors,
Expand Down
26 changes: 24 additions & 2 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10108,13 +10108,35 @@ export interface HostWithIsSourceOfProjectReferenceRedirect {
isSourceOfProjectReferenceRedirect(fileName: string): boolean;
}
/** @internal */
export function skipTypeChecking(sourceFile: SourceFile, options: CompilerOptions, host: HostWithIsSourceOfProjectReferenceRedirect) {
export function skipTypeChecking(
sourceFile: SourceFile,
options: CompilerOptions,
host: HostWithIsSourceOfProjectReferenceRedirect,
) {
return skipTypeCheckingWorker(sourceFile, options, host, /*ignoreNoCheck*/ false);
}

/** @internal */
export function skipTypeCheckingIgnoringNoCheck(
sourceFile: SourceFile,
options: CompilerOptions,
host: HostWithIsSourceOfProjectReferenceRedirect,
) {
return skipTypeCheckingWorker(sourceFile, options, host, /*ignoreNoCheck*/ true);
}

function skipTypeCheckingWorker(
sourceFile: SourceFile,
options: CompilerOptions,
host: HostWithIsSourceOfProjectReferenceRedirect,
ignoreNoCheck: boolean,
) {
// If skipLibCheck is enabled, skip reporting errors if file is a declaration file.
// If skipDefaultLibCheck is enabled, skip reporting errors if file contains a
// '/// <reference no-default-lib="true"/>' directive.
return (options.skipLibCheck && sourceFile.isDeclarationFile ||
options.skipDefaultLibCheck && sourceFile.hasNoDefaultLib) ||
options.noCheck ||
(!ignoreNoCheck && options.noCheck) ||
host.isSourceOfProjectReferenceRedirect(sourceFile.fileName) ||
!canIncludeBindAndCheckDiagnostics(sourceFile, options);
}
Expand Down
1 change: 1 addition & 0 deletions src/testRunner/tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export * from "./unittests/tsc/incremental.js";
export * from "./unittests/tsc/libraryResolution.js";
export * from "./unittests/tsc/listFilesOnly.js";
export * from "./unittests/tsc/moduleResolution.js";
export * from "./unittests/tsc/noCheck.js";
export * from "./unittests/tsc/noEmit.js";
export * from "./unittests/tsc/noEmitOnError.js";
export * from "./unittests/tsc/projectReferences.js";
Expand Down
92 changes: 92 additions & 0 deletions src/testRunner/unittests/helpers/noCheck.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { jsonToReadableText } from "../helpers.js";
import {
noChangeRun,
TestTscEdit,
verifyTsc,
} from "./tsc.js";
import { loadProjectFromFiles } from "./vfs.js";

export function forEachTscScenarioWithNoCheck(buildType: "-b" | "-p") {
const commandLineArgs = buildType === "-b" ?
["-b", "/src/tsconfig.json", "-v"] :
["-p", "/src/tsconfig.json"];

function forEachNoCheckScenarioWorker(
subScenario: string,
aText: string,
) {
const checkNoChangeRun: TestTscEdit = {
...noChangeRun,
caption: "No Change run with checking",
commandLineArgs,
};
const noCheckFixError: TestTscEdit = {
caption: "Fix `a` error with noCheck",
edit: fs => fs.writeFileSync("/src/a.ts", `export const a = "hello";`),
};
const noCheckError: TestTscEdit = {
caption: "Introduce error with noCheck",
edit: fs => fs.writeFileSync("/src/a.ts", aText),
};
const noChangeRunWithCheckPendingDiscrepancy: TestTscEdit = {
...noChangeRun,
discrepancyExplanation: () => [
"Clean build will have check pending since it didnt type check",
"Incremental build has typechecked before this so wont have checkPending",
],
};

[undefined, true].forEach(incremental => {
[{}, { module: "amd", outFile: "../outFile.js" }].forEach(options => {
verifyTsc({
scenario: "noCheck",
subScenario: `${options.outFile ? "outFile" : "multiFile"}/${subScenario}${incremental ? " with incremental" : ""}`,
fs: () =>
loadProjectFromFiles({
"/src/a.ts": aText,
"/src/b.ts": `export const b = 10;`,
"/src/tsconfig.json": jsonToReadableText({
compilerOptions: {
declaration: true,
incremental,
...options,
},
}),
}),
commandLineArgs: [...commandLineArgs, "--noCheck"],
edits: [
noChangeRun, // Should be no op
noCheckFixError, // Fix error with noCheck
noChangeRun, // Should be no op
checkNoChangeRun, // Check errors - should not report any errors - update buildInfo
checkNoChangeRun, // Should be no op
incremental || buildType === "-b" ?
noChangeRunWithCheckPendingDiscrepancy : // Should be no op
noChangeRun, // Should be no op
noCheckError,
noChangeRun, // Should be no op
checkNoChangeRun, // Should check errors and update buildInfo
noCheckFixError, // Fix error with noCheck
checkNoChangeRun, // Should check errors and update buildInfo
{
caption: "Add file with error",
edit: fs => fs.writeFileSync("/src/c.ts", `export const c: number = "hello";`),
commandLineArgs,
},
noCheckError,
noCheckFixError,
checkNoChangeRun,
incremental || buildType === "-b" ?
noChangeRunWithCheckPendingDiscrepancy : // Should be no op
noChangeRun, // Should be no op
checkNoChangeRun, // Should be no op
],
baselinePrograms: true,
});
});
});
}
forEachNoCheckScenarioWorker("syntax errors", `export const a = "hello`);
forEachNoCheckScenarioWorker("semantic errors", `export const a: number = "hello";`);
forEachNoCheckScenarioWorker("dts errors", `export const a = class { private p = 10; };`);
}
Loading