Proper removal of diagnostics when last project file closed

chenglou · chenglou · commit df72d501125a · 2020-09-20T07:27:49.000-07:00
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -4,7 +4,7 @@ Thanks for your interest. Below is an informal spec of how the plugin's server c
 
 ## Editor Diagnostics
 
-They should be synced in from the `bsb` build. Don't take them from other places.
+They should be synced in from `lib/bs/.compiler.log` build. Don't take them from other places.
 
 ### `.compiler.log`
 
@@ -29,6 +29,8 @@ After saving a file and running the build, the results stream into the log file.
 
 Even this fix isn't great. Ideally, the editor's diagnostics can be greyed out while we're updating them...
 
+Keep in mind that you might be tracking multiple `.compiler.log`s. You should do the above for each.
+
 ### Stale Diagnostics Detection
 
 To check whether the artifacts are stale, do **not** check `.bsb.lock` at the project root. This is unreliable, since it's possible that `bsb` wasn't running in watcher mode. We also don't want to encourage overuse of the watcher mode, though it seems increasingly common.
@@ -43,6 +45,15 @@ The bad alternatives are:
 - Not show that file's project's errors. That's wrong for several reasons (looks like the file has no error, assumes an editor window has a default project, etc.).
 - Show only that file's error. That's just weird, the errors are already read from that project's `.compiler.log`. Might as well show all of them (?).
 
+## Running `bsb` in the Editor
+
+**Don't** do that unless you've prompted the user.
+
+- Running an implicit `bsb -w` automatically means you've acquired the build watch mode lockfile. The user won't be able to run his/her own `bsb -w` in the terminal.
+- Running a one-shot `bsb` doesn't conflict, but is a waste. It's also incorrect, as there might be external file system changes you're not detecting.
+- The build might be a step in a bigger build. The editor running `bsb` implicitly goes against that.
+- If you have multiple files with different project roots open, running all of the `bsb`s is too intense.
+
 ## Format
 
 To find the location of `bsc` to run:
diff --git a/server/src/server.ts b/server/src/server.ts
@@ -1,108 +1,170 @@
-import process from "process";
+import process, { allowedNodeEnvironmentFlags } from "process";
 import * as p from "vscode-languageserver-protocol";
 import * as t from "vscode-languageserver-types";
 import * as j from "vscode-jsonrpc";
 import * as m from "vscode-jsonrpc/lib/messages";
 import * as v from "vscode-languageserver";
 import * as path from 'path';
 import fs from 'fs';
-import { DidOpenTextDocumentNotification, DidChangeTextDocumentNotification, DidCloseTextDocumentNotification } from 'vscode-languageserver-protocol';
+// TODO: check DidChangeWatchedFilesNotification. Check DidChangeTextDocumentNotification. Do they fire on uninitialized files?
+import { DidOpenTextDocumentNotification, DidChangeTextDocumentNotification, DidCloseTextDocumentNotification, DidChangeWatchedFilesNotification, CompletionResolveRequest } from 'vscode-languageserver-protocol';
 import { uriToFsPath, URI } from 'vscode-uri';
 import * as utils from './utils';
 import * as c from './constants';
 import * as chokidar from 'chokidar'
+import { assert } from 'console';
+// TODO: what's this?
+import { fileURLToPath } from 'url';
 
 // https://microsoft.github.io/language-server-protocol/specification#initialize
 // According to the spec, there could be requests before the 'initialize' request. Link in comment tells how to handle them.
 let initialized = false;
 // https://microsoft.github.io/language-server-protocol/specification#exit
 let shutdownRequestAlreadyReceived = false;
-// congrats. A simple UI problem is now a distributed system problem
-let stupidFileContentCache: { [key: string]: string } = {}
-let previouslyDiagnosedFiles: Set<string> = new Set()
-let compilerLogPaths: Set<string> = new Set()
+let stupidFileContentCache: Map<string, string> = new Map()
+/*
+  Map {
+    '/foo/lib/bs/.compiler.log': Map {
+      '/foo/src/A.res': {
+        trackedContent: 'let a = 1',
+        hasDiagnostics: false
+      }
+      '/foo/src/B.res': {
+        trackedContent: null,
+        hasDiagnostics: true
+      }
+    },
+  }
+*/
+let projectsFiles: Map<
+  string,
+  {
+    openFiles: Set<string>,
+    filesWithDiagnostics: Set<string>,
+  }>
+  = new Map()
+// ^ caching AND states AND distributed system. Why does LSP has to be stupid like this
 
 let sendUpdatedDiagnostics = () => {
-  let diagnosedFiles: utils.filesDiagnostics = {}
-  compilerLogPaths.forEach(compilerLogPath => {
+  projectsFiles.forEach(({ filesWithDiagnostics }, compilerLogPath) => {
     let content = fs.readFileSync(compilerLogPath, { encoding: 'utf-8' });
-    console.log("new log content: ", content)
-    let { done: buildDone, result: filesAndErrors } = utils.parseCompilerLogOutput(content)
-    // A file can only belong to one .compiler.log root. So we're not overriding
-    // any existing key here
-    Object.assign(diagnosedFiles, filesAndErrors)
-  });
+    console.log("new log content: ", compilerLogPath, content)
+    let { done, result: filesAndErrors } = utils.parseCompilerLogOutput(content)
 
-  // Send new diagnostic, wipe old ones
-  let diagnosedFilePaths = Object.keys(diagnosedFiles)
-  diagnosedFilePaths.forEach(file => {
-    let params: p.PublishDiagnosticsParams = {
-      uri: file,
-      // there's a new optional version param from https://github.com/microsoft/language-server-protocol/issues/201
-      // not using it for now, sigh
-      diagnostics: diagnosedFiles[file],
-    }
-    let notification: m.NotificationMessage = {
-      jsonrpc: c.jsonrpcVersion,
-      method: 'textDocument/publishDiagnostics',
-      params: params,
-    };
-    process.send!(notification);
+    // diff
+    Object.keys(filesAndErrors).forEach(file => {
+      // send diagnostic
+      let params: p.PublishDiagnosticsParams = {
+        uri: file,
+        diagnostics: filesAndErrors[file],
+      }
+      let notification: m.NotificationMessage = {
+        jsonrpc: c.jsonrpcVersion,
+        method: 'textDocument/publishDiagnostics',
+        params: params,
+      };
+      process.send!(notification);
 
-    // this file's taken care of already now. Remove from old diagnostic files
-    previouslyDiagnosedFiles.delete(file)
-  })
-  // wipe the errors from the files that are no longer erroring
-  previouslyDiagnosedFiles.forEach(remainingPreviousFile => {
-    let params: p.PublishDiagnosticsParams = {
-      uri: remainingPreviousFile,
-      diagnostics: [],
+      filesWithDiagnostics.add(file)
+    })
+    if (done) {
+      // clear old files
+      filesWithDiagnostics.forEach(file => {
+        if (filesAndErrors[file] == null) {
+          // Doesn't exist in the new diagnostics. Clear this diagnostic
+          let params: p.PublishDiagnosticsParams = {
+            uri: file,
+            diagnostics: [],
+          }
+          let notification: m.NotificationMessage = {
+            jsonrpc: c.jsonrpcVersion,
+            method: 'textDocument/publishDiagnostics',
+            params: params,
+          };
+          process.send!(notification);
+          filesWithDiagnostics.delete(file)
+        }
+      })
     }
-    let notification: m.NotificationMessage = {
-      jsonrpc: c.jsonrpcVersion,
-      method: 'textDocument/publishDiagnostics',
-      params: params,
-    };
-    process.send!(notification);
-  })
-  previouslyDiagnosedFiles = new Set(diagnosedFilePaths)
+  });
+}
+let deleteProjectDiagnostics = (compilerLogPath: string) => {
+  let compilerLog = projectsFiles.get(compilerLogPath)
+  if (compilerLog != null) {
+    compilerLog.filesWithDiagnostics.forEach(file => {
+      let params: p.PublishDiagnosticsParams = {
+        uri: file,
+        diagnostics: [],
+      }
+      let notification: m.NotificationMessage = {
+        jsonrpc: c.jsonrpcVersion,
+        method: 'textDocument/publishDiagnostics',
+        params: params,
+      };
+      process.send!(notification);
+    })
+
+    projectsFiles.delete(compilerLogPath)
+  }
 }
 
 let compilerLogsWatcher = chokidar.watch([])
   .on('all', (_e, changedPath) => {
     console.log('new log change', changedPath, Math.random())
     sendUpdatedDiagnostics()
   })
+let stopWatchingCompilerLog = () => {
+  // TODO: cleanup of compilerLogs?
+  compilerLogsWatcher.close()
+}
 
-let addCompilerLogToWatch = (fileUri: string) => {
+let openedFile = (fileUri: string, fileContent: string) => {
   let filePath = uriToFsPath(URI.parse(fileUri), true);
+
+  stupidFileContentCache.set(filePath, fileContent)
+
   let compilerLogDir = utils.findDirOfFileNearFile(c.compilerLogPartialPath, filePath)
   if (compilerLogDir != null) {
     let compilerLogPath = path.join(compilerLogDir, c.compilerLogPartialPath);
-    if (!compilerLogPaths.has(compilerLogPath)) {
-      console.log("added new ", compilerLogPath, "from file: ", compilerLogDir)
-      compilerLogPaths.add(compilerLogPath)
+    if (!projectsFiles.has(compilerLogPath)) {
+      projectsFiles.set(compilerLogPath, { openFiles: new Set(), filesWithDiagnostics: new Set() })
       compilerLogsWatcher.add(compilerLogPath)
-      // no need to call sendUpdatedDiagnostics() here; the watcher add will
-      // call the listener which calls it
     }
+    let compilerLog = projectsFiles.get(compilerLogPath)!
+    compilerLog.openFiles.add(filePath)
+    // no need to call sendUpdatedDiagnostics() here; the watcher add will
+    // call the listener which calls it
   }
 }
-let removeCompilerLogToWatch = (fileUri: string) => {
+let closedFile = (fileUri: string) => {
   let filePath = uriToFsPath(URI.parse(fileUri), true);
+
+  stupidFileContentCache.delete(filePath)
+
   let compilerLogDir = utils.findDirOfFileNearFile(c.compilerLogPartialPath, filePath)
   if (compilerLogDir != null) {
     let compilerLogPath = path.join(compilerLogDir, c.compilerLogPartialPath);
-    if (compilerLogPaths.has(compilerLogPath)) {
-      console.log("remove log path ", compilerLogPath)
-      compilerLogPaths.delete(compilerLogPath)
-      compilerLogsWatcher.unwatch(compilerLogPath)
-      sendUpdatedDiagnostics()
+    let compilerLog = projectsFiles.get(compilerLogPath)
+    if (compilerLog != null) {
+      compilerLog.openFiles.delete(filePath)
+      // clear diagnostics too if no open files open in said project
+      if (compilerLog.openFiles.size === 0) {
+        compilerLogsWatcher.unwatch(compilerLogPath)
+        deleteProjectDiagnostics(compilerLogPath)
+      }
     }
   }
 }
-let stopWatchingCompilerLog = () => {
-  compilerLogsWatcher.close()
+let updateOpenedFile = (fileUri: string, fileContent: string) => {
+  let filePath = uriToFsPath(URI.parse(fileUri), true)
+  assert(stupidFileContentCache.has(filePath))
+  stupidFileContentCache.set(filePath, fileContent)
+}
+let getOpenedFileContent = (fileUri: string) => {
+  let filePath = uriToFsPath(URI.parse(fileUri), true)
+  let content = stupidFileContentCache.get(filePath)!
+  assert(content != null)
+  return content
 }
 
 process.on('message', (a: (m.RequestMessage | m.NotificationMessage)) => {
@@ -125,8 +187,7 @@ process.on('message', (a: (m.RequestMessage | m.NotificationMessage)) => {
       let extName = path.extname(params.textDocument.uri)
       if (extName === c.resExt || extName === c.resiExt) {
         console.log("new file coming", params.textDocument.uri)
-        stupidFileContentCache[params.textDocument.uri] = params.textDocument.text;
-        addCompilerLogToWatch(params.textDocument.uri)
+        openedFile(params.textDocument.uri, params.textDocument.text)
       }
     } else if (aa.method === DidChangeTextDocumentNotification.method) {
       let params = (aa.params as p.DidChangeTextDocumentParams);
@@ -137,13 +198,12 @@ process.on('message', (a: (m.RequestMessage | m.NotificationMessage)) => {
           // no change?
         } else {
           // we currently only support full changes
-          stupidFileContentCache[params.textDocument.uri] = changes[changes.length - 1].text;
+          updateOpenedFile(params.textDocument.uri, changes[changes.length - 1].text)
         }
       }
     } else if (aa.method === DidCloseTextDocumentNotification.method) {
       let params = (aa.params as p.DidCloseTextDocumentParams);
-      delete stupidFileContentCache[params.textDocument.uri];
-      removeCompilerLogToWatch(params.textDocument.uri)
+      closedFile(params.textDocument.uri)
     }
   } else {
     // this is a request message, aka client sent request, waits for our reply
@@ -235,7 +295,7 @@ process.on('message', (a: (m.RequestMessage | m.NotificationMessage)) => {
           process.send!(response);
         } else {
           // code will always be defined here, even though technically it can be undefined
-          let code = stupidFileContentCache[params.textDocument.uri];
+          let code = getOpenedFileContent(params.textDocument.uri)
           let formattedResult = utils.formatUsingValidBscPath(
             code,
             path.join(nodeModulesParentPath, c.bscPartialPath),
diff --git a/server/src/utils.ts b/server/src/utils.ts
@@ -88,7 +88,7 @@ export let parseDiagnosticLocation = (location: string): Range => {
 	}
 }
 
-export type filesDiagnostics = {
+type filesDiagnostics = {
 	[key: string]: p.Diagnostic[];
 }
 type parsedCompilerLogResult = {

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ export let parseDiagnosticLocation = (location: string): Range => {`
`88`	`88`	`}`
`89`	`89`	`}`
`90`	`90`
`91`		`-export type filesDiagnostics = {`
	`91`	`+type filesDiagnostics = {`
`92`	`92`	`[key: string]: p.Diagnostic[];`
`93`	`93`	`}`
`94`	`94`	`type parsedCompilerLogResult = {`