From f3c1dbda7ac4681973b18a580242ef0184fec295 Mon Sep 17 00:00:00 2001
From: Bronley Plumb <bronley@gmail.com>
Date: Thu, 14 Apr 2022 09:58:03 -0400
Subject: [PATCH] Create object fixes (#568)

* Move CreateObject validation into plugin

* Only add single CreateObject diagnostics for all scopes

* Don't flag component library CreateObject calls

* Fix lint issues
---
 src/Scope.spec.ts                          |  50 +++++++
 src/Scope.ts                               |  82 +-----------
 src/bscPlugin/BscPlugin.ts                 |  10 +-
 src/bscPlugin/validation/ScopeValidator.ts | 143 ++++++++++++++++++---
 src/util.ts                                |   7 +
 5 files changed, 195 insertions(+), 97 deletions(-)

diff --git a/src/Scope.spec.ts b/src/Scope.spec.ts
index 311c7bc9a..62553bafc 100644
--- a/src/Scope.spec.ts
+++ b/src/Scope.spec.ts
@@ -210,6 +210,56 @@ describe('Scope', () => {
                 ]);
             });
 
+            it('only adds a single diagnostic when the file is used in multiple scopes', () => {
+                program.setFile('components/Comp1.xml', trim`
+                    <?xml version="1.0" encoding="utf-8" ?>
+                    <component name="Comp1" extends="Scene">
+                        <script type="text/brightscript" uri="lib.brs" />
+                    </component>
+                `);
+                program.setFile('components/Comp2.xml', trim`
+                    <?xml version="1.0" encoding="utf-8" ?>
+                    <component name="Comp2" extends="Scene">
+                        <script type="text/brightscript" uri="lib.brs" />
+                    </component>
+                `);
+                program.setFile('components/lib.brs', `
+                    sub init()
+
+                        'unknown BrightScript component
+                        createObject("roDateTime_FAKE")
+
+                        'Wrong number of params
+                        createObject("roDateTime", "this param should not be here")
+
+                        'unknown roSGNode
+                        createObject("roSGNode", "Rectangle_FAKE")
+
+                        'deprecated
+                        fontMetrics = CreateObject("roFontMetrics", "someFontName")
+                    end sub
+                `);
+                program.validate();
+                expectDiagnostics(program, [
+                    DiagnosticMessages.unknownBrightScriptComponent('roDateTime_FAKE'),
+                    DiagnosticMessages.mismatchCreateObjectArgumentCount('roDateTime', [1, 1], 2),
+                    DiagnosticMessages.unknownRoSGNode('Rectangle_FAKE'),
+                    DiagnosticMessages.deprecatedBrightScriptComponent('roFontMetrics').code
+                ]);
+            });
+
+            it('disregards component library components', () => {
+                program.setFile(`source/file.brs`, `
+                    sub main()
+                        scene = CreateObject("roSGNode", "Complib1:MainScene")
+                        button = CreateObject("roSGNode", "buttonlib:Button")
+                        list = CreateObject("roSGNode", "listlib:List")
+                    end sub
+                `);
+                program.validate();
+                expectZeroDiagnostics(program);
+            });
+
             it('disregards non-literal args', () => {
                 program.setFile(`source/file.brs`, `
                     sub main()
diff --git a/src/Scope.ts b/src/Scope.ts
index 69a2bbdfe..132a4dc95 100644
--- a/src/Scope.ts
+++ b/src/Scope.ts
@@ -17,16 +17,7 @@ import { URI } from 'vscode-uri';
 import { LogLevel } from './Logger';
 import type { BrsFile } from './files/BrsFile';
 import type { DependencyGraph, DependencyChangedEvent } from './DependencyGraph';
-import { isBrsFile, isClassMethodStatement, isClassStatement, isCustomType, isEnumStatement, isFunctionStatement, isFunctionType, isLiteralExpression, isXmlFile } from './astUtils/reflection';
-import { nodes, components } from './roku-types';
-import type { BRSComponentData } from './roku-types';
-import type { Token } from './lexer/Token';
-
-/**
- * The lower-case names of all platform-included scenegraph nodes
- */
-const platformNodeNames = new Set(Object.values(nodes).map(x => x.name.toLowerCase()));
-const platformComponentNames = new Set(Object.values(components).map(x => x.name.toLowerCase()));
+import { isBrsFile, isClassMethodStatement, isClassStatement, isCustomType, isEnumStatement, isFunctionStatement, isFunctionType, isXmlFile } from './astUtils/reflection';
 
 /**
  * A class to keep track of all declarations within a given scope (like source scope, component scope)
@@ -240,7 +231,7 @@ export class Scope {
      * Get the list of files referenced by this scope that are actually loaded in the program.
      * Includes files from this scope and all ancestor scopes
      */
-    public getAllFiles() {
+    public getAllFiles(): BscFile[] {
         return this.cache.getOrAdd('getAllFiles', () => {
             let result = [] as BscFile[];
             let dependencies = this.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
@@ -503,78 +494,9 @@ export class Scope {
             this.diagnosticDetectFunctionCollisions(file);
             this.detectVariableNamespaceCollisions(file);
             this.diagnosticDetectInvalidFunctionExpressionTypes(file);
-            this.validateCreateObjectCalls(file);
         });
     }
 
-    /**
-     * Validate every function call to `CreateObject`.
-     * Ideally we would create better type checking/handling for this, but in the mean time, we know exactly
-     * what these calls are supposed to look like, and this is a very common thing for brs devs to do, so just
-     * do this manually for now.
-     */
-    protected validateCreateObjectCalls(file: BrsFile) {
-        for (const call of file.functionCalls) {
-            if (call.name?.toLowerCase() === 'createobject' && isLiteralExpression(call?.args[0]?.expression)) {
-                const firstParamToken = (call?.args[0]?.expression as any)?.token;
-                const firstParamStringValue = firstParamToken?.text?.replace(/"/g, '');
-                //if this is a `createObject('roSGNode'` call, only support known sg node types
-                if (firstParamStringValue?.toLowerCase() === 'rosgnode' && isLiteralExpression(call?.args[1]?.expression)) {
-                    const componentName: Token = (call?.args[1]?.expression as any)?.token;
-                    //add diagnostic for unknown components
-                    const unquotedComponentName = componentName?.text?.replace(/"/g, '');
-                    if (unquotedComponentName && !platformNodeNames.has(unquotedComponentName.toLowerCase()) && !this.program.getComponent(unquotedComponentName)) {
-                        this.diagnostics.push({
-                            file: file as BscFile,
-                            ...DiagnosticMessages.unknownRoSGNode(unquotedComponentName),
-                            range: componentName.range
-                        });
-                    } else if (call?.args.length !== 2) {
-                        // roSgNode should only ever have 2 args in `createObject`
-                        this.diagnostics.push({
-                            file: file as BscFile,
-                            ...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, [2], call?.args.length),
-                            range: call.range
-                        });
-                    }
-                } else if (!platformComponentNames.has(firstParamStringValue.toLowerCase())) {
-                    this.diagnostics.push({
-                        file: file as BscFile,
-                        ...DiagnosticMessages.unknownBrightScriptComponent(firstParamStringValue),
-                        range: firstParamToken.range
-                    });
-                } else {
-                    // This is valid brightscript component
-                    // Test for invalid arg counts
-                    const brightScriptComponent: BRSComponentData = components[firstParamStringValue.toLowerCase()];
-                    // Valid arg counts for createObject are 1+ number of args for constructor
-                    let validArgCounts = brightScriptComponent.constructors.map(cnstr => cnstr.params.length + 1);
-                    if (validArgCounts.length === 0) {
-                        // no constructors for this component, so createObject only takes 1 arg
-                        validArgCounts = [1];
-                    }
-                    if (!validArgCounts.includes(call?.args.length)) {
-                        // Incorrect number of arguments included in `createObject()`
-                        this.diagnostics.push({
-                            file: file as BscFile,
-                            ...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, validArgCounts, call?.args.length),
-                            range: call.range
-                        });
-                    }
-
-                    // Test for deprecation
-                    if (brightScriptComponent.isDeprecated) {
-                        this.diagnostics.push({
-                            file: file as BscFile,
-                            ...DiagnosticMessages.deprecatedBrightScriptComponent(firstParamStringValue, brightScriptComponent.deprecatedDescription),
-                            range: call.range
-                        });
-                    }
-                }
-            }
-        }
-    }
-
     /**
      * Mark this scope as invalid, which means its `validate()` function needs to be called again before use.
      */
diff --git a/src/bscPlugin/BscPlugin.ts b/src/bscPlugin/BscPlugin.ts
index b4a09ccc1..7f83ce7d6 100644
--- a/src/bscPlugin/BscPlugin.ts
+++ b/src/bscPlugin/BscPlugin.ts
@@ -1,6 +1,7 @@
 import { isBrsFile } from '../astUtils/reflection';
 import type { BrsFile } from '../files/BrsFile';
 import type { BeforeFileTranspileEvent, CompilerPlugin, OnFileValidateEvent, OnGetCodeActionsEvent, OnGetSemanticTokensEvent, OnScopeValidateEvent } from '../interfaces';
+import type { Program } from '../Program';
 import { CodeActionsProcessor } from './codeActions/CodeActionsProcessor';
 import { BrsFileSemanticTokensProcessor } from './semanticTokens/BrsFileSemanticTokensProcessor';
 import { BrsFilePreTranspileProcessor } from './transpile/BrsFilePreTranspileProcessor';
@@ -26,8 +27,15 @@ export class BscPlugin implements CompilerPlugin {
         }
     }
 
+    private scopeValidator = new ScopeValidator();
+
     public onScopeValidate(event: OnScopeValidateEvent) {
-        return new ScopeValidator(event).process();
+        this.scopeValidator.processEvent(event);
+    }
+
+    public afterProgramValidate(program: Program) {
+        //release memory once the validation cycle has finished
+        this.scopeValidator.reset();
     }
 
     public beforeFileTranspile(event: BeforeFileTranspileEvent) {
diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts
index 50fc1ad9f..6840f0af1 100644
--- a/src/bscPlugin/validation/ScopeValidator.ts
+++ b/src/bscPlugin/validation/ScopeValidator.ts
@@ -1,42 +1,74 @@
 import { Location } from 'vscode-languageserver';
 import { URI } from 'vscode-uri';
-import { isBrsFile } from '../../astUtils/reflection';
+import { isBrsFile, isLiteralExpression } from '../../astUtils/reflection';
 import { Cache } from '../../Cache';
 import { DiagnosticMessages } from '../../DiagnosticMessages';
 import type { BrsFile } from '../../files/BrsFile';
-import type { BsDiagnostic, OnScopeValidateEvent } from '../../interfaces';
+import type { BscFile, BsDiagnostic, OnScopeValidateEvent } from '../../interfaces';
 import type { DottedGetExpression } from '../../parser/Expression';
 import type { EnumStatement } from '../../parser/Statement';
 import util from '../../util';
+import { nodes, components } from '../../roku-types';
+import type { BRSComponentData } from '../../roku-types';
+import type { Token } from '../../lexer/Token';
 
+/**
+ * The lower-case names of all platform-included scenegraph nodes
+ */
+const platformNodeNames = new Set(Object.values(nodes).map(x => x.name.toLowerCase()));
+const platformComponentNames = new Set(Object.values(components).map(x => x.name.toLowerCase()));
+
+/**
+ * A validator that handles all scope validations for a program validation cycle.
+ * You should create ONE of these to handle all scope events between beforeProgramValidate and afterProgramValidate,
+ * and call reset() before using it again in the next cycle
+ */
 export class ScopeValidator {
-    constructor(
-        private event: OnScopeValidateEvent
-    ) {
+
+    private events: OnScopeValidateEvent[] = [];
+
+    public processEvent(event: OnScopeValidateEvent) {
+        this.events.push(event);
+        this.validateEnumUsage(event);
+        this.detectDuplicateEnums(event);
+        this.validateCreateObjectCalls(event);
+    }
+
+    public reset() {
+        this.cache.clear();
+        this.events = [];
     }
 
-    public process() {
-        this.validateEnumUsage();
-        this.detectDuplicateEnums();
+    /**
+     * Adds a diagnostic to the first scope for this key. Prevents duplicate diagnostics
+     * for diagnostics where scope isn't important. (i.e. CreateObject validations)
+     */
+    private addDiagnosticOnce(event: OnScopeValidateEvent, diagnostic: BsDiagnostic) {
+        const key = `${diagnostic.code}-${diagnostic.message}-${util.rangeToString(diagnostic.range)}`;
+        if (!this.cache.has(key)) {
+            this.cache.set(key, true);
+            event.scope.addDiagnostics([diagnostic]);
+        }
     }
 
+    private cache = new Map<string, boolean>();
 
     /**
      * Find all expressions and validate the ones that look like enums
      */
-    public validateEnumUsage() {
+    public validateEnumUsage(event: OnScopeValidateEvent) {
         const diagnostics = [] as BsDiagnostic[];
 
         const membersByEnum = new Cache<string, Map<string, string>>();
         //if there are any enums defined in this scope
-        const enumLookup = this.event.scope.getEnumMap();
+        const enumLookup = event.scope.getEnumMap();
 
         //skip enum validation if there are no enums defined in this scope
         if (enumLookup.size === 0) {
             return;
         }
 
-        this.event.scope.enumerateOwnFiles((file) => {
+        event.scope.enumerateOwnFiles((file) => {
             //skip non-brs files
             if (!isBrsFile(file)) {
                 return;
@@ -74,13 +106,13 @@ export class ScopeValidator {
                 }
             }
         });
-        this.event.scope.addDiagnostics(diagnostics);
+        event.scope.addDiagnostics(diagnostics);
     }
 
-    private detectDuplicateEnums() {
+    private detectDuplicateEnums(event: OnScopeValidateEvent) {
         const diagnostics: BsDiagnostic[] = [];
         const enumLocationsByName = new Cache<string, Array<{ file: BrsFile; statement: EnumStatement }>>();
-        this.event.scope.enumerateBrsFiles((file) => {
+        event.scope.enumerateBrsFiles((file) => {
             for (const enumStatement of file.parser.references.enumStatements) {
                 const fullName = enumStatement.fullName;
                 const nameLower = fullName?.toLowerCase();
@@ -101,7 +133,7 @@ export class ScopeValidator {
             const fullName = primaryEnum.statement.fullName;
             for (const duplicateEnumInfo of enumLocations) {
                 diagnostics.push({
-                    ...DiagnosticMessages.duplicateEnumDeclaration(this.event.scope.name, fullName),
+                    ...DiagnosticMessages.duplicateEnumDeclaration(event.scope.name, fullName),
                     file: duplicateEnumInfo.file,
                     range: duplicateEnumInfo.statement.tokens.name.range,
                     relatedInformation: [{
@@ -114,6 +146,85 @@ export class ScopeValidator {
                 });
             }
         }
-        this.event.scope.addDiagnostics(diagnostics);
+        event.scope.addDiagnostics(diagnostics);
+    }
+
+    /**
+     * Validate every function call to `CreateObject`.
+     * Ideally we would create better type checking/handling for this, but in the mean time, we know exactly
+     * what these calls are supposed to look like, and this is a very common thing for brs devs to do, so just
+     * do this manually for now.
+     */
+    protected validateCreateObjectCalls(event: OnScopeValidateEvent) {
+        const diagnostics: BsDiagnostic[] = [];
+
+        event.scope.enumerateBrsFiles((file) => {
+            for (const call of file.functionCalls) {
+                //skip non CreateObject function calls
+                if (call.name?.toLowerCase() !== 'createobject' || !isLiteralExpression(call?.args[0]?.expression)) {
+                    continue;
+                }
+                const firstParamToken = (call?.args[0]?.expression as any)?.token;
+                const firstParamStringValue = firstParamToken?.text?.replace(/"/g, '');
+                //if this is a `createObject('roSGNode'` call, only support known sg node types
+                if (firstParamStringValue?.toLowerCase() === 'rosgnode' && isLiteralExpression(call?.args[1]?.expression)) {
+                    const componentName: Token = (call?.args[1]?.expression as any)?.token;
+                    //don't validate any components with a colon in their name (probably component libraries, but regular components can have them too).
+                    if (componentName?.text?.includes(':')) {
+                        continue;
+                    }
+                    //add diagnostic for unknown components
+                    const unquotedComponentName = componentName?.text?.replace(/"/g, '');
+                    if (unquotedComponentName && !platformNodeNames.has(unquotedComponentName.toLowerCase()) && !event.program.getComponent(unquotedComponentName)) {
+                        this.addDiagnosticOnce(event, {
+                            file: file as BscFile,
+                            ...DiagnosticMessages.unknownRoSGNode(unquotedComponentName),
+                            range: componentName.range
+                        });
+                    } else if (call?.args.length !== 2) {
+                        // roSgNode should only ever have 2 args in `createObject`
+                        this.addDiagnosticOnce(event, {
+                            file: file as BscFile,
+                            ...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, [2], call?.args.length),
+                            range: call.range
+                        });
+                    }
+                } else if (!platformComponentNames.has(firstParamStringValue.toLowerCase())) {
+                    this.addDiagnosticOnce(event, {
+                        file: file as BscFile,
+                        ...DiagnosticMessages.unknownBrightScriptComponent(firstParamStringValue),
+                        range: firstParamToken.range
+                    });
+                } else {
+                    // This is valid brightscript component
+                    // Test for invalid arg counts
+                    const brightScriptComponent: BRSComponentData = components[firstParamStringValue.toLowerCase()];
+                    // Valid arg counts for createObject are 1+ number of args for constructor
+                    let validArgCounts = brightScriptComponent.constructors.map(cnstr => cnstr.params.length + 1);
+                    if (validArgCounts.length === 0) {
+                        // no constructors for this component, so createObject only takes 1 arg
+                        validArgCounts = [1];
+                    }
+                    if (!validArgCounts.includes(call?.args.length)) {
+                        // Incorrect number of arguments included in `createObject()`
+                        this.addDiagnosticOnce(event, {
+                            file: file as BscFile,
+                            ...DiagnosticMessages.mismatchCreateObjectArgumentCount(firstParamStringValue, validArgCounts, call?.args.length),
+                            range: call.range
+                        });
+                    }
+
+                    // Test for deprecation
+                    if (brightScriptComponent.isDeprecated) {
+                        this.addDiagnosticOnce(event, {
+                            file: file as BscFile,
+                            ...DiagnosticMessages.deprecatedBrightScriptComponent(firstParamStringValue, brightScriptComponent.deprecatedDescription),
+                            range: call.range
+                        });
+                    }
+                }
+            }
+        });
+        event.scope.addDiagnostics(diagnostics);
     }
 }
diff --git a/src/util.ts b/src/util.ts
index dbb74399a..6c05113f9 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -1323,6 +1323,13 @@ export class Util {
             return undefined;
         }
     }
+
+    /**
+     * Converts a range to a string in the format 1:2-3:4
+     */
+    public rangeToString(range: Range) {
+        return `${range?.start?.line}:${range?.start?.character}-${range?.end?.line}:${range?.end?.character}`;
+    }
 }
 
 /**