diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index ae4ef850a2..6850c202ef 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -3862,6 +3862,12 @@ public override void PostVisit(BinaryOpNode node) var leftType = _txb.GetType(node.Left); var rightType = _txb.GetType(node.Right); + // Helps warn no op comparison, e.g. Filter(table, ThisRecord.Value = Value)) or Filter(table, Value = Value) + if (IsNoOPFirstNameComparison(node)) + { + _txb.ErrorContainer.EnsureError(DocumentErrorSeverity.Warning, node, TexlStrings.WrnNoOpFieldComparison); + } + var res = CheckBinaryOpCore(_txb.ErrorContainer, node, _txb.Features, leftType, rightType, _txb.BindingConfig.NumberIsFloat); foreach (var coercion in res.Coercions) @@ -3882,6 +3888,48 @@ public override void PostVisit(BinaryOpNode node) _txb.SetIsUnliftable(node, _txb.IsUnliftable(node.Left) || _txb.IsUnliftable(node.Right)); } + private static bool IsNoOPFirstNameComparison(BinaryOpNode node) + { + if ((node.Op == BinaryOp.Equal || + node.Op == BinaryOp.NotEqual || + node.Op == BinaryOp.Less || + node.Op == BinaryOp.LessEqual || + node.Op == BinaryOp.Greater || + node.Op == BinaryOp.GreaterEqual) && + IsNoOPFirstNameComparison(node.Left, node.Right)) + { + return true; + } + + return false; + } + + private static bool IsNoOPFirstNameComparison(TexlNode left, TexlNode right) + { + if (left is FirstNameNode lfn1 && right is FirstNameNode rfn1) + { + // This means a field comparison e.g. Value = Value + return lfn1.Ident.Name == rfn1.Ident.Name; + } + else if (left is FirstNameNode leftFirstName && right is DottedNameNode rightDn && rightDn.Left is FirstNameNode rightfn) + { + // This means a field comparison with ThisRecord, where field is on left hand side. e.g. Value = ThisRecord.Value + return leftFirstName.Ident.Name == rightDn.Right.Name && rightfn.Ident.Name == ThisRecordDefaultName; + } + else if (right is FirstNameNode rightFirstName && left is DottedNameNode leftDn && leftDn.Left is FirstNameNode leftfn) + { + // This means a field comparison with ThisRecord, where field is on right hand side. e.g. ThisRecord.Value = Value + return rightFirstName.Ident.Name == leftDn.Right.Name && leftfn.Ident.Name == ThisRecordDefaultName; + } + else if (left is DottedNameNode leftDottedName && right is DottedNameNode rightDottedName) + { + // This means field is a part of dotted name node and we need to compare the field names and recursively check for the left and right nodes. + return leftDottedName.Right.Name == rightDottedName.Right.Name && IsNoOPFirstNameComparison(leftDottedName.Left, rightDottedName.Left); + } + + return false; + } + public override void PostVisit(AsNode node) { Contracts.AssertValue(node); diff --git a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs index 8aa76c5474..ab51b33709 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs @@ -802,5 +802,6 @@ internal static class TexlStrings public static ErrorResourceKey ErrUnknownPartialOp = new ErrorResourceKey("ErrUnknownPartialOp"); public static ErrorResourceKey ErrTruncatedArgWarning = new ErrorResourceKey("ErrTruncatedArgWarning"); + public static ErrorResourceKey WrnNoOpFieldComparison = new ErrorResourceKey("WrnNoOpFieldComparison"); } } diff --git a/src/strings/PowerFxResources.en-US.resx b/src/strings/PowerFxResources.en-US.resx index 04e243ce8a..d565285a7f 100644 --- a/src/strings/PowerFxResources.en-US.resx +++ b/src/strings/PowerFxResources.en-US.resx @@ -4575,4 +4575,8 @@ Delegation warning. The result of this argument '{0}' may be truncated for large data sets before being passed to the '{1}' function. Error message when an argument to non-delegable function has possible delegation and resulting rows may be truncated + + This comparison will always be constant. (Always yields true or false). Please consider using 'As' keyword for aliasing scoped variable if this not what you want. + Warning when comparing same first name node. + \ No newline at end of file diff --git a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/FilterFunctions.txt b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/FilterFunctions.txt index 8cab15d060..438e3beb86 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/FilterFunctions.txt +++ b/src/tests/Microsoft.PowerFx.Core.Tests/ExpressionTestCases/FilterFunctions.txt @@ -66,3 +66,12 @@ Table(Blank()) >> Filter(Table({a:1},{a:2},{a:4},{a:5}), IsBlank(ThisRecord)) Table() + +>> Filter(Table({F1: {N1F1: {N2F1 : 1} } } ), ThisRecord.F1.N1F1.N2F1 = F1.N1F1.N2F1) +Table({F1:{N1F1:{N2F1:1}}}) + +>> Filter(Table({F1: {N1F1: {N2F1 : 1} } } ), F1.N1F1.N2F1 = ThisRecord.F1.N1F1.N2F1) +Table({F1:{N1F1:{N2F1:1}}}) + +>> Filter(Table({F1: {N1F1: {N2F1 : 1} } } ), F1.N1F1.N2F1 = F1.N1F1.N2F1) +Table({F1:{N1F1:{N2F1:1}}}) \ No newline at end of file diff --git a/src/tests/Microsoft.PowerFx.Core.Tests/TexlTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests/TexlTests.cs index 92e9e3fe70..babde6b0df 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests/TexlTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests/TexlTests.cs @@ -2221,12 +2221,12 @@ public void TestTexlFunctionGetSignaturesApi() [Fact] public void TestWarningsOnEqualWithIncompatibleTypes() { - TestBindingWarning("1 = \"hello\"", DType.Boolean, expectedErrorCount: 1); - TestBindingWarning("1 <> \"hello\"", DType.Boolean, expectedErrorCount: 1); - TestBindingWarning("true = 123", DType.Boolean, expectedErrorCount: 1); - TestBindingWarning("true <> 123", DType.Boolean, expectedErrorCount: 1); - TestBindingWarning("false = \"false\"", DType.Boolean, expectedErrorCount: 1); - TestBindingWarning("false <> \"false\"", DType.Boolean, expectedErrorCount: 1); + TestBindingWarning("1 = \"hello\"", expectedErrorCount: 1, expectedType: DType.Boolean); + TestBindingWarning("1 <> \"hello\"", expectedErrorCount: 1, expectedType: DType.Boolean); + TestBindingWarning("true = 123", expectedErrorCount: 1, expectedType: DType.Boolean); + TestBindingWarning("true <> 123", expectedErrorCount: 1, expectedType: DType.Boolean); + TestBindingWarning("false = \"false\"", expectedErrorCount: 1, expectedType: DType.Boolean); + TestBindingWarning("false <> \"false\"", expectedErrorCount: 1, expectedType: DType.Boolean); } [Fact] @@ -3024,8 +3024,8 @@ public void TestWarningOnLiteralPredicate(string script, string expectedType) symbol.AddVariable("TW", new TableType(TestUtils.DT("*[Item:w]"))); TestBindingWarning( script, - TestUtils.DT(expectedType), expectedErrorCount: null, + expectedType: TestUtils.DT(expectedType), symbolTable: symbol); } @@ -3043,8 +3043,8 @@ public void TestWarningOnLiteralPredicate_NumberIsFloat(string script, string ex symbol.AddVariable("TW", new TableType(TestUtils.DT("*[Item:w]"))); TestBindingWarning( script, - TestUtils.DT(expectedType), - expectedErrorCount: null, + expectedErrorCount: null, + expectedType: TestUtils.DT(expectedType), symbolTable: symbol, numberIsFloat: true); } @@ -4196,12 +4196,46 @@ public void TexlFunctionTypeSemanticsTable_Delegation(string script, string expe TestBindingWarning( script, - TestUtils.DT(expectedSchema), errorCount, + expectedType: TestUtils.DT(expectedSchema), symbol, features: Features.PowerFxV1); } + [Theory] + + [InlineData("logicalNum = logicalNum")] + [InlineData("ThisRecord.logicalNum = logicalNum")] + [InlineData("logicalNum = ThisRecord.logicalNum")] + [InlineData("ThisRecord.logicalNum = ThisRecord.logicalNum")] + + [InlineData("DisplayNum = DisplayNum")] + [InlineData("ThisRecord.DisplayNum = DisplayNum")] + [InlineData("DisplayNum = ThisRecord.DisplayNum")] + [InlineData("ThisRecord.DisplayNum = ThisRecord.DisplayNum")] + + [InlineData("Filter(tableVar, f1 = f1)")] + [InlineData("Filter(tableVar, ThisRecord.f1 = f1)")] + [InlineData("Filter(tableVar, f1 = ThisRecord.f1)")] + [InlineData("Filter(tableVar, ThisRecord.f1 = ThisRecord.f1)")] + + [InlineData("Filter(tableVar, F1 = F1)")] + [InlineData("Filter(tableVar, ThisRecord.F1 = F1)")] + [InlineData("Filter(tableVar, F1 = ThisRecord.F1)")] + [InlineData("Filter(tableVar, ThisRecord.F1 = ThisRecord.F1)")] + public void SameFirstNameNodeComparisonWarning(string script) + { + var tableVarType = RecordType.Empty().Add("f1", FormulaType.String, "F1").ToTable(); + var symbolRecord = RecordType.Empty().Add("logicalNum", FormulaType.Number, "DisplayNum").Add("tableVar", tableVarType, "TableVar"); + var symbol = new SymbolTableOverRecordType(symbolRecord, allowThisRecord: true); + + TestBindingWarning( + script, + expectedErrorCount: 1, + symbolTable: symbol, + features: Features.PowerFxV1); + } + private void TestBindingPurity(string script, bool isPure, SymbolTable symbolTable = null) { var config = new PowerFxConfig @@ -4216,7 +4250,7 @@ private void TestBindingPurity(string script, bool isPure, SymbolTable symbolTab Assert.Equal(isPure, result.Binding.IsPure(result.Parse.Root)); } - private void TestBindingWarning(string script, DType expectedType, int? expectedErrorCount, SymbolTable symbolTable = null, bool numberIsFloat = false, Features features = null) + private void TestBindingWarning(string script, int? expectedErrorCount, DType expectedType = null, ReadOnlySymbolTable symbolTable = null, bool numberIsFloat = false, Features features = null) { var config = features != null ? new PowerFxConfig(features) : new PowerFxConfig(Features.None); var parserOptions = new ParserOptions() @@ -4225,9 +4259,13 @@ private void TestBindingWarning(string script, DType expectedType, int? expected }; var engine = new Engine(config); - var result = engine.Check(script, parserOptions, symbolTable); - - Assert.Equal(expectedType, result.Binding.ResultType); + var result = engine.Check(script, parserOptions, symbolTable); + + if (expectedType != null) + { + Assert.Equal(expectedType, result.Binding.ResultType); + } + Assert.True(result.Binding.ErrorContainer.HasErrors()); if (expectedErrorCount != null) { diff --git a/src/tests/Microsoft.PowerFx.Performance.Tests/PvaPerformance.cs b/src/tests/Microsoft.PowerFx.Performance.Tests/PvaPerformance.cs index 0207d65052..230138369d 100644 --- a/src/tests/Microsoft.PowerFx.Performance.Tests/PvaPerformance.cs +++ b/src/tests/Microsoft.PowerFx.Performance.Tests/PvaPerformance.cs @@ -128,7 +128,13 @@ private static string GetRandomExpression() { int j = rnd.Next(0, 10000); // [0 - 9999] int k = rnd.Next(0, 10); // [0 -9] - int l = rnd.Next(0, 10); // [0 -9] + + int l; + do + { + l = rnd.Next(0, 10); // [0 - 9] not same as k. + } + while (l == k); expr.Append($"(OptionSet{j:0000}.Logical{k} "); expr.Append(rnd.Next() > (1 << 30) ? "=" : "<>");