Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Aggregate types in UDF #2775

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix Aggregate types in UDF #2775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3046a16
Restrictive aggregate types in UDF
adithyaselv Nov 22, 2024
9a6f43c
fix test
adithyaselv Nov 22, 2024
a4aa143
Merge branch 'main' into adithyase/udf-restrictive-types
adithyaselv Dec 11, 2024
b386621
Merge branch 'main' into adithyase/udf-restrictive-types
adithyaselv Dec 13, 2024
7f5a525
improve and fix error messages
adithyaselv Dec 13, 2024
422a9b9
Add Tests
adithyaselv Jan 7, 2025
00c51ac
fix tests
adithyaselv Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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; }
Expand All @@ -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<TexlNode, DType> 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) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

Can you use a named argument to make it easier to understand what this parameter means?

!argTypes[i].CoercesTo(ParamTypes[i], true, false, context.Features, true))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, false

Can you use named arguments?

{
errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrBadSchema_ExpectedType, ParamTypes[i].GetKindString());
return false;
}
}

return true;
}

/// <summary>
/// Initializes a new instance of the <see cref="UserDefinedFunction"/> class.
/// </summary>
Expand Down Expand Up @@ -167,15 +191,27 @@ 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,
restrictiveAggregateTypes: true))
{
if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features))
if (actualBodyReturnType.CoercesTo(ReturnType, true, false, context.Features, restrictiveAggregateTypes: 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());
}
}
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
62 changes: 45 additions & 17 deletions src/libraries/Microsoft.PowerFx.Core/Types/DType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1851,12 +1851,13 @@ private bool AcceptsEntityType(DType type, bool usePowerFxV1CompatibilityRules)
/// <param name="useLegacyDateTimeAccepts">Legacy rules for accepting date/time types.</param>
/// <param name="usePowerFxV1CompatibilityRules">Use PFx v1 compatibility rules if enabled (less
/// permissive Accepts relationships).</param>
/// <param name="restrictiveAggregateTypes">Flag to restrict using aggregate types with more fields than expected.</param>
/// <returns>
/// True if <see cref="DType"/> accepts <paramref name="type"/>, false otherwise.
/// </returns>
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restrictiveAggregateTypes

Can you add tests to DTypeTests.cs? It is good to have unit tests closer to where the definition is

{
return Accepts(type, out _, out _, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules);
return Accepts(type, out _, out _, exact, useLegacyDateTimeAccepts, usePowerFxV1CompatibilityRules, restrictiveAggregateTypes);
}

/// <summary>
Expand Down Expand Up @@ -1885,10 +1886,11 @@ public bool Accepts(DType type, bool exact, bool useLegacyDateTimeAccepts, bool
/// <param name="useLegacyDateTimeAccepts">Legacy rules for accepting date/time types.</param>
/// <param name="usePowerFxV1CompatibilityRules">Use PFx v1 compatibility rules if enabled (less
/// permissive Accepts relationships).</param>
/// <param name="restrictiveAggregateTypes">Flag to restrict using aggregate types with more fields than expected.</param>
/// <returns>
/// True if <see cref="DType"/> accepts <paramref name="type"/>, false otherwise.
/// </returns>
public virtual bool Accepts(DType type, out KeyValuePair<string, DType> schemaDifference, out DType schemaDifferenceType, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules)
public virtual bool Accepts(DType type, out KeyValuePair<string, DType> schemaDifference, out DType schemaDifferenceType, bool exact, bool useLegacyDateTimeAccepts, bool usePowerFxV1CompatibilityRules, bool restrictiveAggregateTypes = false)
{
AssertValid();
type.AssertValid();
Expand Down Expand Up @@ -1941,7 +1943,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;
Expand All @@ -1955,7 +1957,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;
Expand Down Expand Up @@ -2175,7 +2177,7 @@ bool DefaultReturnValue(DType targetType) =>
}

// Implements Accepts for Record and Table types.
private static bool TreeAccepts(DType parentType, TypeTree treeDst, TypeTree treeSrc, out KeyValuePair<string, DType> 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<string, DType> schemaDifference, out DType treeSrcSchemaDifferenceType, bool exact = true, bool useLegacyDateTimeAccepts = false, bool usePowerFxV1CompatibilityRules = false, bool restrictiveAggregateTypes = false)
{
treeDst.AssertValid();
treeSrc.AssertValid();
Expand Down Expand Up @@ -2215,7 +2217,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))
{
Expand All @@ -2237,6 +2239,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;
}

Expand Down Expand Up @@ -3141,17 +3154,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<string, DType> schemaDifference, out DType schemaDifferenceType, Features features, bool aggregateCoercion = true)
public bool AggregateCoercesTo(DType typeDest, out bool isSafe, out DType coercionType, out KeyValuePair<string, DType> schemaDifference, out DType schemaDifferenceType, Features features, bool aggregateCoercion = true, bool restrictiveAggregateTypes = false)
{
Contracts.Assert(IsAggregate);

Expand Down Expand Up @@ -3196,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)
Expand Down Expand Up @@ -3231,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);
Expand Down Expand Up @@ -3259,6 +3274,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;
}

Expand All @@ -3273,7 +3299,8 @@ public virtual bool CoercesTo(
out DType schemaDifferenceType,
bool aggregateCoercion,
bool isTopLevelCoercion,
Features features)
Features features,
bool restrictiveAggregateTypes = false)
{
AssertValid();
Contracts.Assert(typeDest.IsValid);
Expand All @@ -3290,7 +3317,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;
Expand Down Expand Up @@ -3335,7 +3362,8 @@ public virtual bool CoercesTo(
out schemaDifference,
out schemaDifferenceType,
features,
aggregateCoercion);
aggregateCoercion,
restrictiveAggregateTypes);
}

var subtypeCoerces = SubtypeCoercesTo(
Expand Down
4 changes: 4 additions & 0 deletions src/strings/PowerFxResources.en-US.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4221,6 +4221,10 @@
<value>The stated function return type '{0}' does not match the return type of the function body '{1}'.</value>
<comment>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'"</comment>
</data>
<data name="ErrUDF_ReturnTypeSchemaDoesNotMatch" xml:space="preserve">
<value>The schema of stated function return type '{0}' does not match the schema of return type of the function body.</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema of stated function return type '{0}' does not match the schema of return type of the function body

This message is a little confusing. In the example from the test f():T = {x: 5, y: 5}; T := Type({x: Number});, the message will say something like "The schema of the state function return type 'Record' does not match the schema of return type of the function body" - but the maker did return a record. For example, we could say which field is extra / missing.

<comment>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."</comment>
</data>
<data name="ErrUDF_TooManyParameters" xml:space="preserve">
<value>Function {0} has too many parameters. User-defined functions support up to {1} parameters.</value>
<comment>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.</comment>
Expand Down
14 changes: 14 additions & 0 deletions src/tests/Microsoft.PowerFx.Core.Tests.Shared/UserDefinedTypeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
Loading
Loading