Fix some null crashes (#1304)

* Prevent crash when validating an orphaned `ReturnStatement`

* Prevent some more crashes

src/DiagnosticManager.spec.ts

@@ -5,9 +5,13 @@ import util from './util';
 
 
 describe('DiagnosticManager', () => {
+    let program: Program;
+    beforeEach(() => {
+        program = new Program({});
+    });
+
     describe('diagnosticIsSuppressed', () => {
         it('does not crash when diagnostic is missing location information', () => {
-            const program = new Program({});
             const file = program.setFile('source/main.brs', '') as BrsFile;
             const diagnostic: BsDiagnostic = {
                 message: 'crash',
@@ -27,5 +31,43 @@ describe('DiagnosticManager', () => {
 
             //test passes if there's no crash
         });
+
+        it('does not crash when diagnostic is missing the entire `.location` object', () => {
+            const file = program.setFile('source/main.brs', '') as BrsFile;
+            const diagnostic: BsDiagnostic = {
+                message: 'crash',
+                //important part of the test, `.uri` must be missing
+                location: { uri: undefined, range: util.createRange(1, 2, 3, 4) }
+            };
+
+            file.commentFlags.push({
+                affectedRange: util.createRange(1, 2, 3, 4),
+                codes: [1, 2, 3],
+                file: file,
+                range: util.createRange(1, 2, 3, 4)
+            });
+            program.diagnostics.register(diagnostic);
+
+            program.diagnostics.isDiagnosticSuppressed(diagnostic);
+
+            //test passes if there's no crash
+        });
+    });
+
+    describe('clearForFile', () => {
+        it('does not crash when filePath is invalid', () => {
+            const diagnostic: BsDiagnostic = {
+                message: 'crash',
+                //important part of the test. `.uri` must be missing
+                location: { uri: undefined, range: util.createRange(1, 2, 3, 4) }
+            };
+
+            program.diagnostics.register(diagnostic);
+
+            program.diagnostics.clearForFile(undefined);
+            program.diagnostics.clearForFile(null);
+            program.diagnostics.clearForFile('');
+            //the test passes because none of these lines throw an error
+        });
     });
 });

src/DiagnosticManager.ts

@@ -164,9 +164,9 @@ export class DiagnosticManager {
     }
 
     public clearForFile(fileSrcPath: string) {
-        const fileSrcPathUri = util.pathToUri(fileSrcPath).toLowerCase();
+        const fileSrcPathUri = util.pathToUri(fileSrcPath)?.toLowerCase?.();
         for (const [key, cachedData] of this.diagnosticsCache.entries()) {
-            if (cachedData.diagnostic.location?.uri.toLowerCase() === fileSrcPathUri) {
+            if (cachedData.diagnostic.location?.uri?.toLowerCase?.() === fileSrcPathUri) {
                 this.diagnosticsCache.delete(key);
             }
         }

src/Program.spec.ts

@@ -941,6 +941,28 @@ describe('Program', () => {
                 program.getCodeActions('not/real/file', util.createRange(1, 2, 3, 4));
             });
         });
+
+        it('does not crash when a diagnostic is missing a URI', () => {
+            const file = program.setFile('source/main.brs', '');
+            const diagnostic = {
+                message: 'crash',
+                location: { uri: util.pathToUri(file.srcPath), range: util.createRange(1, 2, 3, 4) }
+            };
+            program.diagnostics.register(diagnostic);
+            //delete the uri
+            diagnostic.location.uri = undefined;
+
+            doesNotThrow(() => {
+                program.getCodeActions('source/main.brs', util.createRange(1, 2, 3, 4));
+            });
+
+            //delete the entire location
+            diagnostic.location = undefined;
+
+            doesNotThrow(() => {
+                program.getCodeActions('source/main.brs', util.createRange(1, 2, 3, 4));
+            });
+        });
     });
 
     describe('xml inheritance', () => {

src/Program.ts

@@ -1324,7 +1324,7 @@ export class Program {
                 //get all current diagnostics (filtered by diagnostic filters)
                 .getDiagnostics()
                 //only keep diagnostics related to this file
-                .filter(x => x.location.uri === fileUri)
+                .filter(x => x.location?.uri === fileUri)
                 //only keep diagnostics that touch this range
                 .filter(x => util.rangesIntersectOrTouch(x.location.range, range));
 

src/bscPlugin/validation/ScopeValidator.spec.ts

@@ -13,23 +13,50 @@ import { AssociativeArrayType } from '../../types/AssociativeArrayType';
 import undent from 'undent';
 import * as fsExtra from 'fs-extra';
 import { tempDir, rootDir } from '../../testHelpers.spec';
+import { isReturnStatement } from '../../astUtils/reflection';
+import { ScopeValidator } from './ScopeValidator';
+import type { ReturnStatement } from '../../parser/Statement';
 
 describe('ScopeValidator', () => {
 
     let sinon = sinonImport.createSandbox();
     let program: Program;
+
     beforeEach(() => {
         fsExtra.emptyDirSync(tempDir);
         program = new Program({
             rootDir: rootDir
         });
         program.createSourceScope();
     });
+
     afterEach(() => {
         sinon.restore();
         program.dispose();
     });
 
+    it('validateReturnStatement does not crash', () => {
+        program.options.autoImportComponentScript = true;
+        program.setFile('components/Component.xml', trim`
+            <component name="Test" extends="Group">
+            </component>
+        `);
+        const file = program.setFile<BrsFile>('components/Component.bs', trim`
+            function test()
+                return {
+                    method: function()
+                        return true
+                    end function
+                }
+            end function
+        `);
+        const returnStatement = file.ast.findChild<ReturnStatement>(isReturnStatement);
+        delete returnStatement.parent;
+        const validator = new ScopeValidator();
+        //should not crash
+        validator['validateReturnStatement'](file, returnStatement);
+    });
+
     describe('mismatchArgumentCount', () => {
         it('detects calling functions with too many arguments', () => {
             program.setFile('source/file.brs', `

src/bscPlugin/validation/ScopeValidator.ts

@@ -417,7 +417,7 @@ export class ScopeValidator {
     private validateReturnStatement(file: BrsFile, returnStmt: ReturnStatement) {
         const data: ExtraSymbolData = {};
         const getTypeOptions = { flags: SymbolTypeFlag.runtime, data: data };
-        let funcType = returnStmt.findAncestor(isFunctionExpression).getType({ flags: SymbolTypeFlag.typetime });
+        let funcType = returnStmt.findAncestor(isFunctionExpression)?.getType({ flags: SymbolTypeFlag.typetime });
         if (isTypedFunctionType(funcType)) {
             const actualReturnType = this.getNodeTypeWrapper(file, returnStmt?.value, getTypeOptions);
             const compatibilityData: TypeCompatibilityData = {};
@@ -427,7 +427,6 @@ export class ScopeValidator {
                     ...DiagnosticMessages.returnTypeMismatch(actualReturnType.toString(), funcType.returnType.toString(), compatibilityData),
                     location: returnStmt.value.location
                 });
-
             }
         }
     }