From b155a71e702d1bf2824009406d601166213716e0 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 25 Jun 2018 16:41:09 -0700 Subject: [PATCH 1/2] Retain the version information of script infos when they are deleted This helps in having to not restart the versioning, which could potentially have same version but different contents and project could confuse with it --- src/harness/virtualFileSystemWithWatch.ts | 7 ++ src/server/editorServices.ts | 22 +++-- src/server/scriptInfo.ts | 31 ++++-- .../unittests/tsserverProjectSystem.ts | 97 +++++++++++++++++++ .../reference/api/tsserverlibrary.d.ts | 12 ++- 5 files changed, 148 insertions(+), 21 deletions(-) diff --git a/src/harness/virtualFileSystemWithWatch.ts b/src/harness/virtualFileSystemWithWatch.ts index 6645e12c8321a..d56bc37cc2157 100644 --- a/src/harness/virtualFileSystemWithWatch.ts +++ b/src/harness/virtualFileSystemWithWatch.ts @@ -615,6 +615,13 @@ interface Array {}` } } + removeFile(filePath: string) { + const path = this.toFullPath(filePath); + const currentEntry = this.fs.get(path) as FsFile; + Debug.assert(isFsFile(currentEntry)); + this.removeFileOrFolder(currentEntry, returnFalse); + } + removeFolder(folderPath: string, recursive?: boolean) { const path = this.toFullPath(folderPath); const currentEntry = this.fs.get(path) as FsFolder; diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 0801c42405c20..5394d53079d63 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -334,6 +334,11 @@ namespace ts.server { return project.dirty && project.updateGraph(); } + type ScriptInfoOrVersion = ScriptInfo | ScriptInfoVersion; + function isScriptInfoVersion(infoOrVersion: ScriptInfoOrVersion): infoOrVersion is ScriptInfoVersion { + return (infoOrVersion as ScriptInfoVersion).svc !== undefined; + } + export class ProjectService { /*@internal*/ @@ -345,7 +350,7 @@ namespace ts.server { /** * Container of all known scripts */ - private readonly filenameToScriptInfo = createMap(); + private readonly filenameToScriptInfo = createMap(); // Set of all '.js' files ever opened. private readonly allJsFilesForOpenFileTelemetry = createMap(); @@ -884,7 +889,7 @@ namespace ts.server { project.close(); if (Debug.shouldAssert(AssertionLevel.Normal)) { - this.filenameToScriptInfo.forEach(info => Debug.assert(!info.isAttached(project))); + this.filenameToScriptInfo.forEach(info => Debug.assert(isScriptInfoVersion(info) || !info.isAttached(project))); } // Remove the project from pending project updates this.pendingProjectUpdates.delete(project.getProjectName()); @@ -1021,7 +1026,7 @@ namespace ts.server { } private deleteScriptInfo(info: ScriptInfo) { - this.filenameToScriptInfo.delete(info.path); + this.filenameToScriptInfo.set(info.path, info.getVersion()); const realpath = info.getRealpathIfDifferent(); if (realpath) { this.realpathToScriptInfos!.remove(realpath, info); // TODO: GH#18217 @@ -1850,8 +1855,8 @@ namespace ts.server { private getOrCreateScriptInfoWorker(fileName: NormalizedPath, currentDirectory: string, openedByClient: boolean, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean, hostToQueryFileExistsOn?: { fileExists(path: string): boolean; }) { Debug.assert(fileContent === undefined || openedByClient, "ScriptInfo needs to be opened by client to be able to set its user defined content"); const path = normalizedPathToPath(fileName, currentDirectory, this.toCanonicalFileName); - let info = this.getScriptInfoForPath(path); - if (!info) { + let info = this.filenameToScriptInfo.get(path); + if (!info || isScriptInfoVersion(info)) { const isDynamic = isDynamicFileName(fileName); Debug.assert(isRootedDiskPath(fileName) || isDynamic || openedByClient, "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nScript info with non-dynamic relative file name can only be open script info`); Debug.assert(!isRootedDiskPath(fileName) || this.currentDirectory === currentDirectory || !this.openFilesWithNonRootedDiskPath.has(this.toCanonicalFileName(fileName)), "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nOpen script files with non rooted disk path opened with current directory context cannot have same canonical names`); @@ -1860,7 +1865,7 @@ namespace ts.server { if (!openedByClient && !isDynamic && !(hostToQueryFileExistsOn || this.host).fileExists(fileName)) { return; } - info = new ScriptInfo(this.host, fileName, scriptKind!, !!hasMixedContent, path); // TODO: GH#18217 + info = new ScriptInfo(this.host, fileName, scriptKind!, !!hasMixedContent, path, info); // TODO: GH#18217 this.filenameToScriptInfo.set(info.path, info); if (!openedByClient) { this.watchClosedScriptInfo(info); @@ -1894,7 +1899,8 @@ namespace ts.server { } getScriptInfoForPath(fileName: Path) { - return this.filenameToScriptInfo.get(fileName); + const info = this.filenameToScriptInfo.get(fileName); + return info && isScriptInfoVersion(info) ? undefined : info; } setHostConfiguration(args: protocol.ConfigureRequestArguments) { @@ -2145,7 +2151,7 @@ namespace ts.server { // It was then postponed to cleanup these script infos so that they can be reused if // the file from that old project is reopened because of opening file from here. this.filenameToScriptInfo.forEach(info => { - if (!info.isScriptOpen() && info.isOrphan()) { + if (!isScriptInfoVersion(info) && !info.isScriptOpen() && info.isOrphan()) { // if there are not projects that include this script info - delete it this.stopWatchingScriptInfo(info); this.deleteScriptInfo(info); diff --git a/src/server/scriptInfo.ts b/src/server/scriptInfo.ts index e8f1d931ac2af..396c9963b54a7 100644 --- a/src/server/scriptInfo.ts +++ b/src/server/scriptInfo.ts @@ -1,13 +1,20 @@ namespace ts.server { + /*@internal*/ + export interface ScriptInfoVersion { + svc: number; + text: number; + } /* @internal */ export class TextStorage { + /*@internal*/ + version: ScriptInfoVersion; + /** * Generated only on demand (based on edits, or information requested) * The property text is set to undefined when edits happen on the cache */ private svc: ScriptVersionCache | undefined; - private svcVersion = 0; /** * Stores the text when there are no changes to the script version cache @@ -19,7 +26,6 @@ namespace ts.server { * Line map for the text when there is no script version cache present */ private lineMap: number[] | undefined; - private textVersion = 0; /** * True if the text is for the file thats open in the editor @@ -34,13 +40,14 @@ namespace ts.server { */ private pendingReloadFromDisk: boolean; - constructor(private readonly host: ServerHost, private readonly fileName: NormalizedPath) { + constructor(private readonly host: ServerHost, private readonly fileName: NormalizedPath, initialVersion?: ScriptInfoVersion) { + this.version = initialVersion || { svc: 0, text: 0 }; } public getVersion() { return this.svc - ? `SVC-${this.svcVersion}-${this.svc.getSnapshotVersion()}` - : `Text-${this.textVersion}`; + ? `SVC-${this.version.svc}-${this.svc.getSnapshotVersion()}` + : `Text-${this.version.text}`; } public hasScriptVersionCache_TestOnly() { @@ -55,7 +62,7 @@ namespace ts.server { this.svc = undefined; this.text = newText; this.lineMap = undefined; - this.textVersion++; + this.version.text++; } public edit(start: number, end: number, newText: string) { @@ -163,7 +170,7 @@ namespace ts.server { private switchToScriptVersionCache(): ScriptVersionCache { if (!this.svc || this.pendingReloadFromDisk) { this.svc = ScriptVersionCache.fromString(this.getOrLoadText()); - this.svcVersion++; + this.version.svc++; } return this.svc; } @@ -235,10 +242,11 @@ namespace ts.server { readonly fileName: NormalizedPath, readonly scriptKind: ScriptKind, public readonly hasMixedContent: boolean, - readonly path: Path) { + readonly path: Path, + initialVersion?: ScriptInfoVersion) { this.isDynamic = isDynamicFileName(fileName); - this.textStorage = new TextStorage(host, fileName); + this.textStorage = new TextStorage(host, fileName, initialVersion); if (hasMixedContent || this.isDynamic) { this.textStorage.reload(""); this.realpath = this.path; @@ -248,6 +256,11 @@ namespace ts.server { : getScriptKindFromFileName(fileName); } + /*@internal*/ + getVersion() { + return this.textStorage.version; + } + /*@internal*/ public isDynamicOrHasMixedContent() { return this.hasMixedContent || this.isDynamic; diff --git a/src/testRunner/unittests/tsserverProjectSystem.ts b/src/testRunner/unittests/tsserverProjectSystem.ts index 2345cf216a0b9..f3a61a9170bb5 100644 --- a/src/testRunner/unittests/tsserverProjectSystem.ts +++ b/src/testRunner/unittests/tsserverProjectSystem.ts @@ -8830,4 +8830,101 @@ export const x = 10;` assert.equal(moduleInfo.cacheSourceFile.sourceFile.text, updatedModuleContent); }); }); + + describe("tsserverProjectSystem syntax operations", () => { + function navBarFull(session: TestSession, file: File) { + return JSON.stringify(session.executeCommandSeq({ + command: protocol.CommandTypes.NavBarFull, + arguments: { file: file.path } + }).response); + } + + function openFile(session: TestSession, file: File) { + session.executeCommandSeq({ + command: protocol.CommandTypes.Open, + arguments: { file: file.path, fileContent: file.content } + }); + } + + it("works when file is removed and added with different content", () => { + const projectRoot = "/user/username/projects/myproject"; + const app: File = { + path: `${projectRoot}/app.ts`, + content: "console.log('Hello world');" + }; + const unitTest1: File = { + path: `${projectRoot}/unitTest1.ts`, + content: `import assert = require('assert'); + +describe("Test Suite 1", () => { + it("Test A", () => { + assert.ok(true, "This shouldn't fail"); + }); + + it("Test B", () => { + assert.ok(1 === 1, "This shouldn't fail"); + assert.ok(false, "This should fail"); + }); +});` + }; + const tsconfig: File = { + path: `${projectRoot}/tsconfig.json`, + content: "{}" + }; + const files = [app, libFile, tsconfig]; + const host = createServerHost(files); + const session = createSession(host); + const service = session.getProjectService(); + openFile(session, app); + + checkNumberOfProjects(service, { configuredProjects: 1 }); + const project = service.configuredProjects.get(tsconfig.path)!; + const expectedFilesWithoutUnitTest1 = files.map(f => f.path); + checkProjectActualFiles(project, expectedFilesWithoutUnitTest1); + + host.writeFile(unitTest1.path, unitTest1.content); + host.runQueuedTimeoutCallbacks(); + const expectedFilesWithUnitTest1 = expectedFilesWithoutUnitTest1.concat(unitTest1.path); + checkProjectActualFiles(project, expectedFilesWithUnitTest1); + + openFile(session, unitTest1); + checkProjectActualFiles(project, expectedFilesWithUnitTest1); + + const navBarResultUnitTest1 = navBarFull(session, unitTest1); + host.removeFile(unitTest1.path); + host.checkTimeoutQueueLengthAndRun(2); + checkProjectActualFiles(project, expectedFilesWithoutUnitTest1); + + session.executeCommandSeq({ + command: protocol.CommandTypes.Close, + arguments: { file: unitTest1.path } + }); + checkProjectActualFiles(project, expectedFilesWithoutUnitTest1); + + const unitTest1WithChangedContent: File = { + path: unitTest1.path, + content: `import assert = require('assert'); + +export function Test1() { + assert.ok(true, "This shouldn't fail"); +}; + +export function Test2() { + assert.ok(1 === 1, "This shouldn't fail"); + assert.ok(false, "This should fail"); +};` + }; + host.writeFile(unitTest1.path, unitTest1WithChangedContent.content); + host.runQueuedTimeoutCallbacks(); + checkProjectActualFiles(project, expectedFilesWithUnitTest1); + + openFile(session, unitTest1WithChangedContent); + checkProjectActualFiles(project, expectedFilesWithUnitTest1); + const sourceFile = project.getLanguageService().getNonBoundSourceFile(unitTest1WithChangedContent.path); + assert.strictEqual(sourceFile.text, unitTest1WithChangedContent.content); + + const navBarResultUnitTest1WithChangedContent = navBarFull(session, unitTest1WithChangedContent); + assert.notStrictEqual(navBarResultUnitTest1WithChangedContent, navBarResultUnitTest1, "With changes in contents of unitTest file, we should see changed naviagation bar item result"); + }); + }); } diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 489cf717c13b1..d5f37c2aba2ea 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -13319,18 +13319,21 @@ declare namespace ts.server.protocol { } } declare namespace ts.server { + interface ScriptInfoVersion { + svc: number; + text: number; + } class TextStorage { private readonly host; private readonly fileName; + version: ScriptInfoVersion; private svc; - private svcVersion; private text; private lineMap; - private textVersion; isOpen: boolean; private ownFileText; private pendingReloadFromDisk; - constructor(host: ServerHost, fileName: NormalizedPath); + constructor(host: ServerHost, fileName: NormalizedPath, initialVersion?: ScriptInfoVersion); getVersion(): string; hasScriptVersionCache_TestOnly(): boolean; useScriptVersionCache_TestOnly(): void; @@ -13370,7 +13373,8 @@ declare namespace ts.server { readonly isDynamic: boolean; private realpath; cacheSourceFile: DocumentRegistrySourceFileCache; - constructor(host: ServerHost, fileName: NormalizedPath, scriptKind: ScriptKind, hasMixedContent: boolean, path: Path); + constructor(host: ServerHost, fileName: NormalizedPath, scriptKind: ScriptKind, hasMixedContent: boolean, path: Path, initialVersion?: ScriptInfoVersion); + getVersion(): ScriptInfoVersion; isDynamicOrHasMixedContent(): boolean; isScriptOpen(): boolean; open(newText: string): void; From 9f3e5eaaddce416f2e56b8b43a186ab8d1c393e2 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 26 Jun 2018 16:14:27 -0700 Subject: [PATCH 2/2] Use different cache for the ScriptInfoVersion --- src/server/editorServices.ts | 30 ++++++++++--------- .../reference/api/tsserverlibrary.d.ts | 1 + 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 5394d53079d63..af3ddfe33ec8f 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -334,11 +334,6 @@ namespace ts.server { return project.dirty && project.updateGraph(); } - type ScriptInfoOrVersion = ScriptInfo | ScriptInfoVersion; - function isScriptInfoVersion(infoOrVersion: ScriptInfoOrVersion): infoOrVersion is ScriptInfoVersion { - return (infoOrVersion as ScriptInfoVersion).svc !== undefined; - } - export class ProjectService { /*@internal*/ @@ -350,7 +345,13 @@ namespace ts.server { /** * Container of all known scripts */ - private readonly filenameToScriptInfo = createMap(); + private readonly filenameToScriptInfo = createMap(); + /** + * Contains all the deleted script info's version information so that + * it does not reset when creating script info again + * (and could have potentially collided with version where contents mismatch) + */ + private readonly filenameToScriptInfoVersion = createMap(); // Set of all '.js' files ever opened. private readonly allJsFilesForOpenFileTelemetry = createMap(); @@ -889,7 +890,7 @@ namespace ts.server { project.close(); if (Debug.shouldAssert(AssertionLevel.Normal)) { - this.filenameToScriptInfo.forEach(info => Debug.assert(isScriptInfoVersion(info) || !info.isAttached(project))); + this.filenameToScriptInfo.forEach(info => Debug.assert(!info.isAttached(project))); } // Remove the project from pending project updates this.pendingProjectUpdates.delete(project.getProjectName()); @@ -1026,7 +1027,8 @@ namespace ts.server { } private deleteScriptInfo(info: ScriptInfo) { - this.filenameToScriptInfo.set(info.path, info.getVersion()); + this.filenameToScriptInfo.delete(info.path); + this.filenameToScriptInfoVersion.set(info.path, info.getVersion()); const realpath = info.getRealpathIfDifferent(); if (realpath) { this.realpathToScriptInfos!.remove(realpath, info); // TODO: GH#18217 @@ -1855,8 +1857,8 @@ namespace ts.server { private getOrCreateScriptInfoWorker(fileName: NormalizedPath, currentDirectory: string, openedByClient: boolean, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean, hostToQueryFileExistsOn?: { fileExists(path: string): boolean; }) { Debug.assert(fileContent === undefined || openedByClient, "ScriptInfo needs to be opened by client to be able to set its user defined content"); const path = normalizedPathToPath(fileName, currentDirectory, this.toCanonicalFileName); - let info = this.filenameToScriptInfo.get(path); - if (!info || isScriptInfoVersion(info)) { + let info = this.getScriptInfoForPath(path); + if (!info) { const isDynamic = isDynamicFileName(fileName); Debug.assert(isRootedDiskPath(fileName) || isDynamic || openedByClient, "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nScript info with non-dynamic relative file name can only be open script info`); Debug.assert(!isRootedDiskPath(fileName) || this.currentDirectory === currentDirectory || !this.openFilesWithNonRootedDiskPath.has(this.toCanonicalFileName(fileName)), "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nOpen script files with non rooted disk path opened with current directory context cannot have same canonical names`); @@ -1865,8 +1867,9 @@ namespace ts.server { if (!openedByClient && !isDynamic && !(hostToQueryFileExistsOn || this.host).fileExists(fileName)) { return; } - info = new ScriptInfo(this.host, fileName, scriptKind!, !!hasMixedContent, path, info); // TODO: GH#18217 + info = new ScriptInfo(this.host, fileName, scriptKind!, !!hasMixedContent, path, this.filenameToScriptInfoVersion.get(path)); // TODO: GH#18217 this.filenameToScriptInfo.set(info.path, info); + this.filenameToScriptInfoVersion.delete(info.path); if (!openedByClient) { this.watchClosedScriptInfo(info); } @@ -1899,8 +1902,7 @@ namespace ts.server { } getScriptInfoForPath(fileName: Path) { - const info = this.filenameToScriptInfo.get(fileName); - return info && isScriptInfoVersion(info) ? undefined : info; + return this.filenameToScriptInfo.get(fileName); } setHostConfiguration(args: protocol.ConfigureRequestArguments) { @@ -2151,7 +2153,7 @@ namespace ts.server { // It was then postponed to cleanup these script infos so that they can be reused if // the file from that old project is reopened because of opening file from here. this.filenameToScriptInfo.forEach(info => { - if (!isScriptInfoVersion(info) && !info.isScriptOpen() && info.isOrphan()) { + if (!info.isScriptOpen() && info.isOrphan()) { // if there are not projects that include this script info - delete it this.stopWatchingScriptInfo(info); this.deleteScriptInfo(info); diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index d5f37c2aba2ea..3841b80b16b2a 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -13784,6 +13784,7 @@ declare namespace ts.server { readonly typingsCache: TypingsCache; readonly documentRegistry: DocumentRegistry; private readonly filenameToScriptInfo; + private readonly filenameToScriptInfoVersion; private readonly allJsFilesForOpenFileTelemetry; readonly realpathToScriptInfos: MultiMap | undefined; private readonly externalProjectToConfiguredProjectMap;