Diagnostic diff (#204)

* Fix circular reference issue with Callable.ts

* Only send changes to diagnostics to client.

* Removed exclusive test

src/DiagnosticCollection.spec.ts

@@ -0,0 +1,111 @@
+import { BsDiagnostic } from '.';
+import { DiagnosticCollection } from './DiagnosticCollection';
+import { Workspace } from './LanguageServer';
+import { ProgramBuilder } from './ProgramBuilder';
+import { File } from './interfaces';
+import util from './util';
+import { expect } from 'chai';
+
+describe('DiagnosticCollection', () => {
+    let collection: DiagnosticCollection;
+    let diagnostics: BsDiagnostic[];
+    let workspaces: Workspace[];
+    beforeEach(() => {
+        collection = new DiagnosticCollection();
+        diagnostics = [];
+        //make simple mock of workspace to pass tests
+        workspaces = [{
+            firstRunPromise: Promise.resolve(),
+            builder: {
+                getDiagnostics: () => diagnostics
+            } as ProgramBuilder
+        }] as Workspace[];
+    });
+
+    async function testPatch(expected: { [filePath: string]: string[] }) {
+        const patch = await collection.getPatch(workspaces);
+        //convert the patch into our test structure
+        const actual = {};
+        for (const filePath in patch) {
+            actual[filePath] = patch[filePath].map(x => x.message);
+        }
+
+        expect(actual).to.eql(expected);
+    }
+
+    it('returns full list of diagnostics on first call, and nothing on second call', async () => {
+        addDiagnostics('file1.brs', ['message1', 'message2']);
+        addDiagnostics('file2.brs', ['message3', 'message4']);
+        //first patch should return all
+        await testPatch({
+            'file1.brs': ['message1', 'message2'],
+            'file2.brs': ['message3', 'message4']
+        });
+
+        //second patch should return empty (because nothing has changed)
+        await testPatch({});
+    });
+
+    it('removes diagnostics in patch', async () => {
+        addDiagnostics('file1.brs', ['message1', 'message2']);
+        addDiagnostics('file2.brs', ['message3', 'message4']);
+        //first patch should return all
+        await testPatch({
+            'file1.brs': ['message1', 'message2'],
+            'file2.brs': ['message3', 'message4']
+        });
+        removeDiagnostic('file1.brs', 'message1');
+        removeDiagnostic('file1.brs', 'message2');
+        await testPatch({
+            'file1.brs': []
+        });
+    });
+
+    it('adds diagnostics in patch', async () => {
+        addDiagnostics('file1.brs', ['message1', 'message2']);
+        await testPatch({
+            'file1.brs': ['message1', 'message2']
+        });
+
+        addDiagnostics('file2.brs', ['message3', 'message4']);
+        await testPatch({
+            'file2.brs': ['message3', 'message4']
+        });
+    });
+
+    it('sends full list when file diagnostics have changed', async () => {
+        addDiagnostics('file1.brs', ['message1', 'message2']);
+        await testPatch({
+            'file1.brs': ['message1', 'message2']
+        });
+        addDiagnostics('file1.brs', ['message3', 'message4']);
+        await testPatch({
+            'file1.brs': ['message1', 'message2', 'message3', 'message4']
+        });
+    });
+
+    function removeDiagnostic(filePath: string, message: string) {
+        for (let i = 0; i < diagnostics.length; i++) {
+            const diagnostic = diagnostics[i];
+            if (diagnostic.file.pathAbsolute === filePath && diagnostic.message === message) {
+                diagnostics.splice(i, 1);
+                return;
+            }
+        }
+        throw new Error(`Cannot find diagnostic ${filePath}:${message}`);
+    }
+
+    function addDiagnostics(filePath: string, messages: string[]) {
+        for (const message of messages) {
+            diagnostics.push({
+                file: {
+                    pathAbsolute: filePath
+                } as File,
+                range: util.createRange(0, 0, 0, 0),
+                //the code doesn't matter as long as the messages are different, so just enforce unique messages for this test files
+                code: 123,
+                message: message
+            });
+        }
+    }
+});

src/DiagnosticCollection.ts

@@ -0,0 +1,123 @@
+import { BsDiagnostic } from './interfaces';
+import { Workspace } from './LanguageServer';
+
+export class DiagnosticCollection {
+    private previousDiagnosticsByFile = {} as { [fileSrcPathLower: string]: KeyedDiagnostic[] };
+
+    public async getPatch(workspaces: Workspace[]): Promise<{ [fileSrcPathLower: string]: KeyedDiagnostic[] }> {
+        const diagnosticsByFile = await this.getDiagnosticsByFileFromWorkspaces(workspaces);
+
+        const patch = {
+            ...this.getRemovedPatch(diagnosticsByFile),
+            ...this.getModifiedPatch(diagnosticsByFile),
+            ...this.getAddedPatch(diagnosticsByFile)
+        };
+
+        //save the new list of diagnostics
+        this.previousDiagnosticsByFile = diagnosticsByFile;
+        return patch;
+    }
+
+    private async getDiagnosticsByFileFromWorkspaces(workspaces: Workspace[]) {
+        const result = {} as { [fileSrcPathLower: string]: KeyedDiagnostic[] };
+
+        //wait for all programs to finish running. This ensures the `Program` exists.
+        await Promise.all(
+            workspaces.map(x => x.firstRunPromise)
+        );
+
+        //get all diagnostics for all workspaces
+        let diagnostics = Array.prototype.concat.apply([] as KeyedDiagnostic[],
+            workspaces.map((x) => x.builder.getDiagnostics())
+        ) as KeyedDiagnostic[];
+
+        const keys = {};
+        //build the full current set of diagnostics by file
+        for (let diagnostic of diagnostics) {
+            const filePath = diagnostic.file.pathAbsolute;
+            //ensure the file entry exists
+            if (!result[filePath]) {
+                result[filePath] = [];
+            }
+            const diagnosticMap = result[filePath];
+
+            diagnostic.key =
+                diagnostic.file.pathAbsolute.toLowerCase() + '-' +
+                diagnostic.code + '-' +
+                diagnostic.range.start.line + '-' +
+                diagnostic.range.start.character + '-' +
+                diagnostic.range.end.line + '-' +
+                diagnostic.range.end.character +
+                diagnostic.message;
+
+            //don't include duplicates
+            if (!keys[diagnostic.key]) {
+                keys[diagnostic.key] = true;
+                diagnosticMap.push(diagnostic);
+            }
+        }
+        return result;
+    }
+
+    /**
+     * Get a patch for all the files that have been removed since last time
+     */
+    private getRemovedPatch(currentDiagnosticsByFile: { [fileSrcPathLower: string]: KeyedDiagnostic[] }) {
+        const result = {} as { [fileSrcPathLower: string]: KeyedDiagnostic[] };
+        for (const filePath in this.previousDiagnosticsByFile) {
+            if (!currentDiagnosticsByFile[filePath]) {
+                result[filePath] = [];
+            }
+        }
+        return result;
+    }
+
+    /**
+     * Get all files whose diagnostics have changed since last time
+     */
+    private getModifiedPatch(currentDiagnosticsByFile: { [fileSrcPathLower: string]: KeyedDiagnostic[] }) {
+        const result = {} as { [fileSrcPathLower: string]: KeyedDiagnostic[] };
+        for (const filePath in currentDiagnosticsByFile) {
+            //for this file, if there were diagnostics last time AND there are diagnostics this time, and the lists are different
+            if (this.previousDiagnosticsByFile[filePath] && !this.diagnosticListsAreIdentical(this.previousDiagnosticsByFile[filePath], currentDiagnosticsByFile[filePath])) {
+                result[filePath] = currentDiagnosticsByFile[filePath];
+            }
+        }
+        return result;
+    }
+
+    /**
+     * Determine if two diagnostic lists are identical
+     */
+    private diagnosticListsAreIdentical(list1: KeyedDiagnostic[], list2: KeyedDiagnostic[]) {
+        //skip all checks if the lists are not the same size
+        if (list1.length !== list2.length) {
+            return false;
+        }
+        for (let i = 0; i < list1.length; i++) {
+            if (list1[i].key !== list2[i].key) {
+                return false;
+            }
+        }
+
+        //if we made it here, the lists are identical
+        return true;
+    }
+
+    /**
+     * Get diagnostics for all new files not seen since last time
+     */
+    private getAddedPatch(currentDiagnosticsByFile: { [fileSrcPathLower: string]: KeyedDiagnostic[] }) {
+        const result = {} as { [fileSrcPathLower: string]: KeyedDiagnostic[] };
+        for (const filePath in currentDiagnosticsByFile) {
+            if (!this.previousDiagnosticsByFile[filePath]) {
+                result[filePath] = currentDiagnosticsByFile[filePath];
+            }
+        }
+        return result;
+    }
+}
+
+interface KeyedDiagnostic extends BsDiagnostic {
+    key: string;
+}

src/LanguageServer.ts

@@ -6,7 +6,6 @@ import {
     CompletionItem,
     Connection,
     createConnection,
-    Diagnostic,
     DidChangeConfigurationNotification,
     DidChangeWatchedFilesParams,
     FileChangeType,
@@ -29,10 +28,10 @@ import { Deferred } from './deferred';
 import { DiagnosticMessages } from './DiagnosticMessages';
 import { ProgramBuilder } from './ProgramBuilder';
 import { standardizePath as s, util } from './util';
-import { BsDiagnostic } from './interfaces';
 import { Logger } from './Logger';
 import { Throttler } from './Throttler';
 import { KeyedThrottler } from './KeyedThrottler';
+import { DiagnosticCollection } from './DiagnosticCollection';
 
 export class LanguageServer {
     //cast undefined as any to get around strictNullChecks...it's ok in this case
@@ -933,89 +932,29 @@ export class LanguageServer {
         return results;
     }
 
-    /**
-     * The list of all issues, indexed by file. This allows us to keep track of which buckets of
-     * diagnostics to send and which to skip because nothing has changed
-     */
-    private latestDiagnosticsByFile = {} as { [key: string]: Diagnostic[] };
-    private async sendDiagnostics() {
-        //compute the new list of diagnostics for whole project
-        let diagnosticsByFile = {} as { [key: string]: Diagnostic[] };
-        let workspaces = this.getWorkspaces();
-
-        //make a bucket for every file in every project
-        for (let workspace of workspaces) {
-            //Ensure the program was constructued. This prevents race conditions where certain diagnostics are being sent before the program was created.
-            await workspace.firstRunPromise;
-            //if there is no program, skip this workspace (hopefully diagnostics were added to the builder itself
-            if (workspace.builder && workspace.builder.program) {
-                for (let filePath in workspace.builder.program.files) {
-                    diagnosticsByFile[filePath] = [];
-                }
-            }
-        }
-
-        let diagnostics = Array.prototype.concat.apply([] as BsDiagnostic[],
-            workspaces.map((x) => x.builder.getDiagnostics())
-        ) as BsDiagnostic[];
-
-        /**
-         * A map that tracks which diagnostics have been added for each file.
-         * This allows us to remove duplicate diagnostics
-         */
-        let uniqueMap = {} as { [diagnosticKey: string]: boolean };
+    private diagnosticCollection = new DiagnosticCollection();
 
-        for (let diagnostic of diagnostics) {
-            //certain diagnostics are attached to non-tracked files, so create those buckets dynamically
-            if (!diagnosticsByFile[diagnostic.file.pathAbsolute]) {
-                diagnosticsByFile[diagnostic.file.pathAbsolute] = [];
-            }
-            let key =
-                diagnostic.file.pathAbsolute + '-' +
-                diagnostic.code + '-' +
-                diagnostic.range.start.line + '-' +
-                diagnostic.range.start.character + '-' +
-                diagnostic.range.end.line + '-' +
-                diagnostic.range.end.character;
-
-            //filter exact duplicate diagnostics from multiple projects for same file and location
-            if (!uniqueMap[key]) {
-                uniqueMap[key] = true;
-                let d = {
-                    severity: diagnostic.severity,
-                    range: diagnostic.range,
-                    message: diagnostic.message,
-                    relatedInformation: diagnostic.relatedInformation,
-                    code: diagnostic.code,
+    private async sendDiagnostics() {
+        //Get only the changes to diagnostics since the last time we sent them to the client
+        const patch = await this.diagnosticCollection.getPatch(this.workspaces);
+
+        for (let filePath in patch) {
+            const diagnostics = patch[filePath].map(d => {
+                return {
+                    severity: d.severity,
+                    range: d.range,
+                    message: d.message,
+                    relatedInformation: d.relatedInformation,
+                    code: d.code,
                     source: 'brs'
                 };
+            });
 
-                diagnosticsByFile[diagnostic.file.pathAbsolute].push(d);
-            }
-        }
-
-        //send all diagnostics
-        for (let filePath in diagnosticsByFile) {
-            //TODO filter by only the files that have changed
             this.connection.sendDiagnostics({
                 uri: URI.file(filePath).toString(),
-                diagnostics: diagnosticsByFile[filePath]
+                diagnostics: diagnostics
             });
         }
-
-        //clear any diagnostics for files that are no longer present
-        let currentFilePaths = Object.keys(diagnosticsByFile);
-        for (let filePath in this.latestDiagnosticsByFile) {
-            if (!currentFilePaths.includes(filePath)) {
-                this.connection.sendDiagnostics({
-                    uri: URI.file(filePath).toString(),
-                    diagnostics: []
-                });
-            }
-        }
-
-        //save the new list of diagnostics
-        this.latestDiagnosticsByFile = diagnosticsByFile;
     }
 
     public async onExecuteCommand(params: ExecuteCommandParams) {

src/brsTypes/Callable.ts

@@ -7,7 +7,6 @@ import { Range } from 'vscode-languageserver';
 import { TranspileState } from '../parser/TranspileState';
 import { walk, InternalWalkMode, WalkOptions, WalkVisitor } from '../astUtils';
 import { Expression, LiteralExpression } from '../parser/Expression';
-import util from '../util';
 
 /** An argument to a BrightScript `function` or `sub`. */
 export interface Argument {
@@ -57,7 +56,7 @@ export class StdlibArgument implements Argument {
     }
 
     /** A fake location exists only within the BRS runtime. */
-    static InternalRange = util.createRange(-1, -1, -1, -1);
+    static InternalRange = Range.create(-1, -1, -1, -1);
 }
 
 export class FunctionParameterExpression extends Expression {