Fixes in ComposedReadOnlySymbolTable (#2720)

- OptionSets should only return first OS by name (first element wins)
- TryGetVariable not working as expected

src/libraries/Microsoft.PowerFx.Core/Public/Config/ComposedReadOnlySymbolTable.cs

@@ -4,7 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using System.Reflection.Metadata;
 using Microsoft.PowerFx.Core.Binding;
 using Microsoft.PowerFx.Core.Binding.BindInfo;
 using Microsoft.PowerFx.Core.Entities;
@@ -22,7 +21,7 @@ namespace Microsoft.PowerFx
     {
         private readonly IEnumerable<ReadOnlySymbolTable> _symbolTables;
 
-        // In priority order. 
+        // In priority order.
         public ComposedReadOnlySymbolTable(params ReadOnlySymbolTable[] symbolTables)
         {
             _symbolTables = symbolTables.Where(x => x != null);
@@ -50,7 +49,7 @@ namespace Microsoft.PowerFx
         {
             if (slot.Owner == this)
             {
-                // A slot's owner must be a "leaf node" symbol table and note 
+                // A slot's owner must be a "leaf node" symbol table and note
                 // a composed type.
                 // Check to avoid recursion.
                 throw new InvalidOperationException("Slot has illegal owner.");
@@ -65,7 +64,7 @@ namespace Microsoft.PowerFx
         // To keep _cachedVersionHash and _nameResolverFunctions in sync.
         private readonly object _lock = new object();
 
-        // Expose the list to aide in intellisense suggestions. 
+        // Expose the list to aide in intellisense suggestions.
         // Multiple readers ok. But not writing while we read.
         TexlFunctionSet INameResolver.Functions
         {
@@ -85,7 +84,7 @@ namespace Microsoft.PowerFx
                         _cachedVersionHash = current;
                     }
 
-                    // Check that it didn't mutate. 
+                    // Check that it didn't mutate.
                     var newHash = this.VersionHash;
                     if (newHash != current)
                     {
@@ -93,7 +92,7 @@ namespace Microsoft.PowerFx
                     }
                 }
 
-                return _nameResolverFunctions;                
+                return _nameResolverFunctions;
             }
         }
 
@@ -168,16 +167,37 @@ namespace Microsoft.PowerFx
         {
             get
             {
+                HashSet<string> osNames = new HashSet<string>();
+
                 foreach (ReadOnlySymbolTable st in _symbolTables)
                 {
                     foreach (KeyValuePair<string, OptionSet> kvp in st.OptionSets)
                     {
-                        yield return kvp;
+                        if (!osNames.Contains(kvp.Key))
+                        {
+                            osNames.Add(kvp.Key);
+                            yield return kvp;
+                        }
                     }
                 }
             }
         }
 
+        internal override bool TryGetVariable(DName name, out NameLookupInfo symbol, out DName displayName)
+        {
+            foreach (ReadOnlySymbolTable st in _symbolTables)
+            {
+                if (st.TryGetVariable(name, out symbol, out displayName))
+                {
+                    return true;
+                }
+            }
+
+            symbol = default;
+            displayName = default;
+            return false;
+        }
+
         public virtual bool Lookup(DName name, out NameLookupInfo nameInfo, NameLookupPreferences preferences = NameLookupPreferences.None)
         {
             foreach (INameResolver table in _symbolTables)

src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs

@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.PowerFx.Core.Binding;
+using Microsoft.PowerFx.Core.Binding.BindInfo;
 using Microsoft.PowerFx.Core.Texl.Builtins;
 using Microsoft.PowerFx.Core.Utils;
 using Microsoft.PowerFx.Types;
@@ -325,6 +326,149 @@ namespace Microsoft.PowerFx.Core.Tests
             Assert.Equal(2, st3.OptionSets.Count());            
         }
 
+        [Fact]
+        public void AddVariableAndOptionSetWithConflicts()
+        {
+            var st1 = new SymbolTable();            
+            st1.AddVariable("var1", FormulaType.String, displayName: "displayName1");
+            var os1 = new OptionSet("os1", DisplayNameProvider.New(new Dictionary<DName, DName>() { { new DName("ln1"), new DName("dn1") } }));
+            var os2 = new OptionSet("os2", DisplayNameProvider.New(new Dictionary<DName, DName>() { { new DName("xx2"), new DName("yy2") } }));
+            st1.AddOptionSet(os1);
+            st1.AddOptionSet(os2);
+
+            // The variable we just added should be there
+            bool b = st1.TryGetVariable(new DName("var1"), out NameLookupInfo info, out DName displayName);
+
+            Assert.True(b);
+            NameSymbol nameSymbol = Assert.IsType<NameSymbol>(info.Data);
+            Assert.Equal("var1", nameSymbol.Name);
+            Assert.Equal("displayName1", displayName.Value);
+            Assert.Equal("s", info.Type.ToString());
+
+            // The optionset we just added should be there
+            b = st1.TryGetVariable(new DName("os1"), out info, out displayName);
+
+            Assert.True(b);
+            OptionSet optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os1", optionSet.EntityName.Value);
+            Assert.Equal("os1", displayName.Value);
+            Assert.Equal("L{ln1:l}", info.Type.ToString());
+
+            // The second optionset we just added should be there
+            b = st1.TryGetVariable(new DName("os2"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os2", optionSet.EntityName.Value);
+            Assert.Equal("os2", displayName.Value);
+            Assert.Equal("L{xx2:l}", info.Type.ToString());
+
+            // Can't add the same variable twice
+            InvalidOperationException exception = Assert.Throws<InvalidOperationException>(() => st1.AddVariable("var1", FormulaType.String, displayName: "displayName1"));
+            Assert.Equal("var1 is already defined", exception.Message);
+
+            // Can't add the same optionset twice
+            NameCollisionException exception2 = Assert.Throws<NameCollisionException>(() => st1.AddOptionSet(os1));
+            Assert.Equal("Name os1 has a collision with another display or logical name", exception2.Message);
+
+            // New symbol table with SAME variable name but different type & display name
+            var st2 = new SymbolTable();
+            st2.AddVariable("var1", FormulaType.Boolean, displayName: "displayName2");
+            var os3 = new OptionSet("os2", DisplayNameProvider.New(new Dictionary<DName, DName>() { { new DName("zz2"), new DName("tt2") } }));
+            st2.AddOptionSet(os3);
+
+            // The var1 we just added in st2 should be there
+            b = st2.TryGetVariable(new DName("var1"), out info, out displayName);
+
+            Assert.True(b);
+            nameSymbol = Assert.IsType<NameSymbol>(info.Data);
+            Assert.Equal("var1", nameSymbol.Name);
+            Assert.Equal("displayName2", displayName.Value);
+            Assert.Equal("b", info.Type.ToString());
+
+            // The optionset we just added should be there
+            b = st2.TryGetVariable(new DName("os2"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os2", optionSet.EntityName.Value);
+            Assert.Equal("os2", displayName.Value);
+            Assert.Equal("L{zz2:l}", info.Type.ToString());
+
+            // Compose symbol tables in st1, st2 order
+            // even if there are variable name conflicts, Compose will just work fine
+            ReadOnlySymbolTable rost1 = SymbolTable.Compose(new[] { st1, st2 });
+
+            // Here we'll get var1 from st1 and first always win
+            b = rost1.TryGetVariable(new DName("var1"), out info, out displayName);
+
+            Assert.True(b);
+            nameSymbol = Assert.IsType<NameSymbol>(info.Data);
+            Assert.Equal("var1", nameSymbol.Name);
+            Assert.Equal("displayName1", displayName.Value);
+            Assert.Equal("s", info.Type.ToString());
+
+            // os1 is only defined once, so we'll get it as excepted
+            b = rost1.TryGetVariable(new DName("os1"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os1", optionSet.EntityName.Value);
+            Assert.Equal("os1", displayName.Value);
+            Assert.Equal("L{ln1:l}", info.Type.ToString());
+
+            // The optionset os2 is defined twice but we'll get the version from st1 (first one wins)
+            b = rost1.TryGetVariable(new DName("os2"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os2", optionSet.EntityName.Value);
+            Assert.Equal("os2", displayName.Value);
+            Assert.Equal("L{xx2:l}", info.Type.ToString());
+
+            // There should only be 2 'visible' optionset (os1 and os2)
+            Assert.Equal(2, rost1.OptionSets.Count());
+
+            // Check types of OptionSets and confirm os2 is from st1
+            Assert.Equal("os1:ln1_dn1, os2:xx2_yy2", string.Join(", ", rost1.OptionSets.Select(kvp => $"{kvp.Key}:{string.Join(",", kvp.Value.FormulaType._type.DisplayNameProvider.LogicalToDisplayPairs.Select(p => $"{p.Key}_{p.Value}"))}")));
+
+            // Now we compose in the other order: st2 first
+            ReadOnlySymbolTable rost2 = SymbolTable.Compose(new[] { st2, st1 });
+
+            // This time, we'll get var1 from st2 and we can see the difference on display name and type
+            b = rost2.TryGetVariable(new DName("var1"), out info, out displayName);
+
+            Assert.True(b);
+            nameSymbol = Assert.IsType<NameSymbol>(info.Data);
+            Assert.Equal("var1", nameSymbol.Name);
+            Assert.Equal("displayName2", displayName.Value);
+            Assert.Equal("b", info.Type.ToString());
+
+            // No change here as there is only one option set os1
+            b = rost2.TryGetVariable(new DName("os1"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os1", optionSet.EntityName.Value);
+            Assert.Equal("os1", displayName.Value);
+            Assert.Equal("L{ln1:l}", info.Type.ToString());
+
+            // The optionset os2 is defined twice but we'll get the version from st2 (first one wins)
+            b = rost2.TryGetVariable(new DName("os2"), out info, out displayName);
+
+            Assert.True(b);
+            optionSet = Assert.IsType<OptionSet>(info.Data);
+            Assert.Equal("os2", optionSet.EntityName.Value);
+            Assert.Equal("os2", displayName.Value);
+            Assert.Equal("L{zz2:l}", info.Type.ToString());
+
+            // There should only be 2 'visible' optionset (os1 and os2)
+            Assert.Equal(2, rost2.OptionSets.Count());
+
+            // Check types of OptionSets and confirm os2 is from st2
+            Assert.Equal("os2:zz2_tt2, os1:ln1_dn1", string.Join(", ", rost2.OptionSets.Select(kvp => $"{kvp.Key}:{string.Join(",", kvp.Value.FormulaType._type.DisplayNameProvider.LogicalToDisplayPairs.Select(p => $"{p.Key}_{p.Value}"))}")));
+        }
+
         [Fact]
         public void VoidIsNotAllowed()
         {