From 27988bf33a7b7c497bbb208c4300cd404ee891b8 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 7 Aug 2017 14:47:32 -0700 Subject: [PATCH] More updates based on PR feedback --- .../unittests/tsserverProjectSystem.ts | 65 ++++++--- src/server/editorServices.ts | 135 ++++++++++-------- src/server/lsHost.ts | 10 +- 3 files changed, 128 insertions(+), 82 deletions(-) diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index 17953a8bcd5..7d70aa74fdf 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -407,17 +407,20 @@ namespace ts.projectSystem { // Update file if (currentEntry.content !== fileOrFolder.content) { currentEntry.content = fileOrFolder.content; + currentEntry.fileSize = fileOrFolder.fileSize; this.invokeFileWatcher(currentEntry.fullPath, FileWatcherEventKind.Changed); } } else { // TODO: Changing from file => folder + Debug.fail(`Currently ${path} is file and new FS makes it folder which isnt supported yet`); } } else { // Folder if (typeof fileOrFolder.content === "string") { // TODO: Changing from folder => file + Debug.fail(`Currently ${path} is folder and new FS makes it file which isnt supported yet`); } else { // Folder update: Nothing to do. @@ -778,6 +781,20 @@ namespace ts.projectSystem { } } + type ErrorInformation = { diagnosticMessage: DiagnosticMessage, errorTextArguments?: string[] }; + function getProtocolDiagnosticMessage({ diagnosticMessage, errorTextArguments = [] }: ErrorInformation) { + return formatStringFromArgs(diagnosticMessage.message, errorTextArguments); + } + + function verifyDiagnostics(actual: server.protocol.Diagnostic[], expected: ErrorInformation[]) { + const expectedErrors = expected.map(getProtocolDiagnosticMessage); + assert.deepEqual(actual.map(diag => flattenDiagnosticMessageText(diag.text, "\n")), expectedErrors); + } + + function verifyNoDiagnostics(actual: server.protocol.Diagnostic[]) { + verifyDiagnostics(actual, []); + } + describe("tsserver-project-system", () => { const commonFile1: FileOrFolder = { path: "/a/b/commonFile1.ts", @@ -1111,10 +1128,13 @@ namespace ts.projectSystem { server.CommandNames.SemanticDiagnosticsSync, { file: file1.path } ); - let diags = session.executeCommand(getErrRequest).response; // Two errors: CommonFile2 not found and cannot find name y - assert.equal(diags.length, 2, diags.map(diag => flattenDiagnosticMessageText(diag.text, "\n")).join("\n")); + let diags: server.protocol.Diagnostic[] = session.executeCommand(getErrRequest).response; + verifyDiagnostics(diags, [ + { diagnosticMessage: Diagnostics.Cannot_find_name_0, errorTextArguments: ["y"] }, + { diagnosticMessage: Diagnostics.File_0_not_found, errorTextArguments: [commonFile2.path] } + ]); host.reloadFS([file1, commonFile2, libFile]); host.runQueuedTimeoutCallbacks(); @@ -1122,8 +1142,8 @@ namespace ts.projectSystem { assert.strictEqual(projectService.inferredProjects[0], project, "Inferred project should be same"); checkProjectRootFiles(project, [file1.path]); checkProjectActualFiles(project, [file1.path, libFile.path, commonFile2.path]); - diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + diags = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); }); it("should create new inferred projects for files excluded from a configured project", () => { @@ -3110,15 +3130,18 @@ namespace ts.projectSystem { server.CommandNames.SemanticDiagnosticsSync, { file: file1.path } ); - let diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + let diags: server.protocol.Diagnostic[] = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); const moduleFileOldPath = moduleFile.path; const moduleFileNewPath = "/a/b/moduleFile1.ts"; moduleFile.path = moduleFileNewPath; host.reloadFS([moduleFile, file1]); host.runQueuedTimeoutCallbacks(); - diags = session.executeCommand(getErrRequest).response; + diags = session.executeCommand(getErrRequest).response; + verifyDiagnostics(diags, [ + { diagnosticMessage: Diagnostics.Cannot_find_module_0, errorTextArguments: ["./moduleFile"] } + ]); assert.equal(diags.length, 1); moduleFile.path = moduleFileOldPath; @@ -3133,8 +3156,8 @@ namespace ts.projectSystem { session.executeCommand(changeRequest); host.runQueuedTimeoutCallbacks(); - diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + diags = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); }); it("should restore the states for configured projects", () => { @@ -3158,22 +3181,24 @@ namespace ts.projectSystem { server.CommandNames.SemanticDiagnosticsSync, { file: file1.path } ); - let diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + let diags: server.protocol.Diagnostic[] = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); const moduleFileOldPath = moduleFile.path; const moduleFileNewPath = "/a/b/moduleFile1.ts"; moduleFile.path = moduleFileNewPath; host.reloadFS([moduleFile, file1, configFile]); host.runQueuedTimeoutCallbacks(); - diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 1); + diags = session.executeCommand(getErrRequest).response; + verifyDiagnostics(diags, [ + { diagnosticMessage: Diagnostics.Cannot_find_module_0, errorTextArguments: ["./moduleFile"] } + ]); moduleFile.path = moduleFileOldPath; host.reloadFS([moduleFile, file1, configFile]); host.runQueuedTimeoutCallbacks(); - diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + diags = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); }); it("should property handle missing config files", () => { @@ -3239,8 +3264,10 @@ namespace ts.projectSystem { server.CommandNames.SemanticDiagnosticsSync, { file: file1.path } ); - let diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 1); + let diags: server.protocol.Diagnostic[] = session.executeCommand(getErrRequest).response; + verifyDiagnostics(diags, [ + { diagnosticMessage: Diagnostics.Cannot_find_module_0, errorTextArguments: ["./moduleFile"] } + ]); host.reloadFS([file1, moduleFile]); host.runQueuedTimeoutCallbacks(); @@ -3253,8 +3280,8 @@ namespace ts.projectSystem { session.executeCommand(changeRequest); // Recheck - diags = session.executeCommand(getErrRequest).response; - assert.equal(diags.length, 0); + diags = session.executeCommand(getErrRequest).response; + verifyNoDiagnostics(diags); }); }); diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index b71bc939097..2d41a3ccb30 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -283,10 +283,17 @@ namespace ts.server { type ConfigFileExistence = { /** * Cached value of existence of config file + * It is true if there is configured project open for this file. + * It can be either true or false if this is the config file that is being watched by inferred project + * to decide when to update the structure so that it knows about updating the project for its files + * (config file may include the inferred project files after the change and hence may be wont need to be in inferred project) */ exists: boolean; /** - * The value in the open files map is true if the file is inferred project root + * The value in the trackingOpenFilesMap is true if the open file is inferred project root + * and hence tracking changes to this config file + * (either config file is already present but doesnt include the open file in the project structure or config file doesnt exist) + * * Otherwise its false */ trackingOpenFilesMap: Map; @@ -345,7 +352,14 @@ namespace ts.server { private compilerOptionsForInferredProjects: CompilerOptions; private compileOnSaveForInferredProjects: boolean; private readonly projectToSizeMap: Map = createMap(); - private readonly mapOfConfigFilePresence: Map; + /** + * This is a map of config file paths existance that doesnt need query to disk + * - The entry can be present because there is inferred project that needs to watch addition of config file to folder + * In this case the exists could be true/false based on config file is present or not + * - Or it is present if we have configured project open with config file at that location + * In this case the exists property is always true + */ + private readonly mapOfConfigFilePresence = createMap(); private readonly throttledOperations: ThrottledOperations; private readonly hostConfiguration: HostConfiguration; @@ -389,7 +403,6 @@ namespace ts.server { this.currentDirectory = this.host.getCurrentDirectory(); this.toCanonicalFileName = createGetCanonicalFileName(this.host.useCaseSensitiveFileNames); - this.mapOfConfigFilePresence = createMap(); this.throttledOperations = new ThrottledOperations(this.host); this.typingsInstaller.attach(this); @@ -415,7 +428,7 @@ namespace ts.server { } ensureInferredProjectsUpToDate_TestOnly() { - this.ensureInferredProjectsUpToDate(); + this.ensureProjectStructuresUptoDate(); } getCompilerOptionsForInferredProjects() { @@ -505,7 +518,7 @@ namespace ts.server { return undefined; } if (isInferredProjectName(projectName)) { - this.ensureInferredProjectsUpToDate(); + this.ensureProjectStructuresUptoDate(); return findProjectByName(projectName, this.inferredProjects); } return this.findExternalProjectByProjectName(projectName) || this.findConfiguredProjectByProjectName(toNormalizedPath(projectName)); @@ -513,7 +526,7 @@ namespace ts.server { getDefaultProjectForFile(fileName: NormalizedPath, refreshInferredProjects: boolean) { if (refreshInferredProjects) { - this.ensureInferredProjectsUpToDate(); + this.ensureProjectStructuresUptoDate(); } const scriptInfo = this.getScriptInfoForNormalizedPath(fileName); return scriptInfo && scriptInfo.getDefaultProject(); @@ -521,9 +534,16 @@ namespace ts.server { /** * Ensures the project structures are upto date - * @param refreshInferredProjects when true updates the inferred projects even if there is no pending work + * This means, + * - if there are changedFiles (the files were updated but their containing project graph was not upto date), + * their project graph is updated + * - If there are pendingProjectUpdates (scheduled to be updated with delay so they can batch update the graph if there are several changes in short time span) + * their project graph is updated + * - If there were project graph updates and/or there was pending inferred project update and/or called forced the inferred project structure refresh + * Inferred projects are created/updated/deleted based on open files states + * @param forceInferredProjectsRefresh when true updates the inferred projects even if there is no pending work to update the files/project structures */ - private ensureInferredProjectsUpToDate(refreshInferredProjects?: boolean) { + private ensureProjectStructuresUptoDate(forceInferredProjectsRefresh?: boolean) { if (this.changedFiles) { let projectsToUpdate: Project[]; if (this.changedFiles.length === 1) { @@ -546,7 +566,7 @@ namespace ts.server { this.updateProjectGraphs(projectsToUpdate); } - if (this.pendingInferredProjectUpdate || refreshInferredProjects) { + if (this.pendingInferredProjectUpdate || forceInferredProjectsRefresh) { this.pendingInferredProjectUpdate = false; this.refreshInferredProjects(); } @@ -584,26 +604,22 @@ namespace ts.server { const info = this.getScriptInfoForNormalizedPath(fileName); if (!info) { this.logger.info(`Error: got watch notification for unknown file: ${fileName}`); - return; } - - if (eventKind === FileWatcherEventKind.Deleted) { + else if (eventKind === FileWatcherEventKind.Deleted) { // File was deleted this.handleDeletedFile(info); } - else { - if (!info.isScriptOpen()) { - if (info.containingProjects.length === 0) { - // Orphan script info, remove it as we can always reload it on next open file request - this.stopWatchingScriptInfo(info, WatcherCloseReason.OrphanScriptInfoWithChange); - this.filenameToScriptInfo.delete(info.path); - } - else { - // file has been changed which might affect the set of referenced files in projects that include - // this file and set of inferred projects - info.reloadFromFile(); - this.delayUpdateProjectGraphs(info.containingProjects); - } + else if (!info.isScriptOpen()) { + if (info.containingProjects.length === 0) { + // Orphan script info, remove it as we can always reload it on next open file request + this.stopWatchingScriptInfo(info, WatcherCloseReason.OrphanScriptInfoWithChange); + this.filenameToScriptInfo.delete(info.path); + } + else { + // file has been changed which might affect the set of referenced files in projects that include + // this file and set of inferred projects + info.reloadFromFile(); + this.delayUpdateProjectGraphs(info.containingProjects); } } } @@ -821,7 +837,7 @@ namespace ts.server { this.removeProject(project); } - // collect orphanted files and try to re-add them as newly opened + // collect orphaned files and try to re-add them as newly opened // treat orphaned files as newly opened // for all open files for (const f of this.openFiles) { @@ -866,7 +882,7 @@ namespace ts.server { return configFilePresenceInfo.exists; } - // Theorotically we should be adding watch for the directory here itself. + // Theoretically we should be adding watch for the directory here itself. // In practice there will be very few scenarios where the config file gets added // somewhere inside the another config file directory. // And technically we could handle that case in configFile's directory watcher in some cases @@ -935,25 +951,26 @@ namespace ts.server { } private logConfigFileWatchUpdate(configFileName: NormalizedPath, configFilePresenceInfo: ConfigFileExistence, status: ConfigFileWatcherStatus) { - if (this.logger.loggingEnabled()) { - const inferredRoots: string[] = []; - const otherFiles: string[] = []; - configFilePresenceInfo.trackingOpenFilesMap.forEach((value, key: Path) => { - const info = this.getScriptInfoForPath(key); - if (value) { - inferredRoots.push(info.fileName); - } - else { - otherFiles.push(info.fileName); - } - }); - const watchType = status === ConfigFileWatcherStatus.UpdatedCallback || - status === ConfigFileWatcherStatus.ReloadingFiles || - status === ConfigFileWatcherStatus.ReloadingInferredRootFiles ? - (configFilePresenceInfo.configFileWatcher ? WatchType.ConfigFileForInferredRoot : WatchType.ConfigFilePath) : - ""; - this.logger.info(`ConfigFilePresence ${watchType}:: File: ${configFileName} Currently Tracking: InferredRootFiles: ${inferredRoots} OtherFiles: ${otherFiles} Status: ${status}`); + if (!this.logger.loggingEnabled()) { + return; } + const inferredRoots: string[] = []; + const otherFiles: string[] = []; + configFilePresenceInfo.trackingOpenFilesMap.forEach((value, key: Path) => { + const info = this.getScriptInfoForPath(key); + if (value) { + inferredRoots.push(info.fileName); + } + else { + otherFiles.push(info.fileName); + } + }); + const watchType = status === ConfigFileWatcherStatus.UpdatedCallback || + status === ConfigFileWatcherStatus.ReloadingFiles || + status === ConfigFileWatcherStatus.ReloadingInferredRootFiles ? + (configFilePresenceInfo.configFileWatcher ? WatchType.ConfigFileForInferredRoot : WatchType.ConfigFilePath) : + ""; + this.logger.info(`ConfigFilePresence ${watchType}:: File: ${configFileName} Currently Tracking: InferredRootFiles: ${inferredRoots} OtherFiles: ${otherFiles} Status: ${status}`); } private closeConfigFileWatcherIfInferredRoot(configFileName: NormalizedPath, canonicalConfigFilePath: string, @@ -998,7 +1015,7 @@ namespace ts.server { */ private stopWatchingConfigFilesForClosedScriptInfo(info: ScriptInfo) { Debug.assert(!info.isScriptOpen()); - this.enumerateConfigFileLocations(info, (configFileName, canonicalConfigFilePath) => + this.forEachConfigFileLocation(info, (configFileName, canonicalConfigFilePath) => this.closeConfigFileWatchForClosedScriptInfo(configFileName, canonicalConfigFilePath, info) ); } @@ -1034,7 +1051,7 @@ namespace ts.server { /* @internal */ startWatchingConfigFilesForInferredProjectRoot(info: ScriptInfo) { Debug.assert(info.isScriptOpen()); - this.enumerateConfigFileLocations(info, (configFileName, canonicalConfigFilePath) => + this.forEachConfigFileLocation(info, (configFileName, canonicalConfigFilePath) => this.watchConfigFileForInferredProjectRoot(configFileName, canonicalConfigFilePath, info) ); } @@ -1060,7 +1077,7 @@ namespace ts.server { */ /* @internal */ stopWatchingConfigFilesForInferredProjectRoot(info: ScriptInfo, reason: WatcherCloseReason) { - this.enumerateConfigFileLocations(info, (configFileName, canonicalConfigFilePath) => + this.forEachConfigFileLocation(info, (configFileName, canonicalConfigFilePath) => this.closeWatchConfigFileForInferredProjectRoot(configFileName, canonicalConfigFilePath, info, reason) ); } @@ -1073,7 +1090,7 @@ namespace ts.server { * The server must start searching from the directory containing * the newly opened file. */ - private enumerateConfigFileLocations(info: ScriptInfo, + private forEachConfigFileLocation(info: ScriptInfo, action: (configFileName: NormalizedPath, canonicalConfigFilePath: string) => boolean | void, projectRootPath?: NormalizedPath) { let searchPath = asNormalizedPath(getDirectoryPath(info.fileName)); @@ -1113,8 +1130,8 @@ namespace ts.server { private getConfigFileNameForFile(info: ScriptInfo, projectRootPath?: NormalizedPath) { Debug.assert(info.isScriptOpen()); this.logger.info(`Search path: ${getDirectoryPath(info.fileName)}`); - const configFileName = this.enumerateConfigFileLocations(info, - (configFileName: NormalizedPath, canonicalConfigFilePath: string) => + const configFileName = this.forEachConfigFileLocation(info, + (configFileName, canonicalConfigFilePath) => this.configFileExists(configFileName, canonicalConfigFilePath, info), projectRootPath ); @@ -1684,16 +1701,18 @@ namespace ts.server { } /** - * This function is to update the project structure for every projects. + * This function is to update the project structure for every inferred project. * It is called on the premise that all the configured projects are * up to date. + * This will go through open files and assign them to inferred project if open file is not part of any other project + * After that all the inferred project graphs are updated */ private refreshInferredProjects() { this.logger.info("refreshInferredProjects: updating project structure from ..."); this.printProjects(); for (const info of this.openFiles) { - // collect all orphanted script infos from open files + // collect all orphaned script infos from open files if (info.containingProjects.length === 0) { this.assignScriptInfoToInferredProjectIfNecessary(info, /*addToListOfOpenFiles*/ false); } @@ -1826,7 +1845,7 @@ namespace ts.server { // if files were open or closed then explicitly refresh list of inferred projects // otherwise if there were only changes in files - record changed files in `changedFiles` and defer the update if (openFiles || closedFiles) { - this.ensureInferredProjectsUpToDate(/*refreshInferredProjects*/ true); + this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true); } } @@ -1849,7 +1868,7 @@ namespace ts.server { } this.externalProjectToConfiguredProjectMap.delete(fileName); if (shouldRefreshInferredProjects && !suppressRefresh) { - this.ensureInferredProjectsUpToDate(/*refreshInferredProjects*/ true); + this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true); } } else { @@ -1858,7 +1877,7 @@ namespace ts.server { if (externalProject) { this.removeProject(externalProject); if (!suppressRefresh) { - this.ensureInferredProjectsUpToDate(/*refreshInferredProjects*/ true); + this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true); } } } @@ -1882,7 +1901,7 @@ namespace ts.server { this.closeExternalProject(externalProjectName, /*suppressRefresh*/ true); }); - this.ensureInferredProjectsUpToDate(/*refreshInferredProjects*/ true); + this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true); } /** Makes a filename safe to insert in a RegExp */ @@ -2077,7 +2096,7 @@ namespace ts.server { this.createAndAddExternalProject(proj.projectFileName, rootFiles, proj.options, proj.typeAcquisition); } if (!suppressRefreshOfInferredProjects) { - this.ensureInferredProjectsUpToDate(/*refreshInferredProjects*/ true); + this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true); } } } diff --git a/src/server/lsHost.ts b/src/server/lsHost.ts index fb1cd80b660..3bba5513227 100644 --- a/src/server/lsHost.ts +++ b/src/server/lsHost.ts @@ -154,19 +154,19 @@ namespace ts.server { const path = this.toPath(fileOrFolder); const existingResult = this.cachedReadDirectoryResult.get(path); if (existingResult) { + // This was a folder already present, remove it if this doesnt exist any more if (!this.host.directoryExists(fileOrFolder)) { this.cachedReadDirectoryResult.delete(path); } } else { - // Was this earlier file + // This was earlier a file (hence not in cached directory contents) + // or we never cached the directory containing it const parentResult = this.cachedReadDirectoryResult.get(getDirectoryPath(path)); if (parentResult) { const baseName = getBaseFileName(fileOrFolder); - if (parentResult) { - parentResult.files = this.updateFileSystemEntry(parentResult.files, baseName, this.host.fileExists(path)); - parentResult.directories = this.updateFileSystemEntry(parentResult.directories, baseName, this.host.directoryExists(path)); - } + parentResult.files = this.updateFileSystemEntry(parentResult.files, baseName, this.host.fileExists(path)); + parentResult.directories = this.updateFileSystemEntry(parentResult.directories, baseName, this.host.directoryExists(path)); } } }