Fix #378 (#379)

* Fix #378 If(true, OptionSet.Option1) OptionSetValue should derive from ValidFormulaValue, not FormulaValue * Pr feedback Co-authored-by: Mike Stall <[email protected]>
microsoft · May 9, 2022 · 28050c0 · 28050c0
1 parent 4860013
commit 28050c0
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 19 deletions.
diff --git 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
     /// <summary>
     /// A value within an option set. 
     /// </summary>
-    [DebuggerDisplay("{ToString})")]
-    public class OptionSetValue : FormulaValue
+    [DebuggerDisplay("{ToString()})")]
+    public class OptionSetValue : ValidFormulaValue
     {
         /// <summary>
         /// 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
@@ -45,14 +45,7 @@ internal async ValueTask<DValue<T>> EvalArgAsync<T>(FormulaValue arg, SymbolCont
         {
             if (arg is LambdaFormulaValue lambda)
             {
-                var val = await lambda.EvalAsync(this, context);
-                return val switch
-                {
-                    T t => DValue<T>.Of(t),
-                    BlankValue b => DValue<T>.Of(b),
-                    ErrorValue e => DValue<T>.Of(e),
-                    _ => DValue<T>.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
@@ -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
@@ -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
@@ -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<string, string>
+            {
+                { "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
@@ -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
@@ -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<NumberType>(formulaValue3.Type);
             Assert.IsType<BlankValue>(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<BlankValue>(result2);
-            Assert.IsType<NumberType>(result2.Type);        
+            Assert.IsType<NumberType>(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<InvalidOperationException>(() => NewTableT(r1, r2));            
+            Assert.Throws<InvalidOperationException>(() => 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<Type>
+            {
+                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