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) { diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index cd2268a61..9baea47e9 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -11,9 +11,10 @@ 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'; +import type { AAIndexedMemberExpression, VariableExpression, DottedGetExpression } from '../../parser/Expression'; import { ParseMode } from '../../parser/Parser'; import { createVisitor, WalkMode } from '../../astUtils/visitors'; @@ -46,6 +47,7 @@ export class ScopeValidator { this.event = undefined; this.onceCache.clear(); this.multiScopeCache.clear(); + this.aaIndexedMembersByFile.clear(); } private walkFiles() { @@ -58,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.computedPropertyKeyMustBeConstantExpression(), + ...DiagnosticMessages.computedAAKeyMustBeStringExpression(), range: member.key.range }); } - }), { walkMode: WalkMode.visitAllRecursive }); + 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; + } + this.addMultiScopeDiagnostic({ + file: file, + ...DiagnosticMessages.computedPropertyKeyMustBeConstantExpression(), + range: member.key.range + }); } /** @@ -168,19 +183,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 +215,7 @@ export class ScopeValidator { //flag all unknown left-most variables if ( + !info.isDeclaredInFile && !symbolTable?.hasSymbol(firstPart.name?.text) && !namespaceContainer ) { @@ -283,6 +310,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 +607,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 */ 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