From 90441fd95d3da62d652ad8dacbf1be6f3ba9a24f Mon Sep 17 00:00:00 2001 From: Luc Genetier <69138830+LucGenetier@users.noreply.github.com> Date: Mon, 28 Oct 2024 19:16:03 +0100 Subject: [PATCH] Fixes in ComposedReadOnlySymbolTable (#2720) - OptionSets should only return first OS by name (first element wins) - TryGetVariable not working as expected --- .../Config/ComposedReadOnlySymbolTable.cs | 34 ++++- .../SymbolTableTests.cs | 144 ++++++++++++++++++ 2 files changed, 171 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Config/ComposedReadOnlySymbolTable.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Config/ComposedReadOnlySymbolTable.cs index 70da8c476..886daedfe 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Config/ComposedReadOnlySymbolTable.cs +++ b/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 _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 osNames = new HashSet(); + foreach (ReadOnlySymbolTable st in _symbolTables) { foreach (KeyValuePair 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) diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs index 288db61b8..105d2b3dc 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs +++ b/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() { { new DName("ln1"), new DName("dn1") } })); + var os2 = new OptionSet("os2", DisplayNameProvider.New(new Dictionary() { { 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(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(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(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(() => 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(() => 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() { { 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(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(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(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(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(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(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(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(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() {