From 0f7cce2de4d3b825a0b081bdf4387e27f0f7d4fa Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Mon, 23 Sep 2024 11:55:00 -0400 Subject: [PATCH 1/2] Prevent crash when validating an orphaned `ReturnStatement` --- .../validation/ScopeValidator.spec.ts | 27 +++++++++++++++++++ src/bscPlugin/validation/ScopeValidator.ts | 3 +-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/bscPlugin/validation/ScopeValidator.spec.ts b/src/bscPlugin/validation/ScopeValidator.spec.ts index 5c5126e7b..5f12cb38d 100644 --- a/src/bscPlugin/validation/ScopeValidator.spec.ts +++ b/src/bscPlugin/validation/ScopeValidator.spec.ts @@ -13,11 +13,15 @@ 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({ @@ -25,11 +29,34 @@ describe('ScopeValidator', () => { }); program.createSourceScope(); }); + afterEach(() => { sinon.restore(); program.dispose(); }); + it('validateReturnStatement does not crash', () => { + program.options.autoImportComponentScript = true; + program.setFile('components/Component.xml', trim` + + + `); + const file = program.setFile('components/Component.bs', trim` + function test() + return { + method: function() + return true + end function + } + end function + `); + const returnStatement = file.ast.findChild(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', ` diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index f82878818..6b43e8588 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -405,7 +405,7 @@ export class ScopeValidator { */ private validateReturnStatement(file: BrsFile, returnStmt: ReturnStatement) { const getTypeOptions = { flags: SymbolTypeFlag.runtime }; - 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 = {}; @@ -415,7 +415,6 @@ export class ScopeValidator { ...DiagnosticMessages.returnTypeMismatch(actualReturnType.toString(), funcType.returnType.toString(), compatibilityData), location: returnStmt.value.location }); - } } } From 0366409423e494718aafb03201d701fe64eff106 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Mon, 23 Sep 2024 13:56:17 -0400 Subject: [PATCH 2/2] Prevent some more crashes --- src/DiagnosticManager.spec.ts | 44 ++++++++++++++++++++++++++++++++++- src/DiagnosticManager.ts | 4 ++-- src/Program.spec.ts | 22 ++++++++++++++++++ src/Program.ts | 2 +- 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/DiagnosticManager.spec.ts b/src/DiagnosticManager.spec.ts index f06ff1703..dcd937637 100644 --- a/src/DiagnosticManager.spec.ts +++ b/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 + }); }); }); diff --git a/src/DiagnosticManager.ts b/src/DiagnosticManager.ts index 4a5f51816..6607aca32 100644 --- a/src/DiagnosticManager.ts +++ b/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); } } diff --git a/src/Program.spec.ts b/src/Program.spec.ts index 8cf89770b..0e4f976a9 100644 --- a/src/Program.spec.ts +++ b/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', () => { diff --git a/src/Program.ts b/src/Program.ts index dca9e507e..9a259e3a7 100644 --- a/src/Program.ts +++ b/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));