From dbde41671fd35e3e2157093d05ad46f4f93c6c20 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Sun, 11 Oct 2020 14:36:50 -0400 Subject: [PATCH 1/3] Fix circular reference issue with Callable.ts --- src/brsTypes/Callable.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/brsTypes/Callable.ts b/src/brsTypes/Callable.ts index 75c3823a0..8e7a34b48 100644 --- a/src/brsTypes/Callable.ts +++ b/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 { From c05911ab4d26febdfa75ce7d7a2c784eca4483f2 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Sun, 11 Oct 2020 14:37:58 -0400 Subject: [PATCH 2/3] Only send changes to diagnostics to client. --- src/DiagnosticCollection.spec.ts | 111 ++++++++++++++++++++++++++++ src/DiagnosticCollection.ts | 123 +++++++++++++++++++++++++++++++ src/LanguageServer.ts | 93 ++++------------------- 3 files changed, 250 insertions(+), 77 deletions(-) create mode 100644 src/DiagnosticCollection.spec.ts create mode 100644 src/DiagnosticCollection.ts diff --git a/src/DiagnosticCollection.spec.ts b/src/DiagnosticCollection.spec.ts new file mode 100644 index 000000000..c896c9871 --- /dev/null +++ b/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.only('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 + }); + } + } +}); diff --git a/src/DiagnosticCollection.ts b/src/DiagnosticCollection.ts new file mode 100644 index 000000000..71977b953 --- /dev/null +++ b/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; +} diff --git a/src/LanguageServer.ts b/src/LanguageServer.ts index 3d06375f4..ed5af6e80 100644 --- a/src/LanguageServer.ts +++ b/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) { From 5b6e2d4bb6ec4bdb5713bae093d472e636b992a5 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Sun, 11 Oct 2020 14:48:00 -0400 Subject: [PATCH 3/3] Removed exclusive test --- src/DiagnosticCollection.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DiagnosticCollection.spec.ts b/src/DiagnosticCollection.spec.ts index c896c9871..612f78832 100644 --- a/src/DiagnosticCollection.spec.ts +++ b/src/DiagnosticCollection.spec.ts @@ -6,7 +6,7 @@ import { File } from './interfaces'; import util from './util'; import { expect } from 'chai'; -describe.only('DiagnosticCollection', () => { +describe('DiagnosticCollection', () => { let collection: DiagnosticCollection; let diagnostics: BsDiagnostic[]; let workspaces: Workspace[];