Covering some corner cases. Fixing a bug when removing the same record (value) multiple times.

anderson-joyle · anderson-joyle · commit 617a7fdb7917 · 2024-12-26T14:48:35.000-06:00
diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs
@@ -184,7 +184,7 @@ public override DValue<RecordValue> Last(bool mutationCopy = false)
         public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<FormulaValue> recordsToRemove, bool all, CancellationToken cancellationToken)
         {
             var ret = false;
-            var deleteList = new List<T>();
+            var markedToDeletion = new HashSet<T>();
             var errors = new List<ExpressionError>();
 
             cancellationToken.ThrowIfCancellationRequested();
@@ -206,9 +206,15 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula
 
                     if (await MatchesAsync(dRecord.Value, recordToRemove, cancellationToken).ConfigureAwait(false))
                     {
-                        found = true;
-
-                        deleteList.Add(item);
+                        if (markedToDeletion.Contains(item))
+                        {
+                            continue;
+                        }
+                        else
+                        {
+                            found = true;
+                            markedToDeletion.Add(item);
+                        }
 
                         if (!all)
                         {
@@ -224,7 +230,7 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula
                 }
             }
 
-            foreach (var delete in deleteList)
+            foreach (var delete in markedToDeletion)
             {
                 _sourceList.Remove(delete);
                 ret = true;
diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Remove.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Remove.cs
@@ -119,34 +119,16 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp
 
                 if (!argType.IsRecord)
                 {
-                    if (argCount >= 3 && i == argCount - 1)
+                    if (argCount >= 3 && i == argCount - 1 &&
+                        ((context.Features.PowerFxV1CompatibilityRules && BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) ||
+                        (!context.Features.PowerFxV1CompatibilityRules && DType.String.Accepts(argType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))))
                     {
-                        if (context.AnalysisMode)
-                        {
-                            if (!DType.String.Accepts(argType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules) &&
-                                !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))
-                            {
-                                fValid = false;
-                                errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrRemoveAllArg);
-                            }
-                        }
-                        else
-                        {
-                            if (!BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))
-                            {
-                                fValid = false;
-                                errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrRemoveAllArg);
-                            }
-                        }
-
-                        continue;
-                    }
-                    else
-                    {
-                        fValid = false;
-                        errors.EnsureError(args[i], TexlStrings.ErrNeedRecord_Arg, args[i]);
                         continue;
                     }
+
+                    fValid = false;
+                    errors.EnsureError(args[i], TexlStrings.ErrNeedRecord_Arg, args[i]);
+                    continue;
                 }
 
                 var collectionAcceptsRecord = collectionType.Accepts(argType.ToTable(), exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules);
@@ -351,7 +333,7 @@ public override bool ArgMatchesDatasourceType(int argNum)
         }
 
         public RemoveAllFunction()
-            : base("Remove", TexlStrings.AboutRemove, FunctionCategories.Behavior, DType.EmptyTable, 0, 2, 3, DType.EmptyTable, DType.EmptyTable, DType.String)
+            : base("Remove", TexlStrings.AboutRemove, FunctionCategories.Behavior, DType.EmptyTable, 0, 2, 3, DType.EmptyTable, DType.EmptyTable)
         {
         }
 
@@ -425,25 +407,12 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp
                 }
             }
 
-            if (args.Length == 3)
+            if (args.Length == 3 && 
+                ((context.Features.PowerFxV1CompatibilityRules && !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) || 
+                (!context.Features.PowerFxV1CompatibilityRules && !DType.String.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))))
             {
-                if (context.AnalysisMode)
-                {
-                    if (!DType.String.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules) &&
-                        !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))
-                    {
-                        fValid = false;
-                        errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg);
-                    }
-                }
-                else
-                {
-                    if (!BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules))
-                    {
-                        fValid = false;
-                        errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg);
-                    }
-                }
+                fValid = false;
+                errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg);
             }
 
             returnType = context.Features.PowerFxV1CompatibilityRules ? DType.Void : collectionType;
diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs b/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs
@@ -78,7 +78,7 @@ public static async Task<FormulaValue> RemoveCore(FormulaType irContext, Formula
 
                 if (arg is TableValue tableValue)
                 {
-                    var errorRecord = tableValue.Rows.First(row => row.IsError);
+                    var errorRecord = tableValue.Rows.FirstOrDefault(row => row.IsError);
                     if (errorRecord != null)
                     {
                         return errorRecord.Error;
diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt
@@ -4,13 +4,13 @@
 
 // Wrong arguments
 >> Remove(t1, r1,"Al");
-Errors: Error 14-18: If provided, last argument must be 'RemoveFlags.All'. Is there a typo?|Error 0-6: The function 'Remove' has some invalid arguments.
+Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 14-18: Cannot use a non-record value in this context: '"Al"'.
 
 >> Remove(t1, r1,"");
-Errors: Error 14-16: If provided, last argument must be 'RemoveFlags.All'. Is there a typo?|Error 0-6: The function 'Remove' has some invalid arguments.
+Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 14-16: Cannot use a non-record value in this context: '""'.
 
 >> Remove(t1, r1, r1, r1, r1, r1, r1, "Al");
-Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 35-39: If provided, last argument must be 'RemoveFlags.All'. Is there a typo?
+Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 35-39: Cannot use a non-record value in this context: '"Al"'.
 
 >> Remove(t1, "All");
 Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 11-16: Cannot use a non-record value in this context: '"All"'.
diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt
@@ -48,24 +48,58 @@ Errors: Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Err
 >> 4;t1
 Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
 
+>> Set(t3, t1)
+Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+// Removing multiple rows with the same values.
+>> Remove(t3, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)})
+If(true, {test:1}, "Void value (result of the expression can't be used).")
+
+>> 0;t3
+Table({a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+>> 0;Set(t3, t1)
+Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+>> Remove(t3, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, RemoveFlags.All)
+Error(Table({Kind:ErrorKind.NotFound},{Kind:ErrorKind.NotFound}))
+
+>> 1;t3
+Table({a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+>> 1;Set(t3, t1)
+Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+>> Remove(t3, t3)
+If(true, {test:1}, "Void value (result of the expression can't be used).")
+
+>> 2;t3
+Table()
+
+>> 2;Set(t3, t1)
+Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)})
+
+>> Remove(t3, t3, RemoveFlags.All)
+Error(Table({Kind:ErrorKind.NotFound},{Kind:ErrorKind.NotFound}))
+
 // Remove propagates error.
 >> Remove(t1, If(1/0<2, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}))
 Error({Kind:ErrorKind.Div0})
 
->> Set(t3, Table({a:{aa:{aaa:true,bbb:true}}}))
+>> Set(t4, Table({a:{aa:{aaa:true,bbb:true}}}))
 Table({a:{aa:{aaa:true,bbb:true}}})
 
->> Remove(t3, {a:{aa:{aaa:true}}})
+>> Remove(t4, {a:{aa:{aaa:true}}})
 Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 11-30: Missing column. Your formula is missing a column 'a.aa.bbb' with a type of 'Boolean'.|Error 11-30: Invalid argument type. Expecting a Record value, but of a different schema.|Error 11-30: Missing column. Your formula is missing a column 'aa.bbb' with a type of 'Boolean'.
 
->> Remove(t3, {a:{aa:{aaa:true,bbb:false}}})
+>> Remove(t4, {a:{aa:{aaa:true,bbb:false}}})
 Error({Kind:ErrorKind.NotFound})
 
->> Remove(t3, {a:{aa:{aaa:true,bbb:false}}}, RemoveFlags.All)
+>> Remove(t4, {a:{aa:{aaa:true,bbb:false}}}, RemoveFlags.All)
 Error({Kind:ErrorKind.NotFound})
 
->> Remove(t3, {a:{aa:{aaa:true,bbb:true}}})
+>> Remove(t4, {a:{aa:{aaa:true,bbb:true}}})
 If(true, {test:1}, "Void value (result of the expression can't be used).")
 
->> t3
+>> t4
 Table()

Original file line number	Diff line number	Diff line change
`@@ -184,7 +184,7 @@ public override DValue<RecordValue> Last(bool mutationCopy = false)`
`184`	`184`	`public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<FormulaValue> recordsToRemove, bool all, CancellationToken cancellationToken)`
`185`	`185`	`{`
`186`	`186`	`var ret = false;`
`187`		`- var deleteList = new List<T>();`
	`187`	`+ var markedToDeletion = new HashSet<T>();`
`188`	`188`	`var errors = new List<ExpressionError>();`
`189`	`189`
`190`	`190`	`cancellationToken.ThrowIfCancellationRequested();`
`@@ -206,9 +206,15 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula`
`206`	`206`
`207`	`207`	`if (await MatchesAsync(dRecord.Value, recordToRemove, cancellationToken).ConfigureAwait(false))`
`208`	`208`	`{`
`209`		`- found = true;`
`210`		`-`
`211`		`- deleteList.Add(item);`
	`209`	`+ if (markedToDeletion.Contains(item))`
	`210`	`+ {`
	`211`	`+ continue;`
	`212`	`+ }`
	`213`	`+ else`
	`214`	`+ {`
	`215`	`+ found = true;`
	`216`	`+ markedToDeletion.Add(item);`
	`217`	`+ }`
`212`	`218`
`213`	`219`	`if (!all)`
`214`	`220`	`{`
`@@ -224,7 +230,7 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula`
`224`	`230`	`}`
`225`	`231`	`}`
`226`	`232`
`227`		`- foreach (var delete in deleteList)`
	`233`	`+ foreach (var delete in markedToDeletion)`
`228`	`234`	`{`
`229`	`235`	`_sourceList.Remove(delete);`
`230`	`236`	`ret = true;`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ public static async Task<FormulaValue> RemoveCore(FormulaType irContext, Formula`
`78`	`78`
`79`	`79`	`if (arg is TableValue tableValue)`
`80`	`80`	`{`
`81`		`- var errorRecord = tableValue.Rows.First(row => row.IsError);`
	`81`	`+ var errorRecord = tableValue.Rows.FirstOrDefault(row => row.IsError);`
`82`	`82`	`if (errorRecord != null)`
`83`	`83`	`{`
`84`	`84`	`return errorRecord.Error;`