Fix bug with duplicate components in program.

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,20 @@ 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 an error to have multiple components with the sane name. However, we do store an array of components
+     * by name. With this approach we can provide a better developer expreience.
      */
-    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
+            //(because they are sorted by pkgPath, this should provide a consistent read experience)
+            return this.components[componentName.toLowerCase()]?.[0];
         } else {
             return undefined;
         }
@@ -183,18 +188,50 @@ 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 xmlFile = components[i].file;
+            //re-attach the dependencyGraph for every component whose position changed
+            if (xmlFile.dependencyGraphIndex !== i) {
+                xmlFile.dependencyGraphIndex = i;
+                xmlFile.attachDependencyGraph(this.dependencyGraph);
+            }
+        }
     }
 
     /**
@@ -382,6 +419,7 @@ export class Program {
 
                 //create a new scope for this xml file
                 let scope = new XmlScope(xmlFile, this);
+                scope.attachDependencyGraph(this.dependencyGraph);
                 this.addScope(scope);
 
                 //register this compoent now that we have parsed it and know its component name
@@ -413,7 +451,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,24 +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 { DependencyGraph } 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), true)
-        );
     }
 
     /**
@@ -42,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,
@@ -128,9 +126,7 @@ export class Scope {
      * Clean up all event handles
      */
     public dispose() {
-        for (let disconnect of this.programHandles) {
-            disconnect();
-        }
+        this.unsubscribeFromDependencyGraph?.();
     }
 
     /**
@@ -169,6 +165,22 @@ 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 changes, clean up some cached values
+        this.unsubscribeFromDependencyGraph = this.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this), true);
+    }
+
     /**
      * Get the file with the specified pkgPath
      */
@@ -198,7 +210,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.ts

@@ -195,7 +195,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

@@ -1159,4 +1159,61 @@ 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

@@ -71,7 +71,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;
         });
     }
@@ -86,7 +86,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;
         });
     }
@@ -246,17 +246,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();
         });
@@ -286,20 +289,32 @@ 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. If this value is larger than 0, it will be appended to the dependency graph key
+     * so this component won't collide with the primary component
+     */
+    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 (this.dependencyGraphIndex > 0) {
+            key += '[' + this.dependencyGraphIndex + ']';
         }
+        return key;
     }
 
     /**