Repository: rokucommunity/brighterscript (ts)

Issue:
LSP crashes due to invalid diagnostics (v1)
- brighterscript@1.0.0-alpha.46
- @rokucommunity/bslint@1.0.0-alpha.46
- rooibos-roku@6.0.0-alpha.46

```
[01:40:29:070 PM][ERROR] [prj0] Error when calling plugin bsc-language-server.afterProgramValidate: TypeError: Cannot read properties of undefined (reading 'uri')
    at /Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/lsp/Project.ts:195:55
    at Array.map (<anonymous>)
    at Project.getDiagnostics (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/lsp/Project.ts:194:28)
    at Object.afterProgramValidate (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/lsp/Project.ts:100:42)
    at /Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/PluginInterface.ts:63:47
    at Logger.time (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/@rokucommunity/logger/dist/Logger.js:373:20)
    at PluginInterface.emit (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/PluginInterface.ts:62:34)
    at EventEmitter.<anonymous> (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/Program.ts:1209:34)
    at EventEmitter.emit (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/eventemitter3/index.js:180:35)
    at Sequencer.run (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/common/Sequencer.ts:121:26)
    at async Project.validate (/Users/lse16/dev/gst-apps-roku-client-bscv1/node_modules/brighterscript/src/lsp/Project.ts:175:13)
```

https://github.com/rokucommunity/brighterscript/blob/v1/src/lsp/Project.ts#L195

This happens when a given diagnostic happens to have no location, like:

```
{
  key: "No uri undefined:undefined-undefined:undefined - cannot-find-name - Cannot find name 'Rail' for namespace 'browse.PAGE.browse'",
  message: "Cannot find name 'Rail' for namespace 'browse.PAGE.browse'",
  legacyCode: 1001,
  data: {
    name: 'Rail',
    fullName: 'browse.PAGE.browse.Rail',
    typeName: 'browse.PAGE.browse'
  },
  severity: 1,
  code: 'cannot-find-name',
  location: undefined,
  relatedInformation: [ { message: "In scope 'source'", location: undefined } ]
}
```

Pull request description:
Better diagnostic null location handling
Fixes #1487 by excluding diagnostics that don't have location. Also improves the situation by trying to attach diagnostics to other related locations if we couldn't find a valid location. 

Patch:
diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts
index dc46738b..ee279202 100644
--- a/src/DiagnosticManager.ts
+++ b/src/DiagnosticManager.ts
@@ -4,7 +4,7 @@ import type { Scope } from './Scope';
 import { util } from './util';
 import { Cache } from './Cache';
 import { isBsDiagnostic, isXmlScope } from './astUtils/reflection';
-import type { DiagnosticRelatedInformation } from 'vscode-languageserver-protocol';
+import type { DiagnosticRelatedInformation, Location } from 'vscode-languageserver-protocol';
 import { DiagnosticFilterer } from './DiagnosticFilterer';
 import { DiagnosticSeverityAdjuster } from './DiagnosticSeverityAdjuster';
 import type { FinalizedBsConfig } from './BsConfig';
@@ -23,6 +23,13 @@ interface BsDiagnosticWithKey extends BsDiagnostic {
     key: string;
 }
 
+export interface LocationResolverArgs {
+    diagnostic: BsDiagnosticWithKey;
+    contexts: Set<DiagnosticContext>;
+}
+
+type LocationResolver = (options: LocationResolverArgs) => Location | undefined;
+
 /**
  * Manages all diagnostics for a program.
  * Diagnostics can be added specific to a certain file/range and optionally scope or an AST node
@@ -32,8 +39,9 @@ interface BsDiagnosticWithKey extends BsDiagnostic {
  */
 export class DiagnosticManager {
 
-    constructor(options?: { logger?: Logger }) {
+    constructor(options?: { logger?: Logger; locationResolver: LocationResolver }) {
         this.logger = options?.logger ?? createLogger();
+        this.locationResolver = options?.locationResolver ?? ((x) => x?.diagnostic?.location);
     }
 
     private diagnosticsCache = new Cache<string, DiagnosticWithContexts>();
@@ -44,6 +52,8 @@ export class DiagnosticManager {
 
     public logger: Logger;
 
+    public locationResolver: LocationResolver;
+
     public options: FinalizedBsConfig;
 
     public program: Program;
@@ -138,6 +148,7 @@ export class DiagnosticManager {
             });
 
             this.logger?.info(`diagnostic counts: total=${chalk.yellow(diagnostics.length.toString())}, after filter=${chalk.yellow(filteredDiagnostics.length.toString())}`);
+
             return filteredDiagnostics;
         };
 
@@ -173,6 +184,13 @@ export class DiagnosticManager {
 
             }
             diagnostic.relatedInformation = relatedInformation;
+            if (!diagnostic.location?.uri) {
+                diagnostic.location = this.locationResolver?.(cachedDiagnostic);
+                if (diagnostic.location) {
+                    //if we found a location, tweak the message a bit to let devs know this was not the original location
+                    diagnostic.message = `${diagnostic.message} (location unknown, added here for visibility)`;
+                }
+            }
             results.push(diagnostic);
         }
         const filteredResults = results.filter((x) => {
diff --git a/src/Program.spec.ts b/src/Program.spec.ts
index b9c14c6e..72361b04 100644
--- a/src/Program.spec.ts
+++ b/src/Program.spec.ts
@@ -1573,6 +1573,33 @@ describe('Program', () => {
                 DiagnosticMessages.cannotFindFunction('C')
             ]);
         });
+
+        it('attaches locationless diagnostic to the nearest xml file', () => {
+            const xmlFile = program.setFile('components/MainScene.xml', trim`
+                <component name="MainScene" extends="Scene">
+                </component>
+            `);
+            program.diagnostics.register({
+                message: 'test'
+            } as any, {
+                scope: program.getFirstScopeForFile(xmlFile)
+            });
+            expectDiagnostics(program, [{
+                message: 'test (location unknown, added here for visibility)',
+                location: util.createLocation(0, 0, 0, 100, xmlFile.srcPath)
+            }]);
+        });
+
+        it('attaches locationless diagnostic to the nearest manifest file', () => {
+            const manifestFile = program.setFile('manifest', trim``);
+            program.diagnostics.register({
+                message: 'test'
+            } as any);
+            expectDiagnostics(program, [{
+                message: 'test (location unknown, added here for visibility)',
+                location: util.createLocation(0, 0, 0, 100, manifestFile.srcPath)
+            }]);
+        });
     });
 
     describe('getCompletions', () => {
diff --git a/src/Program.ts b/src/Program.ts
index ae346844..b9912f52 100644
--- a/src/Program.ts
+++ b/src/Program.ts
@@ -83,6 +83,35 @@ export class Program {
         this.plugins = plugins || new PluginInterface([], { logger: this.logger });
         this.diagnostics = diagnosticsManager || new DiagnosticManager();
 
+        //try to find a location for the diagnostic if it doesn't have one
+        this.diagnostics.locationResolver = (args) => {
+
+            //find the first xml scope for this diagnostic
+            for (let context of args.contexts) {
+                if (isXmlScope(context.scope) && isXmlFile(context.scope.xmlFile)) {
+                    return util.createLocation(0, 0, 0, 100, context.scope.xmlFile.srcPath);
+                }
+            }
+
+            //we couldn't find an xml scope for this, so try to find the manifest file instead
+            const manifest = this.getFile('manifest', false);
+            if (manifest) {
+                return util.createLocation(0, 0, 0, 100, manifest.srcPath);
+            }
+
+            //if we still don't have a manifest, try to find the first file in the program
+            for (const key in this.files) {
+                if (isBrsFile(this.files[key]) || isXmlFile(this.files[key])) {
+                    return util.createLocation(0, 0, 0, 100, this.files[key].srcPath);
+                }
+            }
+
+            this.logger.warn(`Unable to find a location for the diagnostic.`, args);
+
+            //we couldn't find any locations for the file, so just return undefined
+            return undefined;
+        };
+
         // initialize the diagnostics Manager
         this.diagnostics.logger = this.logger;
         this.diagnostics.options = this.options;
diff --git a/src/astUtils/reflection.ts b/src/astUtils/reflection.ts
index 78abaf45..ef5a8d94 100644
--- a/src/astUtils/reflection.ts
+++ b/src/astUtils/reflection.ts
@@ -36,6 +36,8 @@ import type { BscFile } from '../files/BscFile';
 import type { ComponentType } from '../types/ComponentType';
 import type { AssociativeArrayType } from '../types/AssociativeArrayType';
 import { TokenKind } from '../lexer/TokenKind';
+import type { Program } from '../Program';
+import type { Project } from '../lsp/Project';
 
 // File reflection
 export function isBrsFile(file: BscFile | undefined): file is BrsFile {
@@ -60,6 +62,15 @@ export function isXmlScope(scope: (Scope | undefined)): scope is XmlScope {
 }
 
 
+export function isProgram(arg: any): arg is Program {
+    return arg?.constructor.name === 'Program';
+}
+
+export function isProject(arg: any): arg is Project {
+    return arg?.constructor.name === 'Project';
+}
+
+
 // Statements reflection
 
 /**
diff --git a/src/lsp/Project.spec.ts b/src/lsp/Project.spec.ts
index 00099be1..2170a216 100644
--- a/src/lsp/Project.spec.ts
+++ b/src/lsp/Project.spec.ts
@@ -256,4 +256,22 @@ describe('Project', () => {
             await project['getConfigFilePath'](undefined);
         });
     });
+
+    describe('getDiagnostics', () => {
+        it('does not crash when diagnostic is missing location', async () => {
+            await project.activate({
+                projectPath: rootDir
+            } as any);
+            await project.validate();
+
+            //register a diagnostic with no location
+            project['builder'].diagnostics.register({
+                message: 'test diagnostic',
+                location: undefined
+            });
+
+            //this should not throw
+            project.getDiagnostics();
+        });
+    });
 });
diff --git a/src/lsp/Project.ts b/src/lsp/Project.ts
index 3509f6bf..b9ea32e5 100644
--- a/src/lsp/Project.ts
+++ b/src/lsp/Project.ts
@@ -190,14 +190,21 @@ export class Project implements LspProject {
     }
 
     public getDiagnostics(): LspDiagnostic[] {
-        const diagnostics = this.builder.getDiagnostics();
-        return diagnostics.map(x => {
-            const srcPath = util.uriToPath(x.location.uri);
-            return {
-                ...util.toDiagnostic(x, srcPath),
-                uri: x.location.uri
-            };
-        });
+        const result: LspDiagnostic[] = [];
+        for (const diagnostic of this.builder.getDiagnostics()) {
+            //skip diagnostics that have no location
+            if (!diagnostic?.location?.uri) {
+                this.logger.debug('Skipping diagnostic that has no location', diagnostic);
+                continue;
+            }
+
+            const srcPath = util.uriToPath(diagnostic.location.uri);
+            result.push({
+                ...util.toDiagnostic(diagnostic, srcPath),
+                uri: diagnostic.location.uri
+            });
+        }
+        return result;
     }
 
     /**
diff --git a/src/lsp/ProjectManager.spec.ts b/src/lsp/ProjectManager.spec.ts
index 1008319d..2332477f 100644
--- a/src/lsp/ProjectManager.spec.ts
+++ b/src/lsp/ProjectManager.spec.ts
@@ -234,7 +234,7 @@ describe('ProjectManager', () => {
             }]);
             expectDiagnostics(await onNextDiagnostics(), [
                 DiagnosticMessages.cannotFindName('nameNotDefined').message,
-                'Test diagnostic'
+                'Test diagnostic (location unknown, added here for visibility)'
             ]);
         });
 


Test patch:
diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts
index b1902617..c935657a 100644
--- a/src/testHelpers.spec.ts
+++ b/src/testHelpers.spec.ts
@@ -7,7 +7,7 @@ import { expect } from './chai-config.spec';
 import type { CodeActionShorthand } from './CodeActionUtil';
 import { codeActionUtil } from './CodeActionUtil';
 import type { BrsFile } from './files/BrsFile';
-import { Program } from './Program';
+import type { Program } from './Program';
 import { standardizePath as s } from './util';
 import { getDiagnosticLine } from './diagnosticUtils';
 import { firstBy } from 'thenby';
@@ -15,6 +15,8 @@ import undent from 'undent';
 import type { BscFile } from './files/BscFile';
 import type { BscType } from './types/BscType';
 import type { LspDiagnostic } from './lsp/LspProject';
+import { Project } from './lsp/Project';
+import { isProgram, isProject } from './astUtils/reflection';
 
 export const cwd = s`${__dirname}/../`;
 export const tempDir = s`${__dirname}/../.tmp`;
@@ -37,6 +39,19 @@ type DiagnosticCollectionAsync = DiagnosticCollection | { getDiagnostics(): Prom
 function getDiagnostics(arg: DiagnosticCollection): BsDiagnostic[] {
     if (Array.isArray(arg)) {
         return arg as BsDiagnostic[];
+
+        //project.getDiagnostics() returns LspDiagonstics
+    } else if (arg instanceof Project) {
+        const diagnostics = arg.getDiagnostics();
+        return diagnostics.map(diagnostic => {
+            return {
+                ...diagnostic,
+                location: {
+                    uri: diagnostic.uri,
+                    range: diagnostic.range
+                }
+            } as BsDiagnostic;
+        });
     } else if ((arg as any).getDiagnostics) {
         return (arg as any).getDiagnostics();
     } else if ((arg as any).diagnostics) {
@@ -218,11 +233,18 @@ export function expectZeroDiagnostics(arg: DiagnosticCollection) {
             //escape any newlines
             diagnostic.message = diagnostic.message.replace(/\r/g, '\\r').replace(/\n/g, '\\n');
             message += `\n        • bs:${diagnostic.code} "${diagnostic.message}" at ${diagnostic.location?.uri ?? ''}#(${diagnostic.location?.range?.start.line}:${diagnostic.location?.range?.start.character})-(${diagnostic.location?.range?.end.line}:${diagnostic.location?.range?.end.character})`;
-            //print the line containing the error (if we can find it)srcPath
-            if (arg instanceof Program) {
-                const file = arg.getFile(diagnostic.location.uri);
 
+            let program: Program;
+            if (isProject(arg)) {
+                program = arg['builder'].program;
+            } else if (isProgram(arg)) {
+                program = arg;
+            }
+
+            if (program) {
+                const file = program.getFile(diagnostic.location.uri);
                 const line = (file as BrsFile)?.fileContents?.split(/\r?\n/g)?.[diagnostic.location.range?.start.line];
+                //print the line containing the error (if we can find it)
                 if (line) {
                     message += '\n' + getDiagnosticLine(diagnostic, line, chalk.red);
                 }