diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs index 31d436703c..23acd4ebde 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; @@ -57,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; } @@ -77,6 +80,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, ParamTypes[i].GetKindString()); + return false; + } + } + + return true; + } + /// /// Initializes a new instance of the class. /// @@ -167,15 +191,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.ErrUDF_ReturnTypeSchemaDoesNotMatch, ReturnType.GetKindString()); + return; + } + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Severe, node, TexlStrings.ErrUDF_ReturnTypeDoesNotMatch, ReturnType.GetKindString(), actualBodyReturnType.GetKindString()); } } 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/Public/DefinitionsCheckResult.cs b/src/libraries/Microsoft.PowerFx.Core/Public/DefinitionsCheckResult.cs index f45456fa47..88c02a23b0 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/DefinitionsCheckResult.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/DefinitionsCheckResult.cs @@ -116,6 +116,8 @@ internal ParseUserDefinitionResult ApplyParse() public bool ContainsUDF => _parse.UDFs.Any(); + public bool ContainsUDT => _parse.DefinedTypes.Any(); + internal IReadOnlyDictionary ApplyResolveTypes() { if (_parse == null) diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs index c33312a036..2bfbd65ae5 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DType.cs @@ -1851,12 +1851,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); } /// @@ -1888,7 +1889,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(); @@ -1941,7 +1942,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; @@ -1955,7 +1956,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; @@ -2175,7 +2176,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(); @@ -2215,7 +2216,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)) { @@ -2237,6 +2238,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; } @@ -3141,17 +3153,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); @@ -3259,6 +3271,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; } @@ -3273,7 +3296,8 @@ public virtual bool CoercesTo( out DType schemaDifferenceType, bool aggregateCoercion, bool isTopLevelCoercion, - Features features) + Features features, + bool restrictiveAggregateTypes = false) { AssertValid(); Contracts.Assert(typeDest.IsValid); @@ -3290,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; @@ -3335,7 +3359,8 @@ public virtual bool CoercesTo( out schemaDifference, out schemaDifferenceType, features, - aggregateCoercion); + aggregateCoercion, + restrictiveAggregateTypes); } var subtypeCoerces = SubtypeCoercesTo( diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs index f85f9ad6e4..1189465b3e 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs @@ -386,6 +386,11 @@ public void UpdateSupportedFunctions(SymbolTable s) SupportedFunctions = s; } + public ReadOnlySymbolTable GetAllSymbols() + { + return SymbolTable.Compose(Config.ComposedConfigSymbols, SupportedFunctions, _symbolTable, PrimitiveTypes); + } + /// /// Add a set of user-defined formulas and functions to the engine. /// @@ -420,7 +425,7 @@ public void AddUserDefinitions(string script, CultureInfo parseCulture = null, A } // Compose will handle null symbols - var composedSymbols = SymbolTable.Compose(Config.ComposedConfigSymbols, SupportedFunctions, PrimitiveTypes, _symbolTable); + var composedSymbols = SymbolTable.Compose(Config.ComposedConfigSymbols, SupportedFunctions, _symbolTable, PrimitiveTypes); if (parseResult.DefinedTypes.Any()) { diff --git a/src/libraries/Microsoft.PowerFx.Repl/Repl.cs b/src/libraries/Microsoft.PowerFx.Repl/Repl.cs index 1a04efc2ee..a5940da024 100644 --- a/src/libraries/Microsoft.PowerFx.Repl/Repl.cs +++ b/src/libraries/Microsoft.PowerFx.Repl/Repl.cs @@ -508,13 +508,13 @@ await this.Output.WriteLineAsync($"Error: Can't set '{name}' to a Void value.", definitionsCheckResult.SetText(expression, this.ParserOptions) .ApplyParseErrors(); - if (this.AllowUserDefinedFunctions && definitionsCheckResult.IsSuccess && definitionsCheckResult.ContainsUDF) + if (definitionsCheckResult.IsSuccess) { - var defCheckResult = this.Engine.AddUserDefinedFunction(expression, this.ParserOptions.Culture, extraSymbolTable); + definitionsCheckResult.SetBindingInfo(this.Engine.GetAllSymbols()); - if (!defCheckResult.IsSuccess) + if (definitionsCheckResult.ApplyErrors().Any()) { - foreach (var error in defCheckResult.Errors) + foreach (var error in definitionsCheckResult.Errors) { var kind = error.IsWarning ? OutputKind.Warning : OutputKind.Error; var msg = error.ToString(); @@ -522,11 +522,26 @@ await this.Output.WriteLineAsync($"Error: Can't set '{name}' to a Void value.", await this.Output.WriteLineAsync(lineError + msg, kind, cancel) .ConfigureAwait(false); } + return new ReplResult(); } + this.Engine.AddUserDefinitions(expression, this.ParserOptions.Culture); + return new ReplResult(); } + if (check.ApplyParse().Errors.Any() && definitionsCheckResult.Errors.Any()) + { + foreach (var error in definitionsCheckResult.Errors) + { + var kind = error.IsWarning ? OutputKind.Warning : OutputKind.Error; + var msg = error.ToString(); + + await this.Output.WriteLineAsync(lineError + msg, kind, cancel) + .ConfigureAwait(false); + } + } + foreach (var error in check.Errors) { var kind = error.IsWarning ? OutputKind.Warning : OutputKind.Error; 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 5efafa90d1..f0873041d2 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs @@ -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})", @@ -1949,7 +1944,17 @@ 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});", + "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(); @@ -1970,7 +1975,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); + }); } }