[XELP] Speed up scope validation when many scopes share files

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) {

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<BrsFile, AAIndexedMemberExpression[]>();
     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<ExpressionInfo>[];
         });
 
+        //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<Expression>;
+    /**
+     * 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
      */

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()

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