Fix irrecoverable compiler state on duplicate components (#353)

* Fix bug with duplicate components in program.

* tweak XmlFile dependencyGraphIndex

* removed exclusive test

* comment tweaks.

src/Program.ts

@@ -85,7 +85,8 @@ export class Program {
 
     private createGlobalScope() {
         //create the 'global' scope
-        this.globalScope = new Scope('global', 'scope:global', this);
+        this.globalScope = new Scope('global', this, 'scope:global');
+        this.globalScope.attachDependencyGraph(this.dependencyGraph);
         this.scopes.global = this.globalScope;
         //hardcode the files list for global scope to only contain the global file
         this.globalScope.getAllFiles = () => [globalFile];
@@ -102,7 +103,7 @@ export class Program {
      *      File.xml -> [lib1.brs, lib2.brs]
      *      lib2.brs -> [lib3.brs] //via an import statement
      */
-    public dependencyGraph = new DependencyGraph();
+    private dependencyGraph = new DependencyGraph();
 
     private diagnosticFilterer = new DiagnosticFilterer();
 
@@ -164,16 +165,21 @@ export class Program {
     }
 
     /**
-     * A map of every component currently loaded into the program, indexed by the component name
+     * A map of every component currently loaded into the program, indexed by the component name.
+     * It is a compile-time error to have multiple components with the same name. However, we store an array of components
+     * by name so we can provide a better developer expreience. You shouldn't be directly accessing this array,
+     * but if you do, only ever use the component at index 0.
      */
-    private components = {} as Record<string, { file: XmlFile; scope: XmlScope }>;
+    private components = {} as Record<string, { file: XmlFile; scope: XmlScope }[]>;
 
     /**
      * Get the component with the specified name
      */
     public getComponent(componentName: string) {
         if (componentName) {
-            return this.components[componentName.toLowerCase()];
+            //return the first compoment in the list with this name
+            //(components are ordered in this list by pkgPath to ensure consistency)
+            return this.components[componentName.toLowerCase()]?.[0];
         } else {
             return undefined;
         }
@@ -183,18 +189,52 @@ export class Program {
      * Register (or replace) the reference to a component in the component map
      */
     private registerComponent(xmlFile: XmlFile, scope: XmlScope) {
-        //store a reference to this component by its component name
-        this.components[(xmlFile.componentName?.text ?? xmlFile.pkgPath).toLowerCase()] = {
+        const key = (xmlFile.componentName?.text ?? xmlFile.pkgPath).toLowerCase();
+        if (!this.components[key]) {
+            this.components[key] = [];
+        }
+        this.components[key].push({
             file: xmlFile,
             scope: scope
-        };
+        });
+        this.components[key].sort(
+            (x, y) => x.file.pkgPath.toLowerCase().localeCompare(y.file.pkgPath.toLowerCase())
+        );
+        this.syncComponentDependencyGraph(this.components[key]);
     }
 
     /**
      * Remove the specified component from the components map
      */
     private unregisterComponent(xmlFile: XmlFile) {
-        delete this.components[(xmlFile.componentName?.text ?? xmlFile.pkgPath).toLowerCase()];
+        const key = (xmlFile.componentName?.text ?? xmlFile.pkgPath).toLowerCase();
+        const arr = this.components[key] || [];
+        for (let i = 0; i < arr.length; i++) {
+            if (arr[i].file === xmlFile) {
+                arr.splice(i, 1);
+                break;
+            }
+        }
+        this.syncComponentDependencyGraph(arr);
+    }
+
+    /**
+     * re-attach the dependency graph with a new key for any component who changed
+     * their position in their own named array (only matters when there are multiple
+     * components with the same name)
+     */
+    private syncComponentDependencyGraph(components: Array<{ file: XmlFile; scope: XmlScope }>) {
+        //reattach every dependency graph
+        for (let i = 0; i < components.length; i++) {
+            const { file, scope } = components[i];
+
+            //attach (or re-attach) the dependencyGraph for every component whose position changed
+            if (file.dependencyGraphIndex !== i) {
+                file.dependencyGraphIndex = i;
+                file.attachDependencyGraph(this.dependencyGraph);
+                scope.attachDependencyGraph(this.dependencyGraph);
+            }
+        }
     }
 
     /**
@@ -387,11 +427,6 @@ export class Program {
                 //register this compoent now that we have parsed it and know its component name
                 this.registerComponent(xmlFile, scope);
 
-                //attach the dependency graph, so the component can
-                //   a) be regularly notified of changes
-                //   b) immediately emit its own changes
-                xmlFile.attachDependencyGraph(this.dependencyGraph);
-
                 this.plugins.emit('afterFileValidate', xmlFile);
             } else {
                 //TODO do we actually need to implement this? Figure out how to handle img paths
@@ -413,7 +448,8 @@ export class Program {
      */
     public createSourceScope() {
         if (!this.scopes.source) {
-            const sourceScope = new Scope('source', 'scope:source', this);
+            const sourceScope = new Scope('source', this, 'scope:source');
+            sourceScope.attachDependencyGraph(this.dependencyGraph);
             this.addScope(sourceScope);
         }
     }

src/Scope.ts

@@ -16,27 +16,20 @@ import { URI } from 'vscode-uri';
 import { LogLevel } from './Logger';
 import { isBrsFile, isClassStatement, isFunctionStatement, isFunctionType, isXmlFile, isCustomType, isClassMethodStatement } from './astUtils/reflection';
 import type { BrsFile } from './files/BrsFile';
-import type { DependencyChangedEvent } from './DependencyGraph';
+import type { DependencyGraph, DependencyChangedEvent } from './DependencyGraph';
 
 /**
  * A class to keep track of all declarations within a given scope (like source scope, component scope)
  */
 export class Scope {
     constructor(
         public name: string,
-        public dependencyGraphKey: string,
-        public program: Program
+        public program: Program,
+        private _dependencyGraphKey?: string
     ) {
         this.isValidated = false;
         //used for improved logging performance
         this._debugLogComponentName = `Scope '${chalk.redBright(this.name)}'`;
-
-        //anytime a dependency for this scope changes, we need to be revalidated
-        this.programHandles.push(
-            this.program.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this))
-        );
-        //invalidate immediately since this is a new scope
-        this.invalidate();
     }
 
     /**
@@ -45,10 +38,12 @@ export class Scope {
      */
     public readonly isValidated: boolean;
 
-    protected programHandles = [] as Array<() => void>;
-
     protected cache = new Cache();
 
+    public get dependencyGraphKey() {
+        return this._dependencyGraphKey;
+    }
+
     /**
      * A dictionary of namespaces, indexed by the lower case full name of each namespace.
      * If a namespace is declared as "NameA.NameB.NameC", there will be 3 entries in this dictionary,
@@ -131,9 +126,7 @@ export class Scope {
      * Clean up all event handles
      */
     public dispose() {
-        for (let disconnect of this.programHandles) {
-            disconnect();
-        }
+        this.unsubscribeFromDependencyGraph?.();
     }
 
     /**
@@ -172,6 +165,25 @@ export class Scope {
         }
     }
 
+    private dependencyGraph: DependencyGraph;
+    /**
+     * An unsubscribe function for the dependencyGraph subscription
+     */
+    private unsubscribeFromDependencyGraph: () => void;
+
+    public attachDependencyGraph(dependencyGraph: DependencyGraph) {
+        this.dependencyGraph = dependencyGraph;
+        if (this.unsubscribeFromDependencyGraph) {
+            this.unsubscribeFromDependencyGraph();
+        }
+
+        //anytime a dependency for this scope changes, we need to be revalidated
+        this.unsubscribeFromDependencyGraph = this.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this));
+
+        //invalidate immediately since this is a new scope
+        this.invalidate();
+    }
+
     /**
      * Get the file with the specified pkgPath
      */
@@ -201,7 +213,7 @@ export class Scope {
     public getAllFiles() {
         return this.cache.getOrAdd('getAllFiles', () => {
             let result = [] as BscFile[];
-            let dependencies = this.program.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
+            let dependencies = this.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
             for (let dependency of dependencies) {
                 //load components by their name
                 if (dependency.startsWith('component:')) {

src/XmlScope.ts

@@ -14,7 +14,11 @@ export class XmlScope extends Scope {
         public xmlFile: XmlFile,
         public program: Program
     ) {
-        super(xmlFile.pkgPath, xmlFile.dependencyGraphKey, program);
+        super(xmlFile.pkgPath, program);
+    }
+
+    public get dependencyGraphKey() {
+        return this.xmlFile.dependencyGraphKey;
     }
 
     /**

src/files/BrsFile.spec.ts

@@ -92,6 +92,17 @@ describe('BrsFile', () => {
     });
 
     describe('getCompletions', () => {
+        it('does not crash for callfunc on a function call', () => {
+            const file = program.addOrReplaceFile('source/main.brs', `
+                sub main()
+                    getManager()@.
+                end sub
+            `);
+            expect(() => {
+                program.getCompletions(file.pathAbsolute, util.createPosition(2, 34));
+            }).not.to.throw;
+        });
+
         it('suggests pkg paths in strings that match that criteria', () => {
             program.addOrReplaceFile('source/main.brs', `
                 sub main()

src/files/BrsFile.ts

@@ -196,7 +196,7 @@ export class BrsFile {
         }
 
         //event that fires anytime a dependency changes
-        this.unsubscribeFromDependencyGraph = this.program.dependencyGraph.onchange(this.dependencyGraphKey, () => {
+        this.unsubscribeFromDependencyGraph = dependencyGraph.onchange(this.dependencyGraphKey, () => {
             this.resolveTypedef();
         });
 

src/files/XmlFile.spec.ts

@@ -1160,4 +1160,60 @@ describe('XmlFile', () => {
             ]);
         });
     });
+
+    describe('duplicate components', () => {
+        it('more gracefully handles multiple components with the same name', () => {
+            program.addOrReplaceFile('components/comp1.brs', ``);
+            program.addOrReplaceFile('components/comp1.xml', trim`
+                <?xml version="1.0" encoding="utf-8" ?>
+                <component name="comp1" extends="Group">
+                    <script uri="comp1.brs" />
+                </component>
+            `);
+            //add another component with the same name
+            program.addOrReplaceFile('components/comp2.brs', ``);
+            program.addOrReplaceFile('components/comp2.xml', trim`
+                <?xml version="1.0" encoding="utf-8" ?>
+                <component name="comp1" extends="Group">
+                    <script uri="comp2.brs" />
+                </component>
+            `);
+            program.validate();
+            expect(program.getDiagnostics().map(x => x.message).sort()).to.eql([
+                DiagnosticMessages.duplicateComponentName('comp1').message,
+                DiagnosticMessages.duplicateComponentName('comp1').message
+            ]);
+        });
+
+        it('maintains consistent component selection', () => {
+            //add comp2 first
+            const comp2 = program.addOrReplaceFile('components/comp2.xml', trim`
+                <?xml version="1.0" encoding="utf-8" ?>
+                <component name="comp1">
+                </component>
+            `);
+            expect(program.getComponent('comp1').file.pkgPath).to.equal(comp2.pkgPath);
+
+            //add comp1. it should become the main component with this name
+            const comp1 = program.addOrReplaceFile('components/comp1.xml', trim`
+                <?xml version="1.0" encoding="utf-8" ?>
+                <component name="comp1" extends="Group">
+                </component>
+            `);
+            expect(program.getComponent('comp1').file.pkgPath).to.equal(comp1.pkgPath);
+
+            //remove comp1, comp2 should be the primary again
+            program.removeFile(s`${rootDir}/components/comp1.xml`);
+            expect(program.getComponent('comp1').file.pkgPath).to.equal(comp2.pkgPath);
+
+            //add comp3
+            program.addOrReplaceFile('components/comp3.xml', trim`
+                <?xml version="1.0" encoding="utf-8" ?>
+                <component name="comp1">
+                </component>
+            `);
+            //...the 2nd file should still be main
+            expect(program.getComponent('comp1').file.pkgPath).to.equal(comp2.pkgPath);
+        });
+    });
 });

src/files/XmlFile.ts

@@ -75,7 +75,7 @@ export class XmlFile {
      */
     public getAllDependencies() {
         return this.cache.getOrAdd(`allScriptImports`, () => {
-            const value = this.program.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
+            const value = this.dependencyGraph.getAllDependencies(this.dependencyGraphKey);
             return value;
         });
     }
@@ -90,7 +90,7 @@ export class XmlFile {
      */
     public getOwnDependencies() {
         return this.cache.getOrAdd(`ownScriptImports`, () => {
-            const value = this.program.dependencyGraph.getAllDependencies(this.dependencyGraphKey, [this.parentComponentDependencyGraphKey]);
+            const value = this.dependencyGraph.getAllDependencies(this.dependencyGraphKey, [this.parentComponentDependencyGraphKey]);
             return value;
         });
     }
@@ -272,17 +272,20 @@ export class XmlFile {
         }
     }
 
+    private dependencyGraph: DependencyGraph;
+
     /**
      * Attach the file to the dependency graph so it can monitor changes.
      * Also notify the dependency graph of our current dependencies so other dependents can be notified.
      */
     public attachDependencyGraph(dependencyGraph: DependencyGraph) {
+        this.dependencyGraph = dependencyGraph;
         if (this.unsubscribeFromDependencyGraph) {
             this.unsubscribeFromDependencyGraph();
         }
 
         //anytime a dependency changes, clean up some cached values
-        this.unsubscribeFromDependencyGraph = this.program.dependencyGraph.onchange(this.dependencyGraphKey, () => {
+        this.unsubscribeFromDependencyGraph = dependencyGraph.onchange(this.dependencyGraphKey, () => {
             this.logDebug('clear cache because dependency graph changed');
             this.cache.clear();
         });
@@ -312,20 +315,36 @@ export class XmlFile {
         if (this.parentComponentName) {
             dependencies.push(this.parentComponentDependencyGraphKey);
         }
-        this.program.dependencyGraph.addOrReplace(this.dependencyGraphKey, dependencies);
+        this.dependencyGraph.addOrReplace(this.dependencyGraphKey, dependencies);
     }
 
+    /**
+     * A slight hack. Gives the Program a way to support multiple components with the same name
+     * without causing major issues. A value of 0 will be ignored as part of the dependency graph key.
+     * Howver, a nonzero value will be used as part of the dependency graph key so this component doesn't
+     * collide with the primary component. For example, if there are three components with the same name, you will
+     * have the following dependency graph keys: ["component:CustomGrid", "component:CustomGrid[1]", "component:CustomGrid[2]"]
+     */
+    public dependencyGraphIndex = -1;
+
     /**
      * The key used in the dependency graph for this file.
      * If we have a component name, we will use that so we can be discoverable by child components.
      * If we don't have a component name, use the pkgPath so at least we can self-validate
      */
     public get dependencyGraphKey() {
+        let key: string;
         if (this.componentName) {
-            return `component:${this.componentName.text}`.toLowerCase();
+            key = `component:${this.componentName.text}`.toLowerCase();
         } else {
-            return this.pkgPath.toLowerCase();
+            key = this.pkgPath.toLowerCase();
+        }
+        //if our index is not zero, then we are not the primary component with that name, and need to
+        //append our index to the dependency graph key as to prevent collisions in the program.
+        if (this.dependencyGraphIndex !== 0) {
+            key += '[' + this.dependencyGraphIndex + ']';
         }
+        return key;
     }
 
     /**