From c5a3aeadb5701acf023c8a01ef687186272555fd Mon Sep 17 00:00:00 2001 From: Jas Valgotar <32079188+jas-valgotar@users.noreply.github.com> Date: Tue, 5 Nov 2024 18:02:14 -0500 Subject: [PATCH] Fixes enum store when SymbolTable is replaced while setting up config (#2729) This PR fixes the issue where inbuilt enums are no longer part of the config if the config's symbol table is replaced after initialization. After initializing the config if SymbolTable was replaced, we were loosing the enums attached to the previous default symbol table. This PR will adds a default symbol table(seprate from existing table) to handle that. --- .../Public/Config/PowerFxConfig.cs | 60 +++++++++++++------ .../Public/Config/ReadOnlySymbolTable.cs | 3 +- .../Microsoft.PowerFx.Core/Public/Engine.cs | 4 +- .../Environment/PowerFxConfigExtensions.cs | 4 +- .../RecalcEngine.cs | 2 +- .../SymbolTableTests.cs | 4 +- .../RecalcEngineTests.cs | 22 ++++++- 7 files changed, 72 insertions(+), 27 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Config/PowerFxConfig.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Config/PowerFxConfig.cs index eacf0b3f4..d873582a5 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Config/PowerFxConfig.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/Config/PowerFxConfig.cs @@ -23,20 +23,41 @@ namespace Microsoft.PowerFx internal static readonly int DefaultMaxCallDepth = 20; internal static readonly int DefaultMaximumExpressionLength = 1000; - /// - /// Global symbols. Additional symbols beyond default function set and primitive types. - /// - public SymbolTable SymbolTable { get; set; } = new SymbolTable + private SymbolTable _symbolTable = new SymbolTable { - DebugName = "DefaultConfig" + DebugName = "Host Symbols" }; + private readonly SymbolTable _internalConfigSymbols = new SymbolTable + { + DebugName = "InternalConfigSymbols" + }; + + internal SymbolTable InternalConfigSymbols => _internalConfigSymbols; + + /// + /// We shouldn't cache it as SymbolTable is mutable. + /// + internal ReadOnlySymbolTable ComposedConfigSymbols => ReadOnlySymbolTable.Compose(InternalConfigSymbols, SymbolTable); + + private static EnumStoreBuilder BuiltInEnumStoreBuilder => new EnumStoreBuilder().WithRequiredEnums(BuiltinFunctionsCore._library); + + /// + /// Global symbols. Additional symbols beyond default function set and primitive types defined by host. + /// + /// + public SymbolTable SymbolTable + { + get => _symbolTable; + set => _symbolTable = value; + } + internal readonly Dictionary AdditionalFunctions = new (); [Obsolete("Use Config.EnumStore or symboltable directly")] - internal EnumStoreBuilder EnumStoreBuilder => SymbolTable.EnumStoreBuilder; + internal EnumStoreBuilder EnumStoreBuilder => InternalConfigSymbols.EnumStoreBuilder; - internal IEnumStore EnumStore => ReadOnlySymbolTable.Compose(SymbolTable); + internal IEnumStore EnumStore => ComposedConfigSymbols; public Features Features { get; } @@ -47,7 +68,7 @@ namespace Microsoft.PowerFx private PowerFxConfig(EnumStoreBuilder enumStoreBuilder, Features features = null) { Features = features ?? Features.None; - SymbolTable.EnumStoreBuilder = enumStoreBuilder; + InternalConfigSymbols.EnumStoreBuilder = enumStoreBuilder; MaxCallDepth = DefaultMaxCallDepth; MaximumExpressionLength = DefaultMaximumExpressionLength; } @@ -66,7 +87,7 @@ namespace Microsoft.PowerFx [Obsolete("Migrate to SymbolTables")] public IEnumerable FunctionInfos => new Engine(this).SupportedFunctions.Functions.Functions - .Concat(SymbolTable.Functions.Functions) + .Concat(ComposedConfigSymbols.Functions.Functions) .Select(f => new FunctionInfo(f)); /// @@ -74,7 +95,7 @@ namespace Microsoft.PowerFx /// /// Features to use. public PowerFxConfig(Features features) - : this(new EnumStoreBuilder().WithRequiredEnums(BuiltinFunctionsCore._library), features) + : this(BuiltInEnumStoreBuilder, features) { } @@ -115,12 +136,12 @@ namespace Microsoft.PowerFx } } - internal bool GetSymbols(string name, out NameLookupInfo symbol) => SymbolTable._variables.TryGetValue(name, out symbol); + internal bool GetSymbols(string name, out NameLookupInfo symbol) => InternalConfigSymbols._variables.TryGetValue(name, out symbol) || SymbolTable._variables.TryGetValue(name, out symbol); - internal IEnumerable GetSuggestableSymbolName() => SymbolTable._variables.Keys; + internal IEnumerable GetSuggestableSymbolName() => InternalConfigSymbols._variables.Keys.Concat(SymbolTable._variables.Keys); internal void AddEntity(IExternalEntity entity, DName displayName = default) - => SymbolTable.AddEntity(entity, displayName); + => SymbolTable.AddEntity(entity, displayName); internal void AddFunction(TexlFunction function) { @@ -137,7 +158,7 @@ namespace Microsoft.PowerFx } } - var overloads = SymbolTable.Functions.WithName(function.Name).Where(tf => tf.HasLambdas || tf.HasColumnIdentifiers); + var overloads = ComposedConfigSymbols.Functions.WithName(function.Name).Where(tf => tf.HasLambdas || tf.HasColumnIdentifiers); if (overloads.Any()) { @@ -153,20 +174,25 @@ namespace Microsoft.PowerFx } } - SymbolTable.AddFunction(function); + InternalConfigSymbols.AddFunction(function); } internal void AddFunctions(TexlFunctionSet functionSet) { - SymbolTable.AddFunctions(functionSet); + InternalConfigSymbols.AddFunctions(functionSet); } + /// + /// Adds optionset to . + /// + /// + /// public void AddOptionSet(OptionSet optionSet, DName optionalDisplayName = default) { SymbolTable.AddOptionSet(optionSet, optionalDisplayName); } internal bool TryGetVariable(DName name, out DName displayName) - => SymbolTable.TryGetVariable(name, out _, out displayName); + => ComposedConfigSymbols.TryGetVariable(name, out _, out displayName); } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Config/ReadOnlySymbolTable.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Config/ReadOnlySymbolTable.cs index b9bba138e..3ec510d9e 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Config/ReadOnlySymbolTable.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/Config/ReadOnlySymbolTable.cs @@ -353,7 +353,8 @@ namespace Microsoft.PowerFx // Which enums are available. // These do not compose - only bottom one wins. // ComposedReadOnlySymbolTable will handle composition by looking up in each symbol table. - private protected EnumStoreBuilder _enumStoreBuilder; + private protected EnumStoreBuilder _enumStoreBuilder; + private EnumSymbol[] _enumSymbolCache; private EnumSymbol[] GetEnumSymbolSnapshot diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Engine.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Engine.cs index 06152a842..575e98dca 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Engine.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/Engine.cs @@ -175,7 +175,7 @@ namespace Microsoft.PowerFx // Compose all the symbol tables most likely to have functions into a single // symbol table and then cache that. // That will cache unifying into a single TexlFunctionSet - which is the most expensive part. - var functionList = _functionListCache.GetComposedCached(SupportedFunctions, Config.SymbolTable); + var functionList = _functionListCache.GetComposedCached(SupportedFunctions, Config.ComposedConfigSymbols); var symbols = ReadOnlySymbolTable.Compose(EngineSymbols, functionList, PrimitiveTypes); @@ -548,7 +548,7 @@ namespace Microsoft.PowerFx public DefinitionsCheckResult AddUserDefinedFunction(string script, CultureInfo parseCulture = null, ReadOnlySymbolTable symbolTable = null, bool allowSideEffects = false) { - var engineTypesAndFunctions = ReadOnlySymbolTable.Compose(PrimitiveTypes, SupportedFunctions); + var engineTypesAndFunctions = ReadOnlySymbolTable.Compose(PrimitiveTypes, SupportedFunctions, Config.InternalConfigSymbols); return Config.SymbolTable.AddUserDefinedFunction(script, parseCulture, engineTypesAndFunctions, symbolTable, allowSideEffects); } } diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/Environment/PowerFxConfigExtensions.cs b/src/libraries/Microsoft.PowerFx.Interpreter/Environment/PowerFxConfigExtensions.cs index 69db48a8a..db1615185 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/Environment/PowerFxConfigExtensions.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/Environment/PowerFxConfigExtensions.cs @@ -70,12 +70,12 @@ namespace Microsoft.PowerFx foreach (KeyValuePair func in Library.RegexFunctions(regExTimeout, regexTypeCache)) { - if (config.SymbolTable.Functions.AnyWithName(func.Key.Name)) + if (config.ComposedConfigSymbols.Functions.AnyWithName(func.Key.Name)) { throw new InvalidOperationException("Cannot add RegEx functions more than once."); } - config.SymbolTable.AddFunction(func.Key); + config.InternalConfigSymbols.AddFunction(func.Key); config.AdditionalFunctions.Add(func.Key, func.Value); } } diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs index 0c1f4d2df..f85f9ad6e 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs @@ -420,7 +420,7 @@ namespace Microsoft.PowerFx } // Compose will handle null symbols - var composedSymbols = SymbolTable.Compose(Config.SymbolTable, SupportedFunctions, PrimitiveTypes, _symbolTable); + var composedSymbols = SymbolTable.Compose(Config.ComposedConfigSymbols, SupportedFunctions, PrimitiveTypes, _symbolTable); if (parseResult.DefinedTypes.Any()) { diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs index 105d2b3dc..c0fa0201f 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/SymbolTableTests.cs @@ -487,12 +487,12 @@ namespace Microsoft.PowerFx.Core.Tests PowerFxConfig config = new PowerFxConfig(); - bool fOk = config.SymbolTable.TryGetSymbolType("os1", out var type); + bool fOk = config.ComposedConfigSymbols.TryGetSymbolType("os1", out var type); Assert.False(fOk); config.AddOptionSet(os); - fOk = config.SymbolTable.TryGetSymbolType("os1", out type); + fOk = config.ComposedConfigSymbols.TryGetSymbolType("os1", out type); Assert.True(fOk); AssertOptionSetType(type, os); diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index b71764da4..0947791cb 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -397,8 +397,8 @@ namespace Microsoft.PowerFx.Tests // After (A,B) = 3,6 var result = engine.Eval("With({x:A, y:B}, Set(A,3); x & y & A & B)", options: _opts); Assert.Equal("102036", result.ToObject()); - } - + } + [Fact] public void BasicEval() { @@ -409,6 +409,24 @@ namespace Microsoft.PowerFx.Tests Assert.Equal(14.0, ((NumberValue)result).Value); } + [Fact] + public void BuiltInEnumConfigCheck() + { + var config = new PowerFxConfig() + { + SymbolTable = null + }; + +#pragma warning disable CS0618 // Type or member is obsolete + config.EnableRegExFunctions(); +#pragma warning restore CS0618 // Type or member is obsolete + var expression = "Match(\"test\", \"t\", MatchOptions.Contains)"; + + var engine = new RecalcEngine(config); + var check = engine.Check(expression); + Assert.True(check.IsSuccess); + } + [Fact] public void FormulaErrorUndefined() {