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.

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

Do not calculate signatures if old state is not used #43314

Merged
merged 5 commits into from
Mar 23, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
12 changes: 10 additions & 2 deletions src/compiler/builderState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ namespace ts {
* Otherwise undefined
*/
readonly exportedModulesMap: ESMap<Path, BuilderState.ReferencedSet> | undefined;

/**
* true if file version is used as signature
* This helps in delaying the calculation of the d.ts hash as version for the file till reasonable time
*/
useFileVersionAsSignature: boolean;
/**
* Map of files that have already called update signature.
* That means hence forth these files are assumed to have
Expand Down Expand Up @@ -236,7 +242,8 @@ namespace ts {
fileInfos,
referencedMap,
exportedModulesMap,
hasCalledUpdateShapeSignature
hasCalledUpdateShapeSignature,
useFileVersionAsSignature: !useOldState
};
}

Expand All @@ -258,6 +265,7 @@ namespace ts {
referencedMap: state.referencedMap && new Map(state.referencedMap),
exportedModulesMap: state.exportedModulesMap && new Map(state.exportedModulesMap),
hasCalledUpdateShapeSignature: new Set(state.hasCalledUpdateShapeSignature),
useFileVersionAsSignature: state.useFileVersionAsSignature,
};
}

Expand Down Expand Up @@ -317,7 +325,7 @@ namespace ts {

const prevSignature = info.signature;
let latestSignature: string | undefined;
if (!sourceFile.isDeclarationFile) {
if (!sourceFile.isDeclarationFile && !state.useFileVersionAsSignature) {
const emitOutput = getFileEmitOutput(
programOfThisState,
sourceFile,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,16 @@ namespace ts {
subScenario: "inferred type from transitive module",
fs: () => projFs,
commandLineArgs: ["--b", "/src", "--verbose"],
incrementalScenarios: [{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam,
}],
incrementalScenarios: [
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam,
},
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParamBack,
},
],
});

verifyTscSerializedIncrementalEdits({
Expand All @@ -25,10 +31,16 @@ namespace ts {
scenario: "inferredTypeFromTransitiveModule",
commandLineArgs: ["--b", "/src", "--verbose"],
modifyFs: changeToIsolatedModules,
incrementalScenarios: [{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam
}]
incrementalScenarios: [
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam
},
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParamBack,
},
]
});

verifyTscSerializedIncrementalEdits({
Expand All @@ -43,6 +55,14 @@ import { default as bar } from './bar';
bar("hello");`);
},
incrementalScenarios: [
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam
},
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParamBack,
},
{
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: changeBarParam
Expand All @@ -63,4 +83,8 @@ bar("hello");`);
function changeBarParam(fs: vfs.FileSystem) {
replaceText(fs, "/src/bar.ts", "param: string", "");
}

function changeBarParamBack(fs: vfs.FileSystem) {
replaceText(fs, "/src/bar.ts", "foobar()", "foobar(param: string)");
}
}
14 changes: 10 additions & 4 deletions src/testRunner/unittests/tsbuild/lateBoundSymbol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ namespace ts {
fs: () => loadProjectFromDisk("tests/projects/lateBoundSymbol"),
scenario: "lateBoundSymbol",
commandLineArgs: ["--b", "/src/tsconfig.json", "--verbose"],
incrementalScenarios: [{
buildKind: BuildKind.IncrementalDtsUnchanged,
modifyFs: fs => replaceText(fs, "/src/src/main.ts", "const x = 10;", ""),
}]
incrementalScenarios: [
{
buildKind: BuildKind.IncrementalDtsUnchanged,
modifyFs: fs => replaceText(fs, "/src/src/main.ts", "const x = 10;", ""),
},
{
buildKind: BuildKind.IncrementalDtsUnchanged,
modifyFs: fs => appendText(fs, "/src/src/main.ts", "const x = 10;"),
},
]
});
});
}
53 changes: 31 additions & 22 deletions src/testRunner/unittests/tsc/incremental.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,11 @@ const a: string = 10;`, "utf-8"),
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: fs => appendText(fs, `/src/project/src/main.ts`, `something();`),
},
{
subScenario: "Modify main file again",
buildKind: BuildKind.IncrementalDtsChange,
modifyFs: fs => appendText(fs, `/src/project/src/main.ts`, `something();`),
},
{
subScenario: "Add new file and update main file",
buildKind: BuildKind.IncrementalDtsChange,
Expand All @@ -288,7 +293,9 @@ const a: string = 10;`, "utf-8"),
baselinePrograms: true,
});

const jsxLibraryContent = `
describe("when synthesized imports are added to files", () => {
function getJsxLibraryContent() {
return `
export {};
declare global {
namespace JSX {
Expand All @@ -300,29 +307,31 @@ declare global {
}
}
}`;
}

verifyTsc({
scenario: "react-jsx-emit-mode",
subScenario: "with no backing types found doesn't crash",
fs: () => loadProjectFromFiles({
"/src/project/node_modules/react/jsx-runtime.js": "export {}", // js needs to be present so there's a resolution result
"/src/project/node_modules/@types/react/index.d.ts": jsxLibraryContent, // doesn't contain a jsx-runtime definition
"/src/project/src/index.tsx": `export const App = () => <div propA={true}></div>;`,
"/src/project/tsconfig.json": JSON.stringify({ compilerOptions: { module: "commonjs", jsx: "react-jsx", incremental: true, jsxImportSource: "react" } })
}),
commandLineArgs: ["--p", "src/project"]
});
verifyTsc({
scenario: "react-jsx-emit-mode",
subScenario: "with no backing types found doesn't crash",
fs: () => loadProjectFromFiles({
"/src/project/node_modules/react/jsx-runtime.js": "export {}", // js needs to be present so there's a resolution result
"/src/project/node_modules/@types/react/index.d.ts": getJsxLibraryContent(), // doesn't contain a jsx-runtime definition
"/src/project/src/index.tsx": `export const App = () => <div propA={true}></div>;`,
"/src/project/tsconfig.json": JSON.stringify({ compilerOptions: { module: "commonjs", jsx: "react-jsx", incremental: true, jsxImportSource: "react" } })
}),
commandLineArgs: ["--p", "src/project"]
});

verifyTsc({
scenario: "react-jsx-emit-mode",
subScenario: "with no backing types found doesn't crash under --strict",
fs: () => loadProjectFromFiles({
"/src/project/node_modules/react/jsx-runtime.js": "export {}", // js needs to be present so there's a resolution result
"/src/project/node_modules/@types/react/index.d.ts": jsxLibraryContent, // doesn't contain a jsx-runtime definition
"/src/project/src/index.tsx": `export const App = () => <div propA={true}></div>;`,
"/src/project/tsconfig.json": JSON.stringify({ compilerOptions: { module: "commonjs", jsx: "react-jsx", incremental: true, jsxImportSource: "react" } })
}),
commandLineArgs: ["--p", "src/project", "--strict"]
verifyTsc({
scenario: "react-jsx-emit-mode",
subScenario: "with no backing types found doesn't crash under --strict",
fs: () => loadProjectFromFiles({
"/src/project/node_modules/react/jsx-runtime.js": "export {}", // js needs to be present so there's a resolution result
"/src/project/node_modules/@types/react/index.d.ts": getJsxLibraryContent(), // doesn't contain a jsx-runtime definition
"/src/project/src/index.tsx": `export const App = () => <div propA={true}></div>;`,
"/src/project/tsconfig.json": JSON.stringify({ compilerOptions: { module: "commonjs", jsx: "react-jsx", incremental: true, jsxImportSource: "react" } })
}),
commandLineArgs: ["--p", "src/project", "--strict"]
});
});
});
}
10 changes: 10 additions & 0 deletions src/testRunner/unittests/tscWatch/emit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ namespace ts.tscWatch {
caption: "Make change in the file",
change: sys => sys.writeFile("/a/a.ts", "let x = 11"),
timeouts: runQueuedTimeoutCallbacks
},
{
caption: "Make change in the file again",
change: sys => sys.writeFile("/a/a.ts", "let xy = 11"),
timeouts: runQueuedTimeoutCallbacks
}
]
});
Expand Down Expand Up @@ -410,6 +415,11 @@ export var x = Foo();`
caption: "Append content to f1",
change: sys => sys.appendFile("/a/b/f1.ts", "export function foo2() { return 2; }"),
timeouts: checkSingleTimeoutQueueLengthAndRun,
},
{
caption: "Again Append content to f1",
change: sys => sys.appendFile("/a/b/f1.ts", "export function fooN() { return 2; }"),
timeouts: checkSingleTimeoutQueueLengthAndRun,
}
],
});
Expand Down
43 changes: 42 additions & 1 deletion src/testRunner/unittests/tscWatch/emitAndErrorUpdates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,17 @@ namespace ts.tscWatch {
changes,
baselineIncremental
});
verifyTscWatch({
scenario: "emitAndErrorUpdates",
subScenario: `incremental/${subScenario}`,
commandLineArgs: ["--w", "--i"],
sys: () => createWatchedSystem(
[...files(), configFile(), lib?.() || libFile],
{ currentDirectory: currentDirectory || projectRoot }
),
changes,
baselineIncremental
});
}

function changeCompilerOptions(input: VerifyEmitAndErrorUpdates, additionalOptions: CompilerOptions): File {
Expand Down Expand Up @@ -97,6 +108,16 @@ console.log(b.c.d);`
subScenario: `deepImportChanges/${subScenario}`,
files: () => [aFile, bFile, cFile],
changes: [
{
caption: "Rename property d to d2 of class C to initialize signatures",
change: sys => sys.writeFile(cFile.path, cFile.content.replace("d", "d2")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property d2 to d of class C to revert back to original text",
change: sys => sys.writeFile(cFile.path, cFile.content.replace("d2", "d")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property d to d2 of class C",
change: sys => sys.writeFile(cFile.path, cFile.content.replace("d", "d2")),
Expand Down Expand Up @@ -197,11 +218,21 @@ getPoint().c.x;`
subScenario: "file not exporting a deep multilevel import that changes",
files: () => [aFile, bFile, cFile, dFile, eFile],
changes: [
{
caption: "Rename property x2 to x of interface Coords to initialize signatures",
change: sys => sys.writeFile(aFile.path, aFile.content.replace("x2", "x")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property x to x2 of interface Coords to revert back to original text",
change: sys => sys.writeFile(aFile.path, aFile.content.replace("x: number", "x2: number")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property x2 to x of interface Coords",
change: sys => sys.writeFile(aFile.path, aFile.content.replace("x2", "x")),
timeouts: runQueuedTimeoutCallbacks,
}
},
]
});
});
Expand Down Expand Up @@ -260,6 +291,16 @@ export class Data {
files: () => [lib1ToolsInterface, lib1ToolsPublic, app, lib2Public, lib1Public, ...files],
configFile: () => config,
changes: [
{
caption: "Rename property title to title2 of interface ITest to initialize signatures",
change: sys => sys.writeFile(lib1ToolsInterface.path, lib1ToolsInterface.content.replace("title", "title2")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property title2 to title of interface ITest to revert back to original text",
change: sys => sys.writeFile(lib1ToolsInterface.path, lib1ToolsInterface.content.replace("title2", "title")),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: "Rename property title to title2 of interface ITest",
change: sys => sys.writeFile(lib1ToolsInterface.path, lib1ToolsInterface.content.replace("title", "title2")),
Expand Down
4 changes: 2 additions & 2 deletions src/testRunner/unittests/tscWatch/incremental.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,12 @@ namespace ts.tscWatch {
});
assert.deepEqual(state.fileInfos.get(file1.path as Path), {
version: system.createHash(file1.content),
signature: system.createHash(`${file1.content.replace("export ", "export declare ")}\n`),
signature: system.createHash(file1.content),
affectsGlobalScope: false,
});
assert.deepEqual(state.fileInfos.get(file2.path as Path), {
version: system.createHash(fileModified.content),
signature: system.createHash("export declare const y: string;\n"),
signature: system.createHash(fileModified.content),
affectsGlobalScope: false,
});

Expand Down
10 changes: 10 additions & 0 deletions src/testRunner/unittests/tscWatch/programUpdates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ namespace ts.tscWatch {
return createWatchedSystem([libFile, commonFile1, commonFile2, configFile]);
},
changes: [
{
caption: "change file to ensure signatures are updated",
change: sys => sys.appendFile(commonFile2.path, ";let xy = 10;"),
timeouts: checkSingleTimeoutQueueLengthAndRun,
},
{
caption: "delete file2",
change: sys => sys.deleteFile(commonFile2.path),
Expand Down Expand Up @@ -171,6 +176,11 @@ namespace ts.tscWatch {
return createWatchedSystem([libFile, commonFile1, commonFile2, configFile]);
},
changes: [
{
caption: "change file to ensure signatures are updated",
change: sys => sys.appendFile(commonFile2.path, ";let xy = 10;"),
timeouts: checkSingleTimeoutQueueLengthAndRun,
},
{
caption: "Change config",
change: sys => sys.writeFile(configFilePath, `{
Expand Down
4 changes: 4 additions & 0 deletions src/testRunner/unittests/tsserver/compileOnSave.ts
Original file line number Diff line number Diff line change
Expand Up @@ -951,10 +951,14 @@ function bar() {
}

// Change file1 get affected file list
verifyLocalEdit(file1, "hello", "world", /*returnsAllFilesAsAffected*/ !declaration); // Signatures are not initialized before this request
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if compileOnSave should disable this behavior given it doesnt save all files so will be anyways calculating signatures for affected set and not going to change from one edit to next ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@sheetalkamat Can you please elaborate? I think you're saying

  1. Old behavior: compute (emit?) .d.ts on compile-on-save
  2. New behavior: don't compute (emit?) .d.ts on (first?) compile-on-save
  3. Proposed behavior: restore old behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

If @amcasey's explanation is correct, then yes - my instinct says that compile-on-save should compute+emit .d.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes thats what i meant.. i will revert to old behavior for compile on save

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and ready for review

verifyLocalEdit(file1, "world", "hello");
verifyLocalEdit(file1, "hello", "world");

// Change file2 get affected file list = will return only file2 if --declaration otherwise all files
verifyLocalEdit(file2, "world", "hello", /*returnsAllFilesAsAffected*/ !declaration);
verifyLocalEdit(file2, "hello", "world");
verifyLocalEdit(file2, "world", "hello");

function verifyFileSave(file: File) {
const response = session.executeCommandSeq<protocol.CompileOnSaveEmitFileRequest>({
Expand Down
Loading