From d557fa3624dcd512eb32a605a36f7d3143c14549 Mon Sep 17 00:00:00 2001 From: Rob Reed Date: Sat, 10 Oct 2026 20:08:05 -0700 Subject: [PATCH 1/4] Skip the namespace collision check in scopes without namespaces. detectVariableNamespaceCollisions walked every parameter and assignment up to the root of the tree to find an enclosing namespace, in every scope that includes the file. When the scope has no namespaces, getNamespace can never match, so return before walking. --- src/Scope.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Scope.ts b/src/Scope.ts index 4f4bf005c..353d2fd3d 100644 --- a/src/Scope.ts +++ b/src/Scope.ts @@ -800,6 +800,11 @@ export class Scope { } private detectVariableNamespaceCollisions(file: BrsFile) { + //nothing can collide with a namespace when the scope has none, so skip walking every param and assignment + if (this.namespaceLookup.size === 0) { + return; + } + //find all function parameters for (let func of file.parser.references.functionExpressions) { for (let param of func.parameters) { From 391b063947d3fb449333bc4132fec576b81b7749 Mon Sep 17 00:00:00 2001 From: Rob Reed Date: Sat, 10 Oct 2026 20:08:05 -0700 Subject: [PATCH 2/4] Binary search the top level functions to find a function scope. findFunctionCalls looks up the function scope of every call, and the lookup checked each function in the file in turn, so large files spent most of their parse time here. When every range is present, the top level functions are in order without overlapping, and each nested function sits inside its parent, the first scope containing a position is the top level one a binary search finds. Otherwise, such as after a parse error, the lookup checks every scope as before. --- src/files/BrsFile.spec.ts | 61 +++++++++++++++++++++++++++++++++++++++ src/files/BrsFile.ts | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/src/files/BrsFile.spec.ts b/src/files/BrsFile.spec.ts index 1abd673d1..adf0eab08 100644 --- a/src/files/BrsFile.spec.ts +++ b/src/files/BrsFile.spec.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import * as sinonImport from 'sinon'; import { CompletionItemKind, Position, Range } from 'vscode-languageserver'; import type { BsDiagnostic, Callable, CommentFlag, VariableDeclaration } from '../interfaces'; +import type { FunctionScope } from '../FunctionScope'; import { Program } from '../Program'; import { BooleanType } from '../types/BooleanType'; import { DynamicType } from '../types/DynamicType'; @@ -2224,6 +2225,66 @@ describe('BrsFile', () => { expect(innerScope.variableDeclarations).to.be.lengthOf(1); }); + it('finds the function scope at a position among many functions', () => { + file.parse(` + sub First() + end sub + + sub Second() + inner = sub() + print "inner" + end sub + end sub + + sub Third() + end sub + `); + const startLineAt = (line: number, character: number) => file.getFunctionScopeAtPosition(Position.create(line, character))?.func.range?.start.line; + expect(startLineAt(1, 20)).to.equal(1); + expect(startLineAt(3, 0)).to.be.undefined; + expect(startLineAt(4, 20)).to.equal(4); + expect(startLineAt(6, 25)).to.equal(5); + expect(startLineAt(10, 20)).to.equal(10); + expect(startLineAt(12, 0)).to.be.undefined; + }); + + it('finds the same function scope as checking every scope in order', () => { + const checkEveryScope = (position: Position, scopes: FunctionScope[]): FunctionScope | undefined => { + for (const scope of scopes) { + if (util.rangeContains(scope.range, position)) { + return checkEveryScope(position, scope.childrenScopes) ?? scope; + } + } + return undefined; + }; + const sources = [` + sub First() + inner = sub() + deeper = sub() + end sub + end sub + end sub + sub Second() + end sub + `, ` + sub MissingEnd() + x = 1 + sub Second() + y = sub() + end sub + `]; + for (const source of sources) { + const brsFile = new BrsFile(`${rootDir}/source/main.brs`, 'source/main.brs', program); + brsFile.parse(source); + source.split('\n').forEach((text, line) => { + for (let character = 0; character <= text.length; character++) { + const position = Position.create(line, character); + expect(brsFile.getFunctionScopeAtPosition(position)).to.equal(checkEveryScope(position, brsFile.functionScopes)); + } + }); + } + }); + it('finds variables declared in function scopes', () => { file.parse(` sub Main() diff --git a/src/files/BrsFile.ts b/src/files/BrsFile.ts index 745524223..835713122 100644 --- a/src/files/BrsFile.ts +++ b/src/files/BrsFile.ts @@ -811,6 +811,40 @@ export class BrsFile { } } + /** + * The file's top level function scopes, in order, when a position can be found among them by binary search: every range + * is present, each top level function starts after the previous one ends, and every nested function sits inside its + * parent. Then the first of the file's scopes that contains a position is the top level one found by the search. + * Undefined when that does not hold, such as after a parse error, so the caller checks every scope instead. + * Cached on the parser-level cache, so it is rebuilt when the file re-parses. + */ + private getSortedTopLevelFunctionScopes() { + return this.cache.getOrAdd('sortedTopLevelFunctionScopes', () => this.findSortedTopLevelFunctionScopes(this.functionScopes)); + } + + private findSortedTopLevelFunctionScopes(functionScopes: FunctionScope[]): FunctionScope[] | undefined { + const topLevelScopes = [] as FunctionScope[]; + for (const scope of functionScopes) { + const range = scope.range; + if (!range) { + return undefined; + } + if (scope.parentScope) { + const parentRange = scope.parentScope.range; + if (!parentRange || util.comparePositionToRange(range.start, parentRange) !== 0 || util.comparePositionToRange(range.end, parentRange) !== 0) { + return undefined; + } + } else { + const previous = topLevelScopes[topLevelScopes.length - 1]; + if (previous && util.comparePositionToRange(range.start, previous.range) !== 1) { + return undefined; + } + topLevelScopes.push(scope); + } + } + return topLevelScopes; + } + /** * Find the function scope at the given position. * @param position the position used to find the deepest scope that contains it @@ -825,6 +859,26 @@ export class BrsFile { if (!functionScopes) { functionScopes = this.functionScopes; } + //binary search the top level functions instead of checking every function in the file, when they allow it + const topLevelScopes = position && functionScopes === this._functionScopes ? this.getSortedTopLevelFunctionScopes() : undefined; + if (topLevelScopes) { + let low = 0; + let high = topLevelScopes.length - 1; + while (low <= high) { + const middle = Math.floor((low + high) / 2); + const comparison = util.comparePositionToRange(position, topLevelScopes[middle].range); + if (comparison === 0) { + const scope = topLevelScopes[middle]; + return this._getFunctionScopeAtPosition(position, scope.childrenScopes) ?? scope; + } + if (comparison < 0) { + high = middle - 1; + } else { + low = middle + 1; + } + } + return undefined; + } for (let scope of functionScopes) { if (util.rangeContains(scope.range, position)) { //see if any of that scope's children match the position also, and give them priority From ae2074ee758a026fb277c849bd23a0a3fe6a9f1d Mon Sep 17 00:00:00 2001 From: Rob Reed Date: Sat, 10 Oct 2026 20:08:05 -0700 Subject: [PATCH 3/4] Resolve names declared in the file once rather than in every scope. iterateFileExpressions looked up the symbol table and the first name of every expression again for each scope that includes the file. The tables from an expression up to the file are the same in every scope; only the file table links on to the scope. So the symbol table, and whether the first name is declared in the file, are found once per file. A name declared in the file is always known, and in a scope with no namespaces or enums it has nothing further to check. Expressions inside a namespace are left to the full check, since namespace tables gain scope specific siblings. --- src/bscPlugin/validation/ScopeValidator.ts | 47 +++++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index cd2268a61..a378c49ed 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -11,6 +11,7 @@ import type { BRSComponentData } from '../../roku-types'; import type { Token } from '../../lexer/Token'; import { TokenKind } from '../../lexer/TokenKind'; import type { Scope } from '../../Scope'; +import type { SymbolTable } from '../../SymbolTable'; import type { DiagnosticRelatedInformation } from 'vscode-languageserver'; import type { Expression } from '../../parser/AstNode'; import type { VariableExpression, DottedGetExpression } from '../../parser/Expression'; @@ -168,19 +169,30 @@ export class ScopeValidator { const parts = util.getDottedGetPath(expression); if (parts.length > 0) { + const symbolTable = expression.getSymbolTable(); + const enclosingNamespaceNameLower = expression.findAncestor(isNamespaceStatement)?.getName(ParseMode.BrighterScript)?.toLowerCase(); result.push({ parts: parts, expression: expression, - enclosingNamespaceNameLower: expression.findAncestor(isNamespaceStatement)?.getName(ParseMode.BrighterScript)?.toLowerCase() + symbolTable: symbolTable, + //namespace symbol tables gain scope specific siblings, so only resolve outside of namespaces + isDeclaredInFile: !enclosingNamespaceNameLower && this.isDeclaredInFile(file, symbolTable, parts[0].name?.text), + enclosingNamespaceNameLower: enclosingNamespaceNameLower }); } } return result as unknown as Readonly[]; }); + //without namespaces or enums, a name declared in the file is known and has nothing further to check + const scopeHasNamespacesOrEnums = scope.namespaceLookup.size > 0 || scope.getEnumMap().size > 0; + outer: for (const info of expressionInfos) { - const symbolTable = info.expression.getSymbolTable(); + if (info.isDeclaredInFile && !scopeHasNamespacesOrEnums) { + continue; + } + const symbolTable = info.symbolTable; const firstPart = info.parts[0]; const firstNamespacePart = info.parts[0].name.text; const firstNamespacePartLower = firstNamespacePart?.toLowerCase(); @@ -189,6 +201,7 @@ export class ScopeValidator { //flag all unknown left-most variables if ( + !info.isDeclaredInFile && !symbolTable?.hasSymbol(firstPart.name?.text) && !namespaceContainer ) { @@ -283,6 +296,26 @@ export class ScopeValidator { } } + /** + * Whether the name is declared in the symbol table or one of its ancestors up to the file, which hold the same + * symbols in every scope. Only the file symbol table links on to the scope. + */ + private isDeclaredInFile(file: BrsFile, symbolTable: SymbolTable | undefined, name: string | undefined) { + if (!name) { + return false; + } + const fileSymbolTable = file.parser.symbolTable; + for (let table = symbolTable; table; table = table.parent) { + if (table.hasSymbol(name, false)) { + return true; + } + if (table === fileSymbolTable) { + return false; + } + } + return false; + } + /** * Given a string optionally separated by dots, find an enum related to it. * For example, all of these would return the enum: `SomeNamespace.SomeEnum.SomeMember`, SomeEnum.SomeMember, `SomeEnum` @@ -560,6 +593,16 @@ export class ScopeValidator { interface ExpressionInfo { parts: Readonly<[VariableExpression, ...DottedGetExpression[]]>; expression: Readonly; + /** + * The closest symbol table to the expression, looked up once per file rather than once per scope. + * The table object is the same in every scope; a scope links its namespace siblings onto it while it + * validates, so lookups through it still see the current scope. + */ + symbolTable: SymbolTable | undefined; + /** + * The first part is declared in this file, below the scope, so it is known in every scope + */ + isDeclaredInFile: boolean; /** * The full namespace name that encloses this expression */ From 33c600dd8ef0805d36e8b6b92ab82323ce8565aa Mon Sep 17 00:00:00 2001 From: Rob Reed Date: Sat, 10 Oct 2026 21:00:08 -0700 Subject: [PATCH 4/4] Find computed AA keys once per file rather than in every scope. validateComputedAAKeys walked the whole tree of every file again for each scope that includes it, looking for AAIndexedMemberExpression nodes. The nodes a file holds do not depend on the scope, so collect them once per file and check each against the scope. Most files have none, so the check costs nothing for them. --- src/bscPlugin/validation/ScopeValidator.ts | 116 ++++++++++++--------- 1 file changed, 65 insertions(+), 51 deletions(-) diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index a378c49ed..9baea47e9 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -14,7 +14,7 @@ import type { Scope } from '../../Scope'; import type { SymbolTable } from '../../SymbolTable'; import type { DiagnosticRelatedInformation } from 'vscode-languageserver'; import type { Expression } from '../../parser/AstNode'; -import type { VariableExpression, DottedGetExpression } from '../../parser/Expression'; +import type { AAIndexedMemberExpression, VariableExpression, DottedGetExpression } from '../../parser/Expression'; import { ParseMode } from '../../parser/Parser'; import { createVisitor, WalkMode } from '../../astUtils/visitors'; @@ -47,6 +47,7 @@ export class ScopeValidator { this.event = undefined; this.onceCache.clear(); this.multiScopeCache.clear(); + this.aaIndexedMembersByFile.clear(); } private walkFiles() { @@ -59,64 +60,77 @@ export class ScopeValidator { }); } + private aaIndexedMembersByFile = new Cache(); private validateComputedAAKeys(file: BrsFile) { const { scope } = this.event; - file.ast.walk(createVisitor({ - AAIndexedMemberExpression: (member) => { - // Direct string literal (e.g. ["my-key"]) is valid - if (isLiteralExpression(member.key)) { - if (member.key.token.kind !== TokenKind.StringLiteral) { - this.addMultiScopeDiagnostic({ - file: file, - ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), - range: member.key.range - }); - } - return; - } - const parts = util.getDottedGetPath(member.key); - if (parts.length === 0) { - this.addMultiScopeDiagnostic({ - file: file, - ...DiagnosticMessages.computedPropertyKeyMustBeConstantExpression(), - range: member.key.range - }); - return; - } - const enclosingNamespace = member.key.findAncestor(isNamespaceStatement)?.getName(ParseMode.BrighterScript)?.toLowerCase(); - const entityName = parts.map(p => p.name.text.toLowerCase()).join('.'); - // Check enum member - const memberLink = scope.getEnumMemberFileLink(entityName, enclosingNamespace); - if (memberLink) { - const value = memberLink.item.getValue(); - if (!value?.startsWith('"')) { - this.addMultiScopeDiagnostic({ - file: file, - ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), - range: member.key.range - }); - } - return; - } - // Check const — follow the chain to find the root literal type - const constLink = scope.getConstFileLink(entityName, enclosingNamespace); - if (constLink) { - if (!this.constResolvesToString(constLink.item.value, enclosingNamespace, scope)) { - this.addMultiScopeDiagnostic({ - file: file, - ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), - range: member.key.range - }); - } - return; + //find the computed keys ONCE per file, rather than walking the whole tree again in every scope + const members = this.aaIndexedMembersByFile.getOrAdd(file, () => { + const result = [] as AAIndexedMemberExpression[]; + file.ast.walk(createVisitor({ + AAIndexedMemberExpression: (member) => { + result.push(member); } + }), { walkMode: WalkMode.visitAllRecursive }); + return result; + }); + for (const member of members) { + this.validateComputedAAKey(file, scope, member); + } + } + + private validateComputedAAKey(file: BrsFile, scope: Scope, member: AAIndexedMemberExpression) { + // Direct string literal (e.g. ["my-key"]) is valid + if (isLiteralExpression(member.key)) { + if (member.key.token.kind !== TokenKind.StringLiteral) { + this.addMultiScopeDiagnostic({ + file: file, + ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), + range: member.key.range + }); + } + return; + } + const parts = util.getDottedGetPath(member.key); + if (parts.length === 0) { + this.addMultiScopeDiagnostic({ + file: file, + ...DiagnosticMessages.computedPropertyKeyMustBeConstantExpression(), + range: member.key.range + }); + return; + } + const enclosingNamespace = member.key.findAncestor(isNamespaceStatement)?.getName(ParseMode.BrighterScript)?.toLowerCase(); + const entityName = parts.map(p => p.name.text.toLowerCase()).join('.'); + // Check enum member + const memberLink = scope.getEnumMemberFileLink(entityName, enclosingNamespace); + if (memberLink) { + const value = memberLink.item.getValue(); + if (!value?.startsWith('"')) { + this.addMultiScopeDiagnostic({ + file: file, + ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), + range: member.key.range + }); + } + return; + } + // Check const — follow the chain to find the root literal type + const constLink = scope.getConstFileLink(entityName, enclosingNamespace); + if (constLink) { + if (!this.constResolvesToString(constLink.item.value, enclosingNamespace, scope)) { this.addMultiScopeDiagnostic({ file: file, - ...DiagnosticMessages.computedPropertyKeyMustBeConstantExpression(), + ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), range: member.key.range }); } - }), { walkMode: WalkMode.visitAllRecursive }); + return; + } + this.addMultiScopeDiagnostic({ + file: file, + ...DiagnosticMessages.computedPropertyKeyMustBeConstantExpression(), + range: member.key.range + }); } /**