Skip to content

Commit

Permalink
Updated option set testing (#2446)
Browse files Browse the repository at this point in the history 
Fixes a bug in Canvas apps coercing an option set value to number. There
were missing checks for pre-V1 semantics in DType.cs.

But this bug, along with the other recent fixes, is because we were not
testing with actual option sets, instead relying on enums since those
can be built for all the backing kinds. In general, enums and option
sets aren't that different in V1, but they are very different in pre-V1
which is what Canvas uses today. This has been corrected in this PR. We
now run tests with the test enums registered as option sets instead,
mimicing the semantics in Canvas for Dataverse number and Boolean option
sets. These new typed option sets are not exposed generally, as we
should look at that implementation more closely, today it translates
between string values more than it needs to.

By doing this testing, another bug was found in the interpeter, where we
weren't back converting boolean values to the option set, for example in
`Text( If( false, OptionSet.Value, true ) )`. C# now has the same
semantics as Canvas for Booleans. Number and other backing kinds may
also want this behavior long term, but since those aren't yet supported
we have time to decide how to handle that scenario. Without any changes,
a `CalculatedOptionSetValue` will be returned for the label if coerced
to a string, but the value itself will be correct.
  • Loading branch information
@gregli-msft
gregli-msft authored May 31, 2024
1 parent 0f75d9f commit caa821f
Show file tree
Hide file tree
Showing 8 changed files with 2,346 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/libraries/Microsoft.PowerFx.Core/Types/DType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3414,7 +3414,7 @@ public virtual bool CoercesTo(
DateTime.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
Time.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
Date.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
(Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Number && OptionSetInfo.CanCoerceToBackingKind);
(Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Number && (OptionSetInfo.CanCoerceToBackingKind || !usePowerFxV1CompatibilityRules));
break;
case DKind.Currency:
// Ill-formatted strings coerce to null; unsafe.
Expand All @@ -3435,7 +3435,7 @@ public virtual bool CoercesTo(
DateTime.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
Date.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
Time.Accepts(this, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: usePowerFxV1CompatibilityRules) ||
(Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Number && OptionSetInfo.CanCoerceToBackingKind);
(Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Number && (OptionSetInfo.CanCoerceToBackingKind || !usePowerFxV1CompatibilityRules));
break;
case DKind.String:
doesCoerce = Kind != DKind.Color && Kind != DKind.Control && Kind != DKind.DataEntity && Kind != DKind.OptionSet && Kind != DKind.View && Kind != DKind.Polymorphic && Kind != DKind.File && Kind != DKind.LargeImage;
Expand Down Expand Up @@ -3518,7 +3518,7 @@ public virtual bool CoercesTo(

break;
case DKind.Color:
doesCoerce = Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Color && OptionSetInfo.CanCoerceToBackingKind;
doesCoerce = Kind == DKind.OptionSetValue && OptionSetInfo != null && OptionSetInfo.BackingKind == DKind.Color && (OptionSetInfo.CanCoerceToBackingKind || !usePowerFxV1CompatibilityRules);
break;
case DKind.Enum:
return CoercesTo(
Expand Down
17 changes: 16 additions & 1 deletion src/libraries/Microsoft.PowerFx.Interpreter/Functions/LibraryUnary.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.PowerFx.Core.Errors;
using Microsoft.PowerFx.Core.IR;
using Microsoft.PowerFx.Core.IR.Nodes;
using Microsoft.PowerFx.Core.Localization;
using Microsoft.PowerFx.Core.Utils;
using Microsoft.PowerFx.Interpreter;
using Microsoft.PowerFx.Interpreter.Exceptions;
using Microsoft.PowerFx.Types;
Expand Down Expand Up @@ -1070,7 +1072,20 @@ public static OptionSetValue NumberToOptionSet(IRContext irContext, NumberValue[

public static OptionSetValue BooleanToOptionSet(IRContext irContext, BooleanValue[] args)
{
return new OptionSetValue(irContext, "CalculatedOptionSetValue", (OptionSetValueType)irContext.ResultType, (bool)args[0].Value);
var optionSetInfo = ((OptionSetValueType)irContext.ResultType)._type.OptionSetInfo;

if (optionSetInfo != null && optionSetInfo.BackingKind == Core.Types.DKind.Boolean && optionSetInfo.OptionNames.Count() == 2)
{
foreach (var option in optionSetInfo.OptionNames)
{
if (optionSetInfo.TryGetValue(option, out var value) && (bool)value.ExecutionValue == (bool)args[0].Value)
{
return value;
}
}
}

throw CommonExceptions.RuntimeMisMatch;
}

private static System.Drawing.Color ToColor(double doubValue)
Expand Down
49 changes: 48 additions & 1 deletion ...s/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/StronglyTypedEnum_TestEnums.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,26 @@ Errors: Error 24-28: Invalid argument type (Enum (TestYeaNay)). Expecting a Enum
>> TestXORYesNo( TestYesNo.No, TestYesNo.Yes )
true

>> If( false, TestYeaNay.Yea, false )
TestYeaNay.Nay

>> Text( If( false, TestYeaNay.Yea, false ) )
"Nay"

// 10 is a part of the option set. In the future we may want to have this return .X instead.
>> If( false, TestNumberCompareNumericCoerceFrom.V, 10 )
TestNumberCompareNumericCoerceFrom.CalculatedOptionSetValue

>> Text( If( false, TestNumberCompareNumericCoerceFrom.V, 10 ) )
"CalculatedOptionSetValue"

// 11 is not a part of the option set.
>> If( false, TestNumberCompareNumericCoerceFrom.V, 11 )
TestNumberCompareNumericCoerceFrom.CalculatedOptionSetValue

>> Text( If( false, TestNumberCompareNumericCoerceFrom.V, 11 ) )
"CalculatedOptionSetValue"

//===========================================================================================================
//
// 11. CanCoerceToBackingKind - For example, ErrorKind that can be used as a number
Expand All @@ -284,9 +304,33 @@ true
>> Int( TestNumberCoerceTo.V )
5

>> Power( TestNumberCoerceTo.V, 2 )
25

>> TestSignNumber( TestNumberCoerceTo.V )
1

>> TestSignDecimal( TestNumberCoerceTo.V )
1

>> Power( TestNumberCoerceTo.V, TestNumberCoerceTo.V )
3125

>> Int( TestNumberCompareNumeric.V )
Errors: Error 0-3: The function 'Int' has some invalid arguments.|Error 29-31: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Decimal value instead.

>> Power( TestNumberCompareNumeric.V, 2 )
Errors: Error 0-5: The function 'Power' has some invalid arguments.|Error 31-33: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Number value instead.

>> TestSignNumber( TestNumberCompareNumeric.V )
Errors: Error 40-42: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Number value instead.|Error 0-14: The function 'TestSignNumber' has some invalid arguments.

>> TestSignDecimal( TestNumberCompareNumeric.V )
Errors: Error 41-43: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Decimal value instead.|Error 0-15: The function 'TestSignDecimal' has some invalid arguments.

>> Power( TestNumberCompareNumeric.V, TestNumberCompareNumeric.V )
Errors: Error 0-5: The function 'Power' has some invalid arguments.|Error 31-33: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Number value instead.|Error 59-61: Invalid argument type (Enum (TestNumberCompareNumeric)). Expecting a Number value instead.

// Unless there is a specific reason, CanCoerceToBackingKind is expected to be true for most Boolean option sets

>> TestYesNo.Yes && TestYeaNay.Yea
Expand Down Expand Up @@ -724,7 +768,10 @@ true
false

>> If( false, TestYesNo.Yes, false )
TestYesNo.CalculatedOptionSetValue
TestYesNo.No

>> Text( If( false, TestYesNo.Yes, false ) )
"No"

// Can be used as a Boolean (CanCoerceToBackingKind = true)

Expand Down
Loading

0 comments on commit caa821f

Please sign in to comment.