From 28050c074b90c4df83d047bd187e81315e2098b6 Mon Sep 17 00:00:00 2001 From: Mike Stall Date: Mon, 9 May 2022 10:19:01 -0700 Subject: [PATCH] Fix #378 (#379) * Fix #378 If(true, OptionSet.Option1) OptionSetValue should derive from ValidFormulaValue, not FormulaValue * Pr feedback Co-authored-by: Mike Stall --- .../Public/Values/OptionSetValue.cs | 4 +- .../EvalVisitor.cs | 9 +--- .../ExpressionTestCases/OptionSet.txt | 11 +++++ .../ExpressionTestHelpers/TestRunner.cs | 4 ++ .../DisplayNameOptionSetTests.cs | 20 +++++++++ .../FileExpressionEvaluationTests.cs | 2 +- .../ValueTests.cs | 44 +++++++++++++++---- 7 files changed, 75 insertions(+), 19 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Values/OptionSetValue.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Values/OptionSetValue.cs index 1247a8b1d0..8017478353 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Values/OptionSetValue.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/Values/OptionSetValue.cs @@ -10,8 +10,8 @@ namespace Microsoft.PowerFx.Types /// /// A value within an option set. /// - [DebuggerDisplay("{ToString})")] - public class OptionSetValue : FormulaValue + [DebuggerDisplay("{ToString()})")] + public class OptionSetValue : ValidFormulaValue { /// /// Logical name for this option set value. diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/EvalVisitor.cs b/src/libraries/Microsoft.PowerFx.Interpreter/EvalVisitor.cs index c3e1d505d8..4302c51dd9 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/EvalVisitor.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/EvalVisitor.cs @@ -45,14 +45,7 @@ internal async ValueTask> EvalArgAsync(FormulaValue arg, SymbolCont { if (arg is LambdaFormulaValue lambda) { - var val = await lambda.EvalAsync(this, context); - return val switch - { - T t => DValue.Of(t), - BlankValue b => DValue.Of(b), - ErrorValue e => DValue.Of(e), - _ => DValue.Of(CommonErrors.RuntimeTypeMismatch(irContext)) - }; + arg = await lambda.EvalAsync(this, context); } return arg switch diff --git a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/OptionSet.txt b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/OptionSet.txt index 47eab4328d..5cad8e14dc 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/OptionSet.txt +++ b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/OptionSet.txt @@ -1,5 +1,16 @@ #SETUP: OptionSetTestSetup +// Both logical and display names bind +>> OptionSet.option_1 +OptionSet.option_1 + +>> OptionSet.Option1 +OptionSet.option_1 + +// Can use in If +>> If(true, OptionSet.Option1) +OptionSet.option_1 + >> OptionSet.Option1 <> OptionSet.Option2 true diff --git a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestHelpers/TestRunner.cs b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestHelpers/TestRunner.cs index 14dda5ddfb..1edcded839 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestHelpers/TestRunner.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestHelpers/TestRunner.cs @@ -374,6 +374,10 @@ internal static void TestToString(FormulaValue result, StringBuilder sb) var dateTime = dt.Value; sb.Append($"DateTime({dateTime.Year},{dateTime.Month},{dateTime.Day},{dateTime.Hour},{dateTime.Minute},{dateTime.Second},{dateTime.Millisecond})"); } + else if (result is OptionSetValue opt) + { + sb.Append($"{opt.Type.OptionSetName}.{opt.Option}"); + } else if (result is ErrorValue) { sb.Append(result); diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests/DisplayNameOptionSetTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests/DisplayNameOptionSetTests.cs index e5fba79c54..7cc3a6807c 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests/DisplayNameOptionSetTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests/DisplayNameOptionSetTests.cs @@ -115,5 +115,25 @@ public void PowerFxConfigCollisionsThrow() Assert.True(config.TryGetSymbol(new DName("SomeDisplayName"), out _, out displayName)); Assert.Equal("SomeDisplayName", displayName.Value); } + + [Fact] + public void Sample() + { + var config = new PowerFxConfig(); + var displayNames = DisplayNameUtility.MakeUnique(new Dictionary + { + { "option_1", "Option1" }, + { "option_2", "Option2" } + }); + + var option = new OptionSet("OptionSet", displayNames); + + config.AddOptionSet(option); + + var engine = new RecalcEngine(config); + + var expression = "If(true, OptionSet.Option1)"; + var value = engine.Eval(expression); + } } } diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests/FileExpressionEvaluationTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests/FileExpressionEvaluationTests.cs index 626fdb1e8c..7ba34ec927 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests/FileExpressionEvaluationTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests/FileExpressionEvaluationTests.cs @@ -46,7 +46,7 @@ public void InterpreterTestCase(ExpressionTestCase testCase) Skip.If(true, prefix + msg); break; } - } + } // Since test discovery runs in a separate process, run a dedicated // parse pass as a single unit test to verify all the .txt will parse. diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests/ValueTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests/ValueTests.cs index 258854d0f1..549ee3dddc 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests/ValueTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests/ValueTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections; +using System.Collections.Generic; using System.Linq; using Microsoft.PowerFx.Types; using Xunit; @@ -53,11 +54,11 @@ public void Number(double val, string expectedStr) double? val2 = val; var formulaValue2 = FormulaValue.New((double?)val); // nullable overload Assert.Equal(expectedStr, formulaValue2.Dump()); - + var formulaValue3 = FormulaValue.New((double?)null); Assert.IsType(formulaValue3.Type); Assert.IsType(formulaValue3); - } + } [Theory] [InlineData("abc", "\"abc\"")] @@ -168,7 +169,7 @@ private class TestRow [Fact] public void Table() { - TableValue val = _cache.NewTable( + TableValue val = _cache.NewTable( new TestRow { a = 10, str = "alpha" }, new TestRow { a = 15, str = "beta" }); @@ -183,7 +184,7 @@ public void Table() Assert.Equal("Table({a:10,str:\"alpha\"},{a:15,str:\"beta\"})", resultStr); } - + [Fact] public void TableFromRecords() { @@ -193,7 +194,7 @@ public void TableFromRecords() var result1 = ((RecordValue)val.Index(2).Value).GetField("a").ToObject(); Assert.Equal(15.0, result1); - + dynamic d = val.ToObject(); Assert.Equal(10.0, d[0].a); @@ -211,7 +212,7 @@ public void TableFromMixedRecords() { var cache = new TypeMarshallerCache(); RecordValue r1 = _cache.NewRecord(new { a = 10, b = 20, c = 30 }); - RecordValue r2 = _cache.NewRecord(new { a = 11, c = 31 }); + RecordValue r2 = _cache.NewRecord(new { a = 11, c = 31 }); TableValue val = FormulaValue.NewTable(r1.Type, r1, r2); // Users first type @@ -221,7 +222,7 @@ public void TableFromMixedRecords() var result2 = ((RecordValue)val.Index(2).Value).GetField("b"); Assert.IsType(result2); - Assert.IsType(result2.Type); + Assert.IsType(result2.Type); } [Fact] @@ -259,7 +260,7 @@ public void TableFromPrimitive() Assert.Equal("[10,20]", resultStr); // Must use NewSingleColumnTable to create a single column table. - Assert.Throws(() => NewTableT(r1, r2)); + Assert.Throws(() => NewTableT(r1, r2)); } [Fact] @@ -309,6 +310,33 @@ public void Blanks() Assert.True(r.GetField("missing") is BlankValue); Assert.Equal(15.1, r.GetField("number").ToObject()); } + + [Fact] + public void DeriveFromValidFormulaValue() + { + // Only Blank and Error can derive from FormulaValue directly. + // All else should derive from ValidFormulaValue. + // See ValidFormulaValue for explanation. + var set = new HashSet + { + typeof(BlankValue), + typeof(ErrorValue), + typeof(ValidFormulaValue), + typeof(LambdaFormulaValue), // Special, can eval to any FormulaValue. + }; + + var asmInterpreter = typeof(RecalcEngine).Assembly; + var asmCore = typeof(Engine).Assembly; + var allTypes = asmInterpreter.GetTypes().Concat(asmCore.GetTypes()); + + foreach (var type in allTypes) + { + if (type.BaseType == typeof(FormulaValue)) + { + Assert.True(set.Contains(type), $"Type {type.FullName} should derive from {typeof(ValidFormulaValue).FullName}, not FormulaValue."); + } + } + } } public static class FormulaValueExtensions