From 3046a169c44d672f5ca33a096b07782dc2dea1b8 Mon Sep 17 00:00:00 2001 From: Adithya Selvaprithiviraj Date: Fri, 22 Nov 2024 12:54:31 -0800 Subject: [PATCH 1/5] Restrictive aggregate types in UDF --- .../Functions/TexlFunction.cs | 2 +- .../Functions/UserDefinedFunction.cs | 33 +- .../Microsoft.PowerFx.Core/Types/DType.cs | 53 +- .../RecalcEngineTests.cs | 498 +++++++++--------- 4 files changed, 322 insertions(+), 264 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs index 5271868816..53e022d83f 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs @@ -548,7 +548,7 @@ private bool CheckTypesCore(CheckTypesContext context, TexlNode[] args, DType[] } Contracts.AssertValid(argTypes[i]); - var expectedParamType = ParamTypes[i]; + var expectedParamType = ParamTypes[i]; // If the strong-enum type flag is disabled, treat an enum option set type as the enum supertype instead if (!context.Features.StronglyTypedBuiltinEnums && expectedParamType.OptionSetInfo is EnumSymbol enumSymbol) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs index 31d436703c..69981a88b8 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs @@ -8,6 +8,7 @@ using Microsoft.CodeAnalysis; using Microsoft.PowerFx.Core.App; using Microsoft.PowerFx.Core.App.Controls; +using Microsoft.PowerFx.Core.App.ErrorContainers; using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Binding.BindInfo; using Microsoft.PowerFx.Core.Entities; @@ -77,6 +78,27 @@ public override bool TryGetDataSource(CallNode callNode, TexlBinding binding, ou public bool HasDelegationWarning => _binding?.ErrorContainer.GetErrors().Any(error => error.MessageKey.Contains("SuggestRemoteExecutionHint")) ?? false; + public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DType[] argTypes, IErrorContainer errors, out DType returnType, out Dictionary nodeToCoercedTypeMap) + { + if (!base.CheckTypes(context, args, argTypes, errors, out returnType, out nodeToCoercedTypeMap)) + { + return false; + } + + for (int i = 0; i < argTypes.Length; i++) + { + if ((argTypes[i].IsTableNonObjNull || argTypes[i].IsRecordNonObjNull) && + !ParamTypes[i].Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules, true) && + !argTypes[i].CoercesTo(ParamTypes[i], true, false, context.Features, true)) + { + errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrBadSchema_ExpectedType, argTypes[i].GetKindString(), ParamTypes[i].GetKindString()); + return false; + } + } + + return true; + } + /// /// Initializes a new instance of the class. /// @@ -167,15 +189,22 @@ public void CheckTypesOnDeclaration(CheckTypesContext context, DType actualBodyR Contracts.AssertValue(actualBodyReturnType); Contracts.AssertValue(binding); - if (!ReturnType.Accepts(actualBodyReturnType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) + if (!ReturnType.Accepts(actualBodyReturnType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules, true)) { - if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features)) + if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features, true)) { _binding.SetCoercedType(binding.Top, ReturnType); } else { var node = UdfBody is VariadicOpNode variadicOpNode ? variadicOpNode.Children.Last() : UdfBody; + + if ((ReturnType.IsTable && actualBodyReturnType.IsTable) || (ReturnType.IsRecord && actualBodyReturnType.IsRecord)) + { + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Severe, node, TexlStrings.ErrBadSchema_ExpectedType, ReturnType.GetKindString(), actualBodyReturnType.GetKindString()); + return; + } + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Severe, node, TexlStrings.ErrUDF_ReturnTypeDoesNotMatch, ReturnType.GetKindString(), actualBodyReturnType.GetKindString()); } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs index b70fec26c5..ec31a52b8b 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs @@ -1846,12 +1846,13 @@ private bool AcceptsEntityType(DType type, bool usePowerFxV1CompatibilityRules) /// Legacy rules for accepting date/time types. /// Use PFx v1 compatibility rules if enabled (less /// permissive Accepts relationships). + /// restrictiveAggregateTypes. /// /// True if accepts , false otherwise. /// - public bool Accepts(DType type, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules) + public bool Accepts(DType type, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules, bool restrictiveAggregateTypes = false) { - return Accepts(type, out _, out _, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules); + return Accepts(type, out _, out _, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules, restrictiveAggregateTypes); } /// @@ -1883,7 +1884,7 @@ public bool Accepts(DType type, bool exact, bool useLegacyDateTimeAccepts, bool /// /// True if accepts , false otherwise. /// - public virtual bool Accepts(DType type, out KeyValuePair schemaDifference, out DType schemaDifferenceType, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules) + public virtual bool Accepts(DType type, out KeyValuePair schemaDifference, out DType schemaDifferenceType, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules, bool restrictiveAggregateTypes = false) { AssertValid(); type.AssertValid(); @@ -1936,7 +1937,7 @@ bool DefaultReturnValue(DType targetType) => if (Kind == type.Kind) { - return TreeAccepts(this, TypeTree, type.TypeTree, out schemaDifference, out schemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules); + return TreeAccepts(this, TypeTree, type.TypeTree, out schemaDifference, out schemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules, restrictiveAggregateTypes); } accepts = type.Kind == DKind.Unknown || type.Kind == DKind.Deferred; @@ -1950,7 +1951,7 @@ bool DefaultReturnValue(DType targetType) => if (Kind == type.Kind || type.IsExpandEntity) { - return TreeAccepts(this, TypeTree, type.TypeTree, out schemaDifference, out schemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules); + return TreeAccepts(this, TypeTree, type.TypeTree, out schemaDifference, out schemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules, restrictiveAggregateTypes); } accepts = (IsMultiSelectOptionSet() && TypeTree.GetPairs().First().Value.OptionSetInfo == type.OptionSetInfo) || type.Kind == DKind.Unknown || type.Kind == DKind.Deferred; @@ -2170,7 +2171,7 @@ bool DefaultReturnValue(DType targetType) => } // Implements Accepts for Record and Table types. - private static bool TreeAccepts(DType parentType, TypeTree treeDst, TypeTree treeSrc, out KeyValuePair schemaDifference, out DType treeSrcSchemaDifferenceType, bool exact = true, bool useLegacyDateTimeAccepts = false, bool usePowerFxV1CompatibilityRules = false) + private static bool TreeAccepts(DType parentType, TypeTree treeDst, TypeTree treeSrc, out KeyValuePair schemaDifference, out DType treeSrcSchemaDifferenceType, bool exact = true, bool useLegacyDateTimeAccepts = false, bool usePowerFxV1CompatibilityRules = false, bool restrictiveAggregateTypes = false) { treeDst.AssertValid(); treeSrc.AssertValid(); @@ -2210,7 +2211,7 @@ private static bool TreeAccepts(DType parentType, TypeTree treeDst, TypeTree tre return false; } - if (!pairDst.Value.Accepts(type, out var recursiveSchemaDifference, out var recursiveSchemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules)) + if (!pairDst.Value.Accepts(type, out var recursiveSchemaDifference, out var recursiveSchemaDifferenceType, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules, restrictiveAggregateTypes)) { if (!TryGetDisplayNameForColumn(parentType, pairDst.Key, out var colName)) { @@ -2232,6 +2233,17 @@ private static bool TreeAccepts(DType parentType, TypeTree treeDst, TypeTree tre } } + if (restrictiveAggregateTypes) + { + foreach (var pairSrc in treeSrc) + { + if (!treeDst.Contains(pairSrc.Key)) + { + return false; + } + } + } + return true; } @@ -3136,17 +3148,17 @@ public bool ContainsControlType(DPath path) (n.Type.IsAggregate && n.Type.ContainsControlType(DPath.Root))); } - public bool CoercesTo(DType typeDest, bool aggregateCoercion, bool isTopLevelCoercion, Features features) + public bool CoercesTo(DType typeDest, bool aggregateCoercion, bool isTopLevelCoercion, Features features, bool restrictiveAggregateTypes = false) { - return CoercesTo(typeDest, out _, aggregateCoercion, isTopLevelCoercion, features); + return CoercesTo(typeDest, out _, aggregateCoercion, isTopLevelCoercion, features, restrictiveAggregateTypes); } - public bool CoercesTo(DType typeDest, out bool isSafe, bool aggregateCoercion, bool isTopLevelCoercion, Features features) + public bool CoercesTo(DType typeDest, out bool isSafe, bool aggregateCoercion, bool isTopLevelCoercion, Features features, bool restrictiveAggregateTypes = false) { - return CoercesTo(typeDest, out isSafe, out _, out _, out _, aggregateCoercion, isTopLevelCoercion, features); + return CoercesTo(typeDest, out isSafe, out _, out _, out _, aggregateCoercion, isTopLevelCoercion, features, restrictiveAggregateTypes); } - public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coercionType, out KeyValuePair schemaDifference, out DType schemaDifferenceType, Features features, bool aggregateCoercion = true) + public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coercionType, out KeyValuePair schemaDifference, out DType schemaDifferenceType, Features features, bool aggregateCoercion = true, bool restrictiveAggregateTypes = false) { Contracts.Assert(IsAggregate); @@ -3254,6 +3266,17 @@ public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coerci isSafe &= fieldIsSafe; } + if (restrictiveAggregateTypes) + { + foreach (var typedName in GetNames(DPath.Root)) + { + if (!typeDest.TryGetType(typedName.Name, out _)) + { + return false; + } + } + } + return isValid; } @@ -3268,7 +3291,8 @@ public virtual bool CoercesTo( out DType schemaDifferenceType, bool aggregateCoercion, bool isTopLevelCoercion, - Features features) + Features features, + bool restrictiveAggregateTypes = false) { AssertValid(); Contracts.Assert(typeDest.IsValid); @@ -3330,7 +3354,8 @@ public virtual bool CoercesTo( out schemaDifference, out schemaDifferenceType, features, - aggregateCoercion); + aggregateCoercion, + restrictiveAggregateTypes); } var subtypeCoerces = SubtypeCoercesTo( diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index 2f117e3273..05ef9e8597 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -10,7 +10,7 @@ using System.Threading.Tasks; using Microsoft.PowerFx.Core; using Microsoft.PowerFx.Core.Functions; -using Microsoft.PowerFx.Core.Functions.Delegation; +using Microsoft.PowerFx.Core.Functions.Delegation; using Microsoft.PowerFx.Core.Functions.Delegation.DelegationMetadata; using Microsoft.PowerFx.Core.IR; using Microsoft.PowerFx.Core.Localization; @@ -398,8 +398,8 @@ public void UserDefinitionOnUpdateTest() // 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() { @@ -412,20 +412,20 @@ public void BasicEval() [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); + { + 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] @@ -486,7 +486,7 @@ public void FormulaCantRedefine() false, 14.0)] - // Recursive calls are not allowed + // Recursive calls are not allowed [InlineData( "foo(x:Number):Number = If(x=0,foo(1),If(x=1,foo(2),If(x=2,Float(2))));", "foo(Float(0))", @@ -614,78 +614,78 @@ public void FunctionPrecedenceTest(string script, double expected) var result = check.GetEvaluator().Eval(); Assert.Equal(expected, result.AsDouble()); - } - + } + [Fact] public void ShadowingFunctionPrecedenceTest() - { + { var engine = new RecalcEngine(); - engine.AddUserDefinedFunction("Concat(x:Text):Text = \"xyz\"; Average(x:Number):Number = 11111;"); + engine.AddUserDefinedFunction("Concat(x:Text):Text = \"xyz\"; Average(x:Number):Number = 11111;"); var check = engine.Check("Concat(\"abc\")"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.String, check.ReturnType); - var result = check.GetEvaluator().Eval(); - Assert.Equal("xyz", ((StringValue)result).Value); - + var result = check.GetEvaluator().Eval(); + Assert.Equal("xyz", ((StringValue)result).Value); + check = engine.Check("Average(123)"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Number, check.ReturnType); - result = check.GetEvaluator().Eval(); - Assert.Equal(11111, result.AsDouble()); - - engine.AddUserDefinitions("Test := Type({A: Number}); TestTable := Type([{A: Number}]);" + - "Filter(X: TestTable):Test = First(X); ShowColumns(X: TestTable):TestTable = FirstN(X, 3);"); - + result = check.GetEvaluator().Eval(); + Assert.Equal(11111, result.AsDouble()); + + engine.AddUserDefinitions("Test := Type({A: Number}); TestTable := Type([{A: Number}]);" + + "Filter(X: TestTable):Test = First(X); ShowColumns(X: TestTable):TestTable = FirstN(X, 3);"); + check = engine.Check("Filter([{A: 123}]).A"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Number, check.ReturnType); - result = check.GetEvaluator().Eval(); - Assert.Equal(123, result.AsDouble()); - + result = check.GetEvaluator().Eval(); + Assert.Equal(123, result.AsDouble()); + check = engine.Check("CountRows(ShowColumns([{A: 123}, {A: 124}, {A:125}, {A:126}, {A: 127}]))"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Decimal, check.ReturnType); - result = check.GetEvaluator().Eval(); + result = check.GetEvaluator().Eval(); Assert.Equal(3, result.AsDouble()); } - - [Theory] - - // Behavior function in non-imperative udf + + [Theory] + + // Behavior function in non-imperative udf [InlineData( "TestFunc():Void = Set(a, 123);", true, - "Behavior function in a non-behavior user-defined function", - false)] - - // Behavior function in imperative udf + "Behavior function in a non-behavior user-defined function", + false)] + + // Behavior function in imperative udf [InlineData( "TestFunc():Void = { Set(a, 123); };", false, - null, - true)] + null, + true)] public void BehaviorFunctionInImperativeUDF(string udfExpression, bool expectedError, string expectedErrorKey, bool allowSideEffects) - { - var config = new PowerFxConfig(); - config.EnableSetFunction(); - var engine = new RecalcEngine(config); - engine.UpdateVariable("a", 1m); - - var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); - Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); - - if (expectedError) - { - result.Errors.Any(error => error.MessageKey == expectedErrorKey); + { + var config = new PowerFxConfig(); + config.EnableSetFunction(); + var engine = new RecalcEngine(config); + engine.UpdateVariable("a", 1m); + + var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); + Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); + + if (expectedError) + { + result.Errors.Any(error => error.MessageKey == expectedErrorKey); } - } - + } + [Theory] // Return value with side effectful UDF @@ -710,12 +710,12 @@ public void ImperativeUserDefinedFunctionTest(string udfExpression, string expre config.EnableSetFunction(); var recalcEngine = new RecalcEngine(config); recalcEngine.UpdateVariable("a", 1m); - - var definitionsCheckResult = recalcEngine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); - if (!expectedError) - { - Assert.True(definitionsCheckResult.IsSuccess); + var definitionsCheckResult = recalcEngine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); + + if (!expectedError) + { + Assert.True(definitionsCheckResult.IsSuccess); var result = recalcEngine.Eval(expression, options: _opts); var fvExpected = FormulaValue.New(expected); @@ -724,136 +724,136 @@ public void ImperativeUserDefinedFunctionTest(string udfExpression, string expre } else { - Assert.False(definitionsCheckResult.IsSuccess); + Assert.False(definitionsCheckResult.IsSuccess); Assert.Single(definitionsCheckResult.Errors); Assert.Contains(definitionsCheckResult.Errors, err => err.MessageKey == errorKey); } - } - - [Theory] - - [InlineData( + } + + [Theory] + + [InlineData( "MismatchType():Number = { 1+3; Color.Blue; };", true, - true, - 36, - 41)] - [InlineData( + true, + 36, + 41)] + [InlineData( "MatchType():Text = { 4; 3 };", false, - true, - 0, + true, + 0, 0)] public void TestMismatchReturnType(string udfExpression, bool expectedError, bool allowSideEffects, int min, int lim) { var config = new PowerFxConfig(); config.EnableSetFunction(); var engine = new RecalcEngine(config); - engine.UpdateVariable("x", 1m); - - var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); - Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); - - if (expectedError) - { - var error = result.Errors.First(error => error.MessageKey == "ErrUDF_ReturnTypeDoesNotMatch"); - Assert.NotNull(error); - var span = error.Span; - Assert.True(span.Min == min && span.Lim == lim); + engine.UpdateVariable("x", 1m); + + var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); + Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); + + if (expectedError) + { + var error = result.Errors.First(error => error.MessageKey == "ErrUDF_ReturnTypeDoesNotMatch"); + Assert.NotNull(error); + var span = error.Span; + Assert.True(span.Min == min && span.Lim == lim); } - } - + } + [Fact] public void DelegableUDFTest() - { - var symbolTable = new DelegatableSymbolTable(); - var schema = DType.CreateTable( - new TypedName(DType.Guid, new DName("ID")), + { + var symbolTable = new DelegatableSymbolTable(); + var schema = DType.CreateTable( + new TypedName(DType.Guid, new DName("ID")), new TypedName(DType.Number, new DName("Value"))); - symbolTable.AddEntity(new TestDelegableDataSource( - "MyDataSource", - schema, - new TestDelegationMetadata( - new DelegationCapability(DelegationCapability.Filter), - schema, - new FilterOpMetadata( - schema, - new Dictionary(), - new Dictionary(), - new DelegationCapability(DelegationCapability.GreaterThan), - null)), - true)); + symbolTable.AddEntity(new TestDelegableDataSource( + "MyDataSource", + schema, + new TestDelegationMetadata( + new DelegationCapability(DelegationCapability.Filter), + schema, + new FilterOpMetadata( + schema, + new Dictionary(), + new Dictionary(), + new DelegationCapability(DelegationCapability.GreaterThan), + null)), + true)); symbolTable.AddType(new DName("MyDataSourceTableType"), FormulaType.Build(schema)); - var config = new PowerFxConfig() - { + var config = new PowerFxConfig() + { SymbolTable = symbolTable }; - config.EnableSetFunction(); - - var recalcEngine = new RecalcEngine(config); - - recalcEngine.AddUserDefinedFunction("A():MyDataSourceTableType = Filter(MyDataSource, Value > 10);C():MyDataSourceTableType = A(); B():MyDataSourceTableType = Filter(C(), Value > 11); D():MyDataSourceTableType = { Filter(B(), Value > 12); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); - var func = recalcEngine.Functions.WithName("A").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("C").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("B").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("D").First() as UserDefinedFunction; - - // Imperative function is not delegable - Assert.True(func.IsAsync); - Assert.True(!func.IsDelegatable); - - // Binding fails for recursive definitions and hence function is not added. - Assert.False(recalcEngine.AddUserDefinedFunction("E():Void = { E(); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true).IsSuccess); - } - - [Fact] + config.EnableSetFunction(); + + var recalcEngine = new RecalcEngine(config); + + recalcEngine.AddUserDefinedFunction("A():MyDataSourceTableType = Filter(MyDataSource, Value > 10);C():MyDataSourceTableType = A(); B():MyDataSourceTableType = Filter(C(), Value > 11); D():MyDataSourceTableType = { Filter(B(), Value > 12); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); + var func = recalcEngine.Functions.WithName("A").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("C").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("B").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("D").First() as UserDefinedFunction; + + // Imperative function is not delegable + Assert.True(func.IsAsync); + Assert.True(!func.IsDelegatable); + + // Binding fails for recursive definitions and hence function is not added. + Assert.False(recalcEngine.AddUserDefinedFunction("E():Void = { E(); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true).IsSuccess); + } + + [Fact] public void TestInheritanceOfDelegationWarningsInUDFs() - { - var symbolTable = new DelegatableSymbolTable(); - var schema = DType.CreateTable( + { + var symbolTable = new DelegatableSymbolTable(); + var schema = DType.CreateTable( new TypedName(DType.Number, new DName("Value"))); - symbolTable.AddEntity(new TestDelegableDataSource( - "MyDataSource", - schema, - new TestDelegationMetadata( - new DelegationCapability(DelegationCapability.Filter), - schema, - new FilterOpMetadata( - schema, - new Dictionary(), - new Dictionary(), - new DelegationCapability(DelegationCapability.GreaterThan), - null)), - true)); + symbolTable.AddEntity(new TestDelegableDataSource( + "MyDataSource", + schema, + new TestDelegationMetadata( + new DelegationCapability(DelegationCapability.Filter), + schema, + new FilterOpMetadata( + schema, + new Dictionary(), + new Dictionary(), + new DelegationCapability(DelegationCapability.GreaterThan), + null)), + true)); symbolTable.AddType(new DName("MyDataSourceTableType"), FormulaType.Build(schema)); - var config = new PowerFxConfig() - { + var config = new PowerFxConfig() + { SymbolTable = symbolTable }; - var engine = new RecalcEngine(config); + var engine = new RecalcEngine(config); + + var result = engine.AddUserDefinedFunction("NonDelegatableUDF():MyDataSourceTableType = Filter(MyDataSource, Sqrt(Value) > 5);", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); + Assert.True(result.IsSuccess); + var func = engine.Functions.WithName("NonDelegatableUDF").First() as UserDefinedFunction; + Assert.True(func.HasDelegationWarning); - var result = engine.AddUserDefinedFunction("NonDelegatableUDF():MyDataSourceTableType = Filter(MyDataSource, Sqrt(Value) > 5);", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); - Assert.True(result.IsSuccess); - var func = engine.Functions.WithName("NonDelegatableUDF").First() as UserDefinedFunction; - Assert.True(func.HasDelegationWarning); - - result = engine.AddUserDefinedFunction("NonDelegatableUDF2():MyDataSourceTableType = NonDelegatableUDF();", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); - Assert.True(result.IsSuccess); - func = engine.Functions.WithName("NonDelegatableUDF2").First() as UserDefinedFunction; - Assert.True(func.HasDelegationWarning); + result = engine.AddUserDefinedFunction("NonDelegatableUDF2():MyDataSourceTableType = NonDelegatableUDF();", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); + Assert.True(result.IsSuccess); + func = engine.Functions.WithName("NonDelegatableUDF2").First() as UserDefinedFunction; + Assert.True(func.HasDelegationWarning); } // Binding to inner functions does not impact outer functions. @@ -1768,68 +1768,68 @@ public void LookupBuiltinOptionSets() Assert.True(ok); // Wrong type: https://github.com/microsoft/Power-Fx/issues/2342 - } - - [Fact] - public void LazyJsonTest() - { - bool LazyGetField1(string name, out FormulaType type) - { - type = name switch - { - "field1" => FormulaType.Decimal, - "field2" => FormulaType.String, - _ => FormulaType.Blank, - }; - - return type != FormulaType.Blank; - } - - PowerFxConfig config = new PowerFxConfig(); - config.EnableJsonFunctions(); - - RecalcEngine engine = new RecalcEngine(config); - SymbolTable symbolTable = new SymbolTable(); - TestLazyRecordType lazyRecordType = new TestLazyRecordType("test", new List() { "field1", "field2" }, LazyGetField1); - - ISymbolSlot slot = symbolTable.AddVariable("var1", lazyRecordType); - - CheckResult checkResult = engine.Check("JSON(var1)", symbolTable: symbolTable); - Assert.True(checkResult.IsSuccess, string.Join(", ", checkResult.Errors.Select(err => err.Message))); - - SymbolValues symbolValues = new SymbolValues(symbolTable); - symbolValues.Set(slot, new LazyRecordValue(lazyRecordType)); - - FormulaValue formulaValue = checkResult.GetEvaluator().Eval(symbolValues); - StringValue stringValue = Assert.IsType(formulaValue); - - Assert.Equal(@"{""field1"":10,""field2"":""Str""}", stringValue.Value); - } - - public class LazyRecordValue : RecordValue - { - public LazyRecordValue(RecordType type) - : base(type) - { - } - - protected override bool TryGetField(FormulaType fieldType, string fieldName, out FormulaValue result) - { - if (fieldName == "field1") - { - result = FormulaValue.New(10); - return true; - } - - if (fieldName == "field2") - { - result = FormulaValue.New("Str"); - return true; - } - - result = null; - return false; - } + } + + [Fact] + public void LazyJsonTest() + { + bool LazyGetField1(string name, out FormulaType type) + { + type = name switch + { + "field1" => FormulaType.Decimal, + "field2" => FormulaType.String, + _ => FormulaType.Blank, + }; + + return type != FormulaType.Blank; + } + + PowerFxConfig config = new PowerFxConfig(); + config.EnableJsonFunctions(); + + RecalcEngine engine = new RecalcEngine(config); + SymbolTable symbolTable = new SymbolTable(); + TestLazyRecordType lazyRecordType = new TestLazyRecordType("test", new List() { "field1", "field2" }, LazyGetField1); + + ISymbolSlot slot = symbolTable.AddVariable("var1", lazyRecordType); + + CheckResult checkResult = engine.Check("JSON(var1)", symbolTable: symbolTable); + Assert.True(checkResult.IsSuccess, string.Join(", ", checkResult.Errors.Select(err => err.Message))); + + SymbolValues symbolValues = new SymbolValues(symbolTable); + symbolValues.Set(slot, new LazyRecordValue(lazyRecordType)); + + FormulaValue formulaValue = checkResult.GetEvaluator().Eval(symbolValues); + StringValue stringValue = Assert.IsType(formulaValue); + + Assert.Equal(@"{""field1"":10,""field2"":""Str""}", stringValue.Value); + } + + public class LazyRecordValue : RecordValue + { + public LazyRecordValue(RecordType type) + : base(type) + { + } + + protected override bool TryGetField(FormulaType fieldType, string fieldName, out FormulaValue result) + { + if (fieldName == "field1") + { + result = FormulaValue.New(10); + return true; + } + + if (fieldName == "field2") + { + result = FormulaValue.New("Str"); + return true; + } + + result = null; + return false; + } } [Theory] @@ -1865,12 +1865,7 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out true, 42.0)] - // Functions accept record with more/less fields - [InlineData( - "People := Type([{Name: Text, Age: Number}]); countMinors(p: People): Number = CountRows(Filter(p, Age < 18));", - "countMinors([{Name: \"Bob\", Age: 21, Title: \"Engineer\"}, {Name: \"Alice\", Age: 25, Title: \"Manager\"}])", - true, - 0.0)] + // Functions accept record with less fields [InlineData( "Employee := Type({Name: Text, Age: Number, Title: Text}); getAge(e: Employee): Number = e.Age;", "getAge({Name: \"Bob\", Age: 21})", @@ -1952,13 +1947,22 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out true, 1.0)] + // Aggregate types with more than expected fields are not allowed in UDF + [InlineData( + "f():T = {x: 5, y: 5}; T := Type({x: Number});", + "f().x", + false)] + [InlineData( + "People := Type([{Name: Text, Age: Number}]); countMinors(p: People): Number = CountRows(Filter(p, Age < 18));", + "countMinors([{Name: \"Bob\", Age: 21, Title: \"Engineer\"}, {Name: \"Alice\", Age: 25, Title: \"Manager\"}])", + false)] public void UserDefinedTypeTest(string userDefinitions, string evalExpression, bool isValid, double expectedResult = 0) { var config = new PowerFxConfig(); var recalcEngine = new RecalcEngine(config); var parserOptions = new ParserOptions() { - AllowsSideEffects = false, + AllowsSideEffects = false, }; if (isValid) @@ -1974,24 +1978,24 @@ public void UserDefinedTypeTest(string userDefinitions, string evalExpression, b { Assert.Throws(() => recalcEngine.AddUserDefinitions(userDefinitions, CultureInfo.InvariantCulture)); } - } - - [Theory] + } + + [Theory] [InlineData( "F():Point = {x: 5, y:5};", "F().x", - true, + true, 5.0)] [InlineData( "F():Number = { Sqrt(1); 42; };", "F()", - true, - 42.0)] + true, + 42.0)] [InlineData( "F():Point = { {x:0, y:1729}; };", "F().y", - true, - 1729.0)] + true, + 1729.0)] [InlineData( "F():Point = {x: 5, 42};", "F().x", @@ -2003,9 +2007,9 @@ public void UDFImperativeVsRecordAmbiguityTest(string udf, string evalExpression var parserOptions = new ParserOptions() { AllowsSideEffects = true, - }; - - var extraSymbols = new SymbolTable(); + }; + + var extraSymbols = new SymbolTable(); extraSymbols.AddType(new DName("Point"), FormulaType.Build(TestUtils.DT("![x:n, y:n]"))); if (isValid) From 9a6f43c040e75676f863b9117d58d9b323c29d0d Mon Sep 17 00:00:00 2001 From: Adithya Selvaprithiviraj Date: Fri, 22 Nov 2024 15:22:53 -0800 Subject: [PATCH 2/5] fix test --- .../RecalcEngineTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index 05ef9e8597..ec2fff95ac 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -1976,7 +1976,11 @@ public void UserDefinedTypeTest(string userDefinitions, string evalExpression, b } else { - Assert.Throws(() => recalcEngine.AddUserDefinitions(userDefinitions, CultureInfo.InvariantCulture)); + Assert.ThrowsAny(() => + { + recalcEngine.AddUserDefinitions(userDefinitions, CultureInfo.InvariantCulture); + recalcEngine.Eval(evalExpression, options: parserOptions); + }); } } From 7f5a52538f6e9d84ea0792d948553d71159d2619 Mon Sep 17 00:00:00 2001 From: Adithya Selvaprithiviraj Date: Fri, 13 Dec 2024 15:41:51 -0800 Subject: [PATCH 3/5] improve and fix error messages --- .../Functions/TexlFunction.cs | 2 +- .../Functions/UserDefinedFunction.cs | 6 +- .../Localization/Strings.cs | 1 + .../Microsoft.PowerFx.Core/Types/DType.cs | 2 +- src/strings/PowerFxResources.en-US.resx | 4 + .../RecalcEngineTests.cs | 584 +++++++++--------- 6 files changed, 303 insertions(+), 296 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs index f3b9d73073..5778346a41 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs @@ -559,7 +559,7 @@ private bool CheckTypesCore(CheckTypesContext context, TexlNode[] args, DType[] } Contracts.AssertValid(argTypes[i]); - var expectedParamType = ParamTypes[i]; + var expectedParamType = ParamTypes[i]; // If the strong-enum type flag is disabled, treat an enum option set type as the enum supertype instead if (!context.Features.StronglyTypedBuiltinEnums && expectedParamType.OptionSetInfo is EnumSymbol enumSymbol) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs index 69981a88b8..23acd4ebde 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs @@ -58,6 +58,8 @@ public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding) public override bool SupportsParamCoercion => true; + public override bool HasPreciseErrors => true; + private const int MaxParameterCount = 30; public TexlNode UdfBody { get; } @@ -91,7 +93,7 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp !ParamTypes[i].Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules, true) && !argTypes[i].CoercesTo(ParamTypes[i], true, false, context.Features, true)) { - errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrBadSchema_ExpectedType, argTypes[i].GetKindString(), ParamTypes[i].GetKindString()); + errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrBadSchema_ExpectedType, ParamTypes[i].GetKindString()); return false; } } @@ -201,7 +203,7 @@ public void CheckTypesOnDeclaration(CheckTypesContext context, DType actualBodyR if ((ReturnType.IsTable && actualBodyReturnType.IsTable) || (ReturnType.IsRecord && actualBodyReturnType.IsRecord)) { - binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Severe, node, TexlStrings.ErrBadSchema_ExpectedType, ReturnType.GetKindString(), actualBodyReturnType.GetKindString()); + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Severe, node, TexlStrings.ErrUDF_ReturnTypeSchemaDoesNotMatch, ReturnType.GetKindString()); return; } diff --git a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs index 33610571dc..b3eecd25c9 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs @@ -783,6 +783,7 @@ internal static class TexlStrings public static ErrorResourceKey ErrUDF_DuplicateParameter = new ErrorResourceKey("ErrUDF_DuplicateParameter"); public static ErrorResourceKey ErrUDF_UnknownType = new ErrorResourceKey("ErrUDF_UnknownType"); public static ErrorResourceKey ErrUDF_ReturnTypeDoesNotMatch = new ErrorResourceKey("ErrUDF_ReturnTypeDoesNotMatch"); + public static ErrorResourceKey ErrUDF_ReturnTypeSchemaDoesNotMatch = new ErrorResourceKey("ErrUDF_ReturnTypeSchemaDoesNotMatch"); public static ErrorResourceKey ErrUDF_TooManyParameters = new ErrorResourceKey("ErrUDF_TooManyParameters"); public static ErrorResourceKey ErrUDF_MissingReturnType = new ErrorResourceKey("ErrUDF_MissingReturnType"); public static ErrorResourceKey ErrUDF_MissingParamType = new ErrorResourceKey("ErrUDF_MissingParamType"); diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs index 11bf2f5d06..2bfbd65ae5 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs @@ -3314,7 +3314,7 @@ public virtual bool CoercesTo( return false; } - if (typeDest.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules)) + if (typeDest.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules, restrictiveAggregateTypes)) { coercionType = typeDest; return true; diff --git a/src/strings/PowerFxResources.en-US.resx b/src/strings/PowerFxResources.en-US.resx index 998cd665d6..e8f01ecbdc 100644 --- a/src/strings/PowerFxResources.en-US.resx +++ b/src/strings/PowerFxResources.en-US.resx @@ -4221,6 +4221,10 @@ The stated function return type '{0}' does not match the return type of the function body '{1}'. This error message shows up when expected return type does not match with actual return type. The arguments '{0}' and '{1}' will be replaced with data types. For example, "The stated function return type 'Number' does not match the return type of the function body 'Table'" + + The schema of stated function return type '{0}' does not match the schema of return type of the function body. + This error message shows up when expected return type schema does not match with schema of actual return type. The arguments '{0}' will be replaced with aggregate data types. For example, "The schema of stated function return type 'Table' does not match the schema of return type of the function body." + Function {0} has too many parameters. User-defined functions support up to {1} parameters. This error message shows up when a user tries to define a function with too many parameters. {0} - the name of the user-defined function, {1} - the max number of parameters allowed. diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index 3487130dc5..f0873041d2 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -10,7 +10,7 @@ using System.Threading.Tasks; using Microsoft.PowerFx.Core; using Microsoft.PowerFx.Core.Functions; -using Microsoft.PowerFx.Core.Functions.Delegation; +using Microsoft.PowerFx.Core.Functions.Delegation; using Microsoft.PowerFx.Core.Functions.Delegation.DelegationMetadata; using Microsoft.PowerFx.Core.IR; using Microsoft.PowerFx.Core.Localization; @@ -398,8 +398,8 @@ public void UserDefinitionOnUpdateTest() // 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() { @@ -412,20 +412,20 @@ public void BasicEval() [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); + { + 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] @@ -486,7 +486,7 @@ public void FormulaCantRedefine() false, 14.0)] - // Recursive calls are not allowed + // Recursive calls are not allowed [InlineData( "foo(x:Number):Number = If(x=0,foo(1),If(x=1,foo(2),If(x=2,Float(2))));", "foo(Float(0))", @@ -614,78 +614,78 @@ public void FunctionPrecedenceTest(string script, double expected) var result = check.GetEvaluator().Eval(); Assert.Equal(expected, result.AsDouble()); - } - + } + [Fact] public void ShadowingFunctionPrecedenceTest() - { + { var engine = new RecalcEngine(); - engine.AddUserDefinedFunction("Concat(x:Text):Text = \"xyz\"; Average(x:Number):Number = 11111;"); + engine.AddUserDefinedFunction("Concat(x:Text):Text = \"xyz\"; Average(x:Number):Number = 11111;"); var check = engine.Check("Concat(\"abc\")"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.String, check.ReturnType); - var result = check.GetEvaluator().Eval(); - Assert.Equal("xyz", ((StringValue)result).Value); - + var result = check.GetEvaluator().Eval(); + Assert.Equal("xyz", ((StringValue)result).Value); + check = engine.Check("Average(123)"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Number, check.ReturnType); - result = check.GetEvaluator().Eval(); - Assert.Equal(11111, result.AsDouble()); - - engine.AddUserDefinitions("Test := Type({A: Number}); TestTable := Type([{A: Number}]);" + - "Filter(X: TestTable):Test = First(X); ShowColumns(X: TestTable):TestTable = FirstN(X, 3);"); - + result = check.GetEvaluator().Eval(); + Assert.Equal(11111, result.AsDouble()); + + engine.AddUserDefinitions("Test := Type({A: Number}); TestTable := Type([{A: Number}]);" + + "Filter(X: TestTable):Test = First(X); ShowColumns(X: TestTable):TestTable = FirstN(X, 3);"); + check = engine.Check("Filter([{A: 123}]).A"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Number, check.ReturnType); - result = check.GetEvaluator().Eval(); - Assert.Equal(123, result.AsDouble()); - + result = check.GetEvaluator().Eval(); + Assert.Equal(123, result.AsDouble()); + check = engine.Check("CountRows(ShowColumns([{A: 123}, {A: 124}, {A:125}, {A:126}, {A: 127}]))"); Assert.True(check.IsSuccess); Assert.Equal(FormulaType.Decimal, check.ReturnType); - result = check.GetEvaluator().Eval(); + result = check.GetEvaluator().Eval(); Assert.Equal(3, result.AsDouble()); } - - [Theory] - - // Behavior function in non-imperative udf + + [Theory] + + // Behavior function in non-imperative udf [InlineData( "TestFunc():Void = Set(a, 123);", true, - "Behavior function in a non-behavior user-defined function", - false)] - - // Behavior function in imperative udf + "Behavior function in a non-behavior user-defined function", + false)] + + // Behavior function in imperative udf [InlineData( "TestFunc():Void = { Set(a, 123); };", false, - null, - true)] + null, + true)] public void BehaviorFunctionInImperativeUDF(string udfExpression, bool expectedError, string expectedErrorKey, bool allowSideEffects) - { - var config = new PowerFxConfig(); - config.EnableSetFunction(); - var engine = new RecalcEngine(config); - engine.UpdateVariable("a", 1m); - - var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); - Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); - - if (expectedError) - { - result.Errors.Any(error => error.MessageKey == expectedErrorKey); + { + var config = new PowerFxConfig(); + config.EnableSetFunction(); + var engine = new RecalcEngine(config); + engine.UpdateVariable("a", 1m); + + var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); + Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); + + if (expectedError) + { + result.Errors.Any(error => error.MessageKey == expectedErrorKey); } - } - + } + [Theory] // Return value with side effectful UDF @@ -710,12 +710,12 @@ public void ImperativeUserDefinedFunctionTest(string udfExpression, string expre config.EnableSetFunction(); var recalcEngine = new RecalcEngine(config); recalcEngine.UpdateVariable("a", 1m); + + var definitionsCheckResult = recalcEngine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); - var definitionsCheckResult = recalcEngine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); - - if (!expectedError) - { - Assert.True(definitionsCheckResult.IsSuccess); + if (!expectedError) + { + Assert.True(definitionsCheckResult.IsSuccess); var result = recalcEngine.Eval(expression, options: _opts); var fvExpected = FormulaValue.New(expected); @@ -724,136 +724,136 @@ public void ImperativeUserDefinedFunctionTest(string udfExpression, string expre } else { - Assert.False(definitionsCheckResult.IsSuccess); + Assert.False(definitionsCheckResult.IsSuccess); Assert.Single(definitionsCheckResult.Errors); Assert.Contains(definitionsCheckResult.Errors, err => err.MessageKey == errorKey); } - } - - [Theory] - - [InlineData( + } + + [Theory] + + [InlineData( "MismatchType():Number = { 1+3; Color.Blue; };", true, - true, - 36, - 41)] - [InlineData( + true, + 36, + 41)] + [InlineData( "MatchType():Text = { 4; 3 };", false, - true, - 0, + true, + 0, 0)] public void TestMismatchReturnType(string udfExpression, bool expectedError, bool allowSideEffects, int min, int lim) { var config = new PowerFxConfig(); config.EnableSetFunction(); var engine = new RecalcEngine(config); - engine.UpdateVariable("x", 1m); - - var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); - Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); - - if (expectedError) - { - var error = result.Errors.First(error => error.MessageKey == "ErrUDF_ReturnTypeDoesNotMatch"); - Assert.NotNull(error); - var span = error.Span; - Assert.True(span.Min == min && span.Lim == lim); + engine.UpdateVariable("x", 1m); + + var result = engine.AddUserDefinedFunction(udfExpression, CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: allowSideEffects); + Assert.True(expectedError ? result.Errors.Count() > 0 : result.Errors.Count() == 0); + + if (expectedError) + { + var error = result.Errors.First(error => error.MessageKey == "ErrUDF_ReturnTypeDoesNotMatch"); + Assert.NotNull(error); + var span = error.Span; + Assert.True(span.Min == min && span.Lim == lim); } - } - + } + [Fact] public void DelegableUDFTest() - { - var symbolTable = new DelegatableSymbolTable(); - var schema = DType.CreateTable( - new TypedName(DType.Guid, new DName("ID")), + { + var symbolTable = new DelegatableSymbolTable(); + var schema = DType.CreateTable( + new TypedName(DType.Guid, new DName("ID")), new TypedName(DType.Number, new DName("Value"))); - symbolTable.AddEntity(new TestDelegableDataSource( - "MyDataSource", - schema, - new TestDelegationMetadata( - new DelegationCapability(DelegationCapability.Filter), - schema, - new FilterOpMetadata( - schema, - new Dictionary(), - new Dictionary(), - new DelegationCapability(DelegationCapability.GreaterThan), - null)), - true)); + symbolTable.AddEntity(new TestDelegableDataSource( + "MyDataSource", + schema, + new TestDelegationMetadata( + new DelegationCapability(DelegationCapability.Filter), + schema, + new FilterOpMetadata( + schema, + new Dictionary(), + new Dictionary(), + new DelegationCapability(DelegationCapability.GreaterThan), + null)), + true)); symbolTable.AddType(new DName("MyDataSourceTableType"), FormulaType.Build(schema)); - var config = new PowerFxConfig() - { + var config = new PowerFxConfig() + { SymbolTable = symbolTable }; - config.EnableSetFunction(); - - var recalcEngine = new RecalcEngine(config); - - recalcEngine.AddUserDefinedFunction("A():MyDataSourceTableType = Filter(MyDataSource, Value > 10);C():MyDataSourceTableType = A(); B():MyDataSourceTableType = Filter(C(), Value > 11); D():MyDataSourceTableType = { Filter(B(), Value > 12); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); - var func = recalcEngine.Functions.WithName("A").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("C").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("B").First() as UserDefinedFunction; - - Assert.True(func.IsAsync); - Assert.True(func.IsDelegatable); - - func = recalcEngine.Functions.WithName("D").First() as UserDefinedFunction; - - // Imperative function is not delegable - Assert.True(func.IsAsync); - Assert.True(!func.IsDelegatable); - - // Binding fails for recursive definitions and hence function is not added. - Assert.False(recalcEngine.AddUserDefinedFunction("E():Void = { E(); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true).IsSuccess); - } - - [Fact] + config.EnableSetFunction(); + + var recalcEngine = new RecalcEngine(config); + + recalcEngine.AddUserDefinedFunction("A():MyDataSourceTableType = Filter(MyDataSource, Value > 10);C():MyDataSourceTableType = A(); B():MyDataSourceTableType = Filter(C(), Value > 11); D():MyDataSourceTableType = { Filter(B(), Value > 12); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true); + var func = recalcEngine.Functions.WithName("A").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("C").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("B").First() as UserDefinedFunction; + + Assert.True(func.IsAsync); + Assert.True(func.IsDelegatable); + + func = recalcEngine.Functions.WithName("D").First() as UserDefinedFunction; + + // Imperative function is not delegable + Assert.True(func.IsAsync); + Assert.True(!func.IsDelegatable); + + // Binding fails for recursive definitions and hence function is not added. + Assert.False(recalcEngine.AddUserDefinedFunction("E():Void = { E(); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true).IsSuccess); + } + + [Fact] public void TestInheritanceOfDelegationWarningsInUDFs() - { - var symbolTable = new DelegatableSymbolTable(); - var schema = DType.CreateTable( + { + var symbolTable = new DelegatableSymbolTable(); + var schema = DType.CreateTable( new TypedName(DType.Number, new DName("Value"))); - symbolTable.AddEntity(new TestDelegableDataSource( - "MyDataSource", - schema, - new TestDelegationMetadata( - new DelegationCapability(DelegationCapability.Filter), - schema, - new FilterOpMetadata( - schema, - new Dictionary(), - new Dictionary(), - new DelegationCapability(DelegationCapability.GreaterThan), - null)), - true)); + symbolTable.AddEntity(new TestDelegableDataSource( + "MyDataSource", + schema, + new TestDelegationMetadata( + new DelegationCapability(DelegationCapability.Filter), + schema, + new FilterOpMetadata( + schema, + new Dictionary(), + new Dictionary(), + new DelegationCapability(DelegationCapability.GreaterThan), + null)), + true)); symbolTable.AddType(new DName("MyDataSourceTableType"), FormulaType.Build(schema)); - var config = new PowerFxConfig() - { + var config = new PowerFxConfig() + { SymbolTable = symbolTable }; - var engine = new RecalcEngine(config); - - var result = engine.AddUserDefinedFunction("NonDelegatableUDF():MyDataSourceTableType = Filter(MyDataSource, Sqrt(Value) > 5);", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); - Assert.True(result.IsSuccess); - var func = engine.Functions.WithName("NonDelegatableUDF").First() as UserDefinedFunction; - Assert.True(func.HasDelegationWarning); + var engine = new RecalcEngine(config); - result = engine.AddUserDefinedFunction("NonDelegatableUDF2():MyDataSourceTableType = NonDelegatableUDF();", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); - Assert.True(result.IsSuccess); - func = engine.Functions.WithName("NonDelegatableUDF2").First() as UserDefinedFunction; - Assert.True(func.HasDelegationWarning); + var result = engine.AddUserDefinedFunction("NonDelegatableUDF():MyDataSourceTableType = Filter(MyDataSource, Sqrt(Value) > 5);", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); + Assert.True(result.IsSuccess); + var func = engine.Functions.WithName("NonDelegatableUDF").First() as UserDefinedFunction; + Assert.True(func.HasDelegationWarning); + + result = engine.AddUserDefinedFunction("NonDelegatableUDF2():MyDataSourceTableType = NonDelegatableUDF();", CultureInfo.InvariantCulture, symbolTable: engine.EngineSymbols, allowSideEffects: true); + Assert.True(result.IsSuccess); + func = engine.Functions.WithName("NonDelegatableUDF2").First() as UserDefinedFunction; + Assert.True(func.HasDelegationWarning); } // Binding to inner functions does not impact outer functions. @@ -1768,68 +1768,68 @@ public void LookupBuiltinOptionSets() Assert.True(ok); // Wrong type: https://github.com/microsoft/Power-Fx/issues/2342 - } - - [Fact] - public void LazyJsonTest() - { - bool LazyGetField1(string name, out FormulaType type) - { - type = name switch - { - "field1" => FormulaType.Decimal, - "field2" => FormulaType.String, - _ => FormulaType.Blank, - }; - - return type != FormulaType.Blank; - } - - PowerFxConfig config = new PowerFxConfig(); - config.EnableJsonFunctions(); - - RecalcEngine engine = new RecalcEngine(config); - SymbolTable symbolTable = new SymbolTable(); - TestLazyRecordType lazyRecordType = new TestLazyRecordType("test", new List() { "field1", "field2" }, LazyGetField1); - - ISymbolSlot slot = symbolTable.AddVariable("var1", lazyRecordType); - - CheckResult checkResult = engine.Check("JSON(var1)", symbolTable: symbolTable); - Assert.True(checkResult.IsSuccess, string.Join(", ", checkResult.Errors.Select(err => err.Message))); - - SymbolValues symbolValues = new SymbolValues(symbolTable); - symbolValues.Set(slot, new LazyRecordValue(lazyRecordType)); - - FormulaValue formulaValue = checkResult.GetEvaluator().Eval(symbolValues); - StringValue stringValue = Assert.IsType(formulaValue); - - Assert.Equal(@"{""field1"":10,""field2"":""Str""}", stringValue.Value); - } - - public class LazyRecordValue : RecordValue - { - public LazyRecordValue(RecordType type) - : base(type) - { - } - - protected override bool TryGetField(FormulaType fieldType, string fieldName, out FormulaValue result) - { - if (fieldName == "field1") - { - result = FormulaValue.New(10); - return true; - } - - if (fieldName == "field2") - { - result = FormulaValue.New("Str"); - return true; - } - - result = null; - return false; - } + } + + [Fact] + public void LazyJsonTest() + { + bool LazyGetField1(string name, out FormulaType type) + { + type = name switch + { + "field1" => FormulaType.Decimal, + "field2" => FormulaType.String, + _ => FormulaType.Blank, + }; + + return type != FormulaType.Blank; + } + + PowerFxConfig config = new PowerFxConfig(); + config.EnableJsonFunctions(); + + RecalcEngine engine = new RecalcEngine(config); + SymbolTable symbolTable = new SymbolTable(); + TestLazyRecordType lazyRecordType = new TestLazyRecordType("test", new List() { "field1", "field2" }, LazyGetField1); + + ISymbolSlot slot = symbolTable.AddVariable("var1", lazyRecordType); + + CheckResult checkResult = engine.Check("JSON(var1)", symbolTable: symbolTable); + Assert.True(checkResult.IsSuccess, string.Join(", ", checkResult.Errors.Select(err => err.Message))); + + SymbolValues symbolValues = new SymbolValues(symbolTable); + symbolValues.Set(slot, new LazyRecordValue(lazyRecordType)); + + FormulaValue formulaValue = checkResult.GetEvaluator().Eval(symbolValues); + StringValue stringValue = Assert.IsType(formulaValue); + + Assert.Equal(@"{""field1"":10,""field2"":""Str""}", stringValue.Value); + } + + public class LazyRecordValue : RecordValue + { + public LazyRecordValue(RecordType type) + : base(type) + { + } + + protected override bool TryGetField(FormulaType fieldType, string fieldName, out FormulaValue result) + { + if (fieldName == "field1") + { + result = FormulaValue.New(10); + return true; + } + + if (fieldName == "field2") + { + result = FormulaValue.New("Str"); + return true; + } + + result = null; + return false; + } } [Theory] @@ -1884,7 +1884,7 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out hasNoAge(e: Employee): Number = IsBlank(getAge(e));", "hasNoAge({Name: \"Bob\", Title: \"CEO\"})", true, - 1.0)] + 1.0)] // Types with UDF restricted primitive types resolve successfully [InlineData( @@ -1944,8 +1944,8 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out "f():TestEntity = Entity; g(e: TestEntity):Number = 1;", "g(f())", true, - 1.0)] - + 1.0)] + // Aggregate types with more than expected fields are not allowed in UDF [InlineData( "f():T = {x: 5, y: 5}; T := Type({x: Number});", @@ -1961,7 +1961,7 @@ public void UserDefinedTypeTest(string userDefinitions, string evalExpression, b var recalcEngine = new RecalcEngine(config); var parserOptions = new ParserOptions() { - AllowsSideEffects = false, + AllowsSideEffects = false, }; if (isValid) @@ -1975,30 +1975,30 @@ public void UserDefinedTypeTest(string userDefinitions, string evalExpression, b } else { - Assert.ThrowsAny(() => - { + Assert.ThrowsAny(() => + { recalcEngine.AddUserDefinitions(userDefinitions, CultureInfo.InvariantCulture); recalcEngine.Eval(evalExpression, options: parserOptions); }); } - } - - [Theory] + } + + [Theory] [InlineData( "F():Point = {x: 5, y:5};", "F().x", - true, + true, 5.0)] [InlineData( "F():Number = { Sqrt(1); 42; };", "F()", - true, - 42.0)] + true, + 42.0)] [InlineData( "F():Point = { {x:0, y:1729}; };", "F().y", - true, - 1729.0)] + true, + 1729.0)] [InlineData( "F():Point = {x: 5, 42};", "F().x", @@ -2010,9 +2010,9 @@ public void UDFImperativeVsRecordAmbiguityTest(string udf, string evalExpression var parserOptions = new ParserOptions() { AllowsSideEffects = true, - }; - - var extraSymbols = new SymbolTable(); + }; + + var extraSymbols = new SymbolTable(); extraSymbols.AddType(new DName("Point"), FormulaType.Build(TestUtils.DT("![x:n, y:n]"))); if (isValid) @@ -2024,117 +2024,117 @@ public void UDFImperativeVsRecordAmbiguityTest(string udf, string evalExpression { Assert.False(recalcEngine.AddUserDefinedFunction(udf, CultureInfo.InvariantCulture, extraSymbols, true).IsSuccess); } - } - - [Theory] + } + + [Theory] [InlineData( "Points := Type([{x : Number, y : Number}]); Point := Type(RecordOf(Points)); distance(a: Point, b: Point): Number = Sqrt(Power(b.x-a.x, 2) + Power(b.y-a.y, 2));", "distance({x: 0, y: 0}, {x: 0, y: 5})", true, - 5.0)] + 5.0)] [InlineData( "Account := Type(RecordOf(Accounts)); getAccountId(a: Account): Number = a.id;", "getAccountId({id: 42, name: \"T-Rex\", address: \"Museum\"})", true, - 42.0)] + 42.0)] [InlineData( "Account := Type(RecordOf(Accounts)); FirstAccount(a: Accounts): Account = First(a); getAccountId(a: Account): Number = a.id;", "getAccountId(FirstAccount([{id: 1729, name: \"Bob\"}, {id: 42, name: \"T-Rex\"}]))", true, - 1729.0)] + 1729.0)] [InlineData( "NewAccounts := Type([RecordOf(Accounts)]); getFirstAccountId(a: NewAccounts): Number = First(a).id;", "getFirstAccountId([{id: 1729, name: \"Bob\"}, {id: 42, name: \"T-Rex\"}])", true, - 1729.0)] + 1729.0)] [InlineData( "AccountWithAge := Type({age: Number, acc: RecordOf(Accounts)}); getAccountAge(a: AccountWithAge): Number = a.age;", "getAccountAge({age : 25, acc:First([{id: 1729, name: \"Bob\"}, {id: 42, name: \"T-Rex\"}])})", true, - 25.0)] + 25.0)] [InlineData( "Points := Type([{x : Number, y : Number}]); ComplexType := Type({p: RecordOf(Points), a: RecordOf(Accounts), s: SomeRecord}); getX(c: ComplexType): Number = c.p.x;", "getX({s: {id: 1729, name: \"Bob\"}, p : {x: 1, y: 2}, a: {id: 42, name: \"Alice\", address: \"internet\"}})", true, - 1.0)] - - // No error on a UDF named RecordOf + 1.0)] + + // No error on a UDF named RecordOf [InlineData( @"RecordOf(x:Number): Number = x + 1;", "RecordOf(41)", true, - 42.0)] - - // Fails for any type other than table - [InlineData( - "Account := Type(RecordOf(SomeRecord));", - "", - false)] - [InlineData( - "Account := Type(RecordOf(Void));", - "", - false)] - [InlineData( - "Account := Type(RecordOf(UntypedObject));", - "", - false)] - - // invalid helper name + 42.0)] + + // Fails for any type other than table + [InlineData( + "Account := Type(RecordOf(SomeRecord));", + "", + false)] + [InlineData( + "Account := Type(RecordOf(Void));", + "", + false)] + [InlineData( + "Account := Type(RecordOf(UntypedObject));", + "", + false)] + + // invalid helper name [InlineData( "Account := Type(Recordof(Accounts));", "", - false)] + false)] [InlineData( "Account := Type(recordOf(Accounts));", "", - false)] + false)] [InlineData( "Account := Type(First(Accounts));", "", - false)] - - // Does not allow anything other firstname + false)] + + // Does not allow anything other firstname [InlineData( "Points := Type([{x : Number, y : Number}]); Point := Type(RecordOf(Points As P));", "", - false)] + false)] [InlineData( "Points := Type([{x : Number, y : Number}]); Point := Type(RecordOf(Points, Accounts));", "", - false)] + false)] [InlineData( "Account := Type((Accounts, SomeRecord));", "", - false)] + false)] [InlineData( "Point := Type(RecordOf([{x : Number, y : Number}]));", "", - false)] + false)] [InlineData( "T1 := Type(RecordOf(Type([{A:Number}])));", "", - false)] + false)] [InlineData( "T1 := Type(RecordOf(RecordOf([{x:Number, y:Number}])));", "", - false)] - - // RecordOf not in type literal + false)] + + // RecordOf not in type literal [InlineData( "Account = RecordOf(Accounts);", "", - false)] + false)] [InlineData( "F():Accounts = RecordOf(Accounts);", "", - false)] + false)] public void RecordOfTests(string userDefinitions, string evalExpression, bool isValid, double expectedResult = 0) { var config = new PowerFxConfig(); var recalcEngine = new RecalcEngine(config); - var parserOptions = new ParserOptions(); - - recalcEngine.Config.SymbolTable.AddType(new DName("Accounts"), FormulaType.Build(TestUtils.DT("*[id: n, name:s, address:s]"))); + var parserOptions = new ParserOptions(); + + recalcEngine.Config.SymbolTable.AddType(new DName("Accounts"), FormulaType.Build(TestUtils.DT("*[id: n, name:s, address:s]"))); recalcEngine.Config.SymbolTable.AddType(new DName("SomeRecord"), FormulaType.Build(TestUtils.DT("![id: n, name:s]"))); if (isValid) From 422a9b9cd32c8b9ca96c41da03240577520cc191 Mon Sep 17 00:00:00 2001 From: Adithya Selvaprithiviraj Date: Tue, 7 Jan 2025 10:06:41 -0800 Subject: [PATCH 4/5] Add Tests --- .../Functions/UserDefinedFunction.cs | 9 +++- .../Microsoft.PowerFx.Core/Types/DType.cs | 9 ++-- .../UserDefinedTypeTests.cs | 14 +++++ .../RecalcEngineTests.cs | 52 +++++++++++++++---- 4 files changed, 70 insertions(+), 14 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs index 23acd4ebde..939db68a88 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs @@ -191,9 +191,14 @@ public void CheckTypesOnDeclaration(CheckTypesContext context, DType actualBodyR Contracts.AssertValue(actualBodyReturnType); Contracts.AssertValue(binding); - if (!ReturnType.Accepts(actualBodyReturnType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules, true)) + if (!ReturnType.Accepts( + actualBodyReturnType, + exact: true, + useLegacyDateTimeAccepts: false, + usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules, + restrictiveAggregateTypes: true)) { - if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features, true)) + if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features, restrictiveAggregateTypes: true)) { _binding.SetCoercedType(binding.Top, ReturnType); } diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs index 2bfbd65ae5..5b9cad8e3f 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs @@ -1851,7 +1851,7 @@ private bool AcceptsEntityType(DType type, bool usePowerFxV1CompatibilityRules) /// Legacy rules for accepting date/time types. /// Use PFx v1 compatibility rules if enabled (less /// permissive Accepts relationships). - /// restrictiveAggregateTypes. + /// Flag to restrict using aggregate types with more fields than expected. /// /// True if accepts , false otherwise. /// @@ -1886,6 +1886,7 @@ public bool Accepts(DType type, bool exact, bool useLegacyDateTimeAccepts, bool /// Legacy rules for accepting date/time types. /// Use PFx v1 compatibility rules if enabled (less /// permissive Accepts relationships). + /// Flag to restrict using aggregate types with more fields than expected. /// /// True if accepts , false otherwise. /// @@ -3208,7 +3209,8 @@ public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coerci out schemaDifferenceType, aggregateCoercion: true, isTopLevelCoercion: false, - features); + features, + restrictiveAggregateTypes); } if (Kind != typeDest.Kind) @@ -3243,7 +3245,8 @@ public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coerci out var fieldSchemaDifferenceType, aggregateCoercion, isTopLevelCoercion: false, - features); + features, + restrictiveAggregateTypes); // This is the attempted coercion type. If we fail, we need to know this for error handling coercionType = coercionType.Add(typedName.Name, fieldCoercionType); diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/UserDefinedTypeTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/UserDefinedTypeTests.cs index 89e951ca6e..880e9ac56f 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/UserDefinedTypeTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/UserDefinedTypeTests.cs @@ -156,6 +156,20 @@ public void TestRecordOfErrors(string typeDefinition, string expectedMessageKey) Assert.Contains(errors, e => e.MessageKey.Contains(expectedMessageKey)); } + [Theory] + [InlineData("f():T = {x: 5, y: 5}; T := Type({x: Number});", "ErrUDF_ReturnTypeSchemaDoesNotMatch")] + [InlineData("f(x:T):Number = x.n; T := Type({n: Number}); g(): Number = f({n: 5, m: 5});", "ErrBadSchema_ExpectedType")] + [InlineData("f():T = [{x: 5, y: 5}]; T := Type([{x: Number}]);", "ErrUDF_ReturnTypeSchemaDoesNotMatch")] + [InlineData("f(x:T):T = x; T := Type([{n: Number}]); g(): T = f([{n: 5, m: 5}]);", "ErrBadSchema_ExpectedType")] + public void TestAggregateTypeErrors(string typeDefinition, string expectedMessageKey) + { + var checkResult = new DefinitionsCheckResult() + .SetText(typeDefinition) + .SetBindingInfo(_primitiveTypes); + var errors = checkResult.ApplyErrors(); + Assert.Contains(errors, e => e.MessageKey.Contains(expectedMessageKey)); + } + [Theory] [InlineData("T := Type({ x: 5+5, y: -5 });", 2)] [InlineData("T := Type(Type(Number));", 1)] diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index f0873041d2..44ffb9aeae 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -1871,13 +1871,6 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out "getAge({Name: \"Bob\", Age: 21})", true, 21.0)] - [InlineData( - @"Employee := Type({Name: Text, Age: Number, Title: Text}); Employees := Type([Employee]); EmployeeNames := Type([{Name: Text}]); - getNames(e: Employees):EmployeeNames = ShowColumns(e, Name); - getNamesCount(e: EmployeeNames):Number = CountRows(getNames(e));", - "getNamesCount([{Name: \"Jim\", Age:25}, {Name: \"Tony\", Age:42}])", - true, - 2.0)] [InlineData( @"Employee := Type({Name: Text, Age: Number, Title: Text}); getAge(e: Employee): Number = e.Age; @@ -1946,14 +1939,55 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out true, 1.0)] - // Aggregate types with more than expected fields are not allowed in UDF + // Aggregate types with more than expected fields are not allowed in UDF args and return types + // Records [InlineData( "f():T = {x: 5, y: 5}; T := Type({x: Number});", - "f().x", + "", + false)] + [InlineData( + "f():T = {x: 5, y: 5}; T1 := Type([{x: Number}]); T2 := Type(RecordOf(T1));", + "", + false)] + [InlineData( + "g(x:T):Number = x.n; T := Type({n: Number});", + "g({x: 5, y: 5})", + false)] + + // Nested Records + [InlineData( + "f():T = {a: 5, b: {c: {d: 5, e:42}}}; T := Type({a: Number, b: {c: {d: Number}}});", + "", + false)] + [InlineData( + "g(x:T):Number = x.b.c.d; T := Type({a: Number, b: {c: {d: Number}}});", + "g({a: 5, b: {c: {d: 5, e:42}}})", + false)] + + // Tables + [InlineData( + "f():T = [{x: 5, y: 5}]; T := Type([{x: Number}]);", + "", false)] [InlineData( "People := Type([{Name: Text, Age: Number}]); countMinors(p: People): Number = CountRows(Filter(p, Age < 18));", "countMinors([{Name: \"Bob\", Age: 21, Title: \"Engineer\"}, {Name: \"Alice\", Age: 25, Title: \"Manager\"}])", + false)] + [InlineData( + @"Employee := Type({Name: Text, Age: Number, Title: Text}); Employees := Type([Employee]); EmployeeNames := Type([{Name: Text}]); + getNames(e: Employees):EmployeeNames = ShowColumns(e, Name); + getNamesCount(e: EmployeeNames):Number = CountRows(getNames(e));", + "getNamesCount([{Name: \"Jim\", Age:25}, {Name: \"Tony\", Age:42}])", + false)] + + // Nested Tables + [InlineData( + "f():T = {a: 5, b: [{c: {d: 5, e:42}}]}; T := Type([{a: Number, b: [{c: {d: Number}}]}]);", + "", + false)] + [InlineData( + "g(x:T):Number = First(First(x).b).c.d; T := Type([{a: Number, b: [{c: {d: Number}}]}])", + "g({a: 5, b: [{c: {d: 5, e:42}}]})", false)] public void UserDefinedTypeTest(string userDefinitions, string evalExpression, bool isValid, double expectedResult = 0) { From 00c51ac12962a3bdb0744c51548ec95600a9a5d2 Mon Sep 17 00:00:00 2001 From: Adithya Selvaprithiviraj Date: Tue, 7 Jan 2025 10:22:54 -0800 Subject: [PATCH 5/5] fix tests --- .../RecalcEngineTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs index 44ffb9aeae..cdfdbf17a2 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -1946,7 +1946,7 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out "", false)] [InlineData( - "f():T = {x: 5, y: 5}; T1 := Type([{x: Number}]); T2 := Type(RecordOf(T1));", + "f():T2 = {x: 5, y: 5}; T1 := Type([{x: Number}]); T2 := Type(RecordOf(T1));", "", false)] [InlineData( @@ -1986,7 +1986,7 @@ protected override bool TryGetField(FormulaType fieldType, string fieldName, out "", false)] [InlineData( - "g(x:T):Number = First(First(x).b).c.d; T := Type([{a: Number, b: [{c: {d: Number}}]}])", + "g(x:T):Number = First(First(x).b).c.d; T := Type([{a: Number, b: [{c: {d: Number}}]}]);", "g({a: 5, b: [{c: {d: 5, e:42}}]})", false)] public void UserDefinedTypeTest(string userDefinitions, string evalExpression, bool isValid, double expectedResult = 0)