From 98eb5f2b031a8f00af67117e6ceb2797953c4516 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Wed, 12 Feb 2025 15:20:07 -0600 Subject: [PATCH 01/13] WIP --- .../Functions/FunctionScopeInfo.cs | 18 ++- .../Functions/TexlFunction.cs | 2 - .../Syntax/Visitors/ScopePredicateVisitor.cs | 130 ++++++++++++++++++ .../Texl/Builtins/StatisticalTableFunction.cs | 2 - .../IntellisenseTests/SuggestTest.cs | 26 ++++ 5 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index 2a6f6fac84..15ae0aa226 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -6,6 +6,7 @@ using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Localization; +using Microsoft.PowerFx.Core.Syntax.Visitors; using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Utils; using Microsoft.PowerFx.Syntax; @@ -192,9 +193,14 @@ public virtual bool CheckInput(Features features, TexlNode inputNode, DType inpu public virtual bool CheckInput(Features features, CallNode callNode, TexlNode inputNode, DType inputSchema, out DType typeScope) { return CheckInput(features, inputNode, inputSchema, TexlFunction.DefaultErrorContainer, out typeScope); + } + + protected virtual void CheckPredicates(TexlNode[] inputNodes, DType typeScope, bool wholeScope, IErrorContainer errors = null) + { + CheckLiteralPredicates(inputNodes, errors); } - public void CheckLiteralPredicates(TexlNode[] args, IErrorContainer errors) + protected void CheckLiteralPredicates(TexlNode[] args, IErrorContainer errors) { Contracts.AssertValue(args); Contracts.AssertValue(errors); @@ -301,7 +307,7 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode[] typeScope = DType.EmptyRecord; - GetScopeIdent(inputNodes, out DName[] idents); + var wholeScope = GetScopeIdent(inputNodes, out DName[] idents); for (var i = 0; i < argCount; i++) { @@ -309,6 +315,8 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode[] typeScope = typeScope.Add(idents[i], type); } + CheckPredicates(inputNodes, typeScope, wholeScope); + return ret; } @@ -338,5 +346,11 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) // Meaning that the scope is a record type and we are accessing the fields directly. return false; } + + protected override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, bool wholeScope, IErrorContainer errors = null) + { + var visitor = new ScopePredicateVisitor(typeScope, wholeScope); + inputNodes[2].Accept(visitor); + } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs index e739d8dabf..477b26cc60 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs @@ -609,8 +609,6 @@ private bool CheckTypesCore(CheckTypesContext context, TexlNode[] args, DType[] nodeToCoercedTypeMap = null; } - ScopeInfo?.CheckLiteralPredicates(args, errors); - // Default return type. returnType = ReturnType; diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs new file mode 100644 index 0000000000..00c185bf20 --- /dev/null +++ b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs @@ -0,0 +1,130 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using System; +using Microsoft.PowerFx.Core.Functions; +using Microsoft.PowerFx.Core.Types; +using Microsoft.PowerFx.Core.Utils; +using Microsoft.PowerFx.Syntax; + +namespace Microsoft.PowerFx.Core.Syntax.Visitors +{ + internal class ScopePredicateVisitor : TexlVisitor + { + private readonly DType _typeScope; + private readonly bool _wholeScope; + + public ScopePredicateVisitor(DType typeScope, bool wholeScope) + { + _typeScope = typeScope; + _wholeScope = wholeScope; + } + + public override void Visit(TypeLiteralNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(ErrorNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(BlankNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(BoolLitNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(StrLitNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(NumLitNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(DecLitNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(FirstNameNode node) + { + if (_wholeScope) + { + throw new NotImplementedException(); + } + else + { + var test = _typeScope.TryGetType(node.Ident.Name, out _); + } + } + + public override void Visit(ParentNode node) + { + throw new NotImplementedException(); + } + + public override void Visit(SelfNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(StrInterpNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(DottedNameNode node) + { + return; + } + + public override void PostVisit(UnaryOpNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(BinaryOpNode node) + { + return; + } + + public override void PostVisit(VariadicOpNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(CallNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(ListNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(RecordNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(TableNode node) + { + throw new NotImplementedException(); + } + + public override void PostVisit(AsNode node) + { + throw new NotImplementedException(); + } + } +} diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs index 98a6397e78..1b36068f6e 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs @@ -116,8 +116,6 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp return false; } - ScopeInfo?.CheckLiteralPredicates(args, errors); - return true; } } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs index ac243c30c2..714f98d4ab 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs @@ -314,6 +314,32 @@ public void TestSuggestEscapedEnumName() Assert.Contains("'Name That.Requires!escaping'.Field2", result); } + [Theory] + [InlineData("in", "in_text")] + [InlineData("or", "or_text")] + [InlineData("and", "and_text")] + [InlineData("In", "In_text")] + [InlineData("Or", "Or_text")] + [InlineData("And", "And_text")] + public void TesafdghghffgnfrghhhhhhhhhhhhhhhhhhhhhhhhhhhhpedEnumName(string expr, string varName) + { + var engine = new Engine(); + engine.Config.SymbolTable.AddVariable(varName, FormulaType.String); + + var test = Features.PowerFxV1.GetType(); + var tese = test.GetProperty("SkipExpandableSetSemantics", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + + foreach (var textFirst in new[] { false, true }) + { + expr = textFirst ? "=" + expr : expr; + + var check = engine.Check(expr, options: new ParserOptions() { TextFirst = textFirst }); + var suggestions = engine.Suggest(check, expr.Length); + + Assert.Equal(textFirst ? 1 : 0, suggestions.ReplacementStartIndex); + } + } + [Theory] [InlineData("SortByColumns(|", 3, "The table to sort.", "SortByColumns(source, column, ...)")] [InlineData("SortByColumns(tbl1,|", 3, "A unique column name.", "SortByColumns(source, column, ...)")] From 2b5d8a019cbd561e5e423f5edd196e17080e3d84 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Mon, 24 Feb 2025 18:58:36 -0600 Subject: [PATCH 02/13] WIP --- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 6 +- .../Functions/FunctionScopeInfo.cs | 84 +++++++++++-------- .../Microsoft.PowerFx.Core/IR/IRTranslator.cs | 3 +- .../Localization/Strings.cs | 2 +- .../Syntax/Visitors/ScopePredicateVisitor.cs | 72 ++++++++++------ .../Texl/Builtins/Filter.cs | 2 +- .../Texl/Builtins/StatisticalTableFunction.cs | 2 +- .../ParsedExpression.cs | 2 +- src/strings/PowerFxResources.en-US.resx | 6 +- .../ExpressionErrorTests.cs | 37 ++++++++ .../Helpers/TestUtils.cs | 2 +- .../YamlTexlFunction.cs | 2 +- 12 files changed, 146 insertions(+), 74 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index 1a34766e6b..a036f12cc9 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -123,7 +123,7 @@ internal sealed partial class TexlBinding // This is set when a First Name node or child First Name node contains itself in its variable weight // and can be read by the back end to determine whether it may generate code that lifts or caches an // expression - private readonly BitArray _isUnliftable; + private readonly BitArray _isUnliftable; public bool HasLocalScopeReferences { get; private set; } @@ -4494,7 +4494,7 @@ public override bool PreVisit(CallNode node) // Determine the Scope Identifier using the func.ScopeArgs arg required = scopeInfo.GetScopeIdent(node.Args.Children.ToArray(), out scopeIdentifiers); - if (scopeInfo.CheckInput(_txb.Features, node, node.Args.Children.ToArray(), out scope, GetScopeArgsTypes(node.Args.Children, argsCount))) + if (scopeInfo.CheckInput(_txb.Features, node, node.Args.Children.ToArray(), out scope, _txb.ErrorContainer, GetScopeArgsTypes(node.Args.Children, argsCount))) { if (_txb.TryGetEntityInfo(node.Args.Children[0], out expandInfo)) { @@ -4593,7 +4593,7 @@ public override bool PreVisit(CallNode node) args[i].Accept(this); } - fArgsValid = scopeInfo.CheckInput(_txb.Features, node, args, out typeScope, GetScopeArgsTypes(node.Args.Children, maybeFunc.ScopeArgs)); + fArgsValid = scopeInfo.CheckInput(_txb.Features, node, args, out typeScope, _txb.ErrorContainer, GetScopeArgsTypes(node.Args.Children, maybeFunc.ScopeArgs)); // Determine the scope identifier using the first node for lambda params identRequired = scopeInfo.GetScopeIdent(args, out scopeIdent); diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index 15ae0aa226..3eea5f50ed 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. using System; +using System.Linq; using Microsoft.PowerFx.Core.App.ErrorContainers; using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Errors; @@ -40,7 +41,7 @@ internal class FunctionScopeInfo /// If false, the author will be warned when inputting predicates that /// do not reference the input table. /// - public bool AcceptsLiteralPredicates { get; } + public bool CheckPredicateUsage { get; } /// /// True indicates that the function performs some sort of iteration over @@ -73,7 +74,7 @@ public FunctionScopeInfo( TexlFunction function, bool usesAllFieldsInScope = true, bool supportsAsyncLambdas = true, - bool acceptsLiteralPredicates = true, + bool checkPredicateUsage = false, bool iteratesOverScope = true, DType scopeType = null, Func appliesToArgument = null, @@ -81,7 +82,7 @@ public FunctionScopeInfo( { UsesAllFieldsInScope = usesAllFieldsInScope; SupportsAsyncLambdas = supportsAsyncLambdas; - AcceptsLiteralPredicates = acceptsLiteralPredicates; + CheckPredicateUsage = checkPredicateUsage; IteratesOverScope = iteratesOverScope; ScopeType = scopeType; _function = function; @@ -96,11 +97,19 @@ public FunctionScopeInfo( /// Caller call node. /// ArgN node. /// Calculated DType type. + /// /// List of data sources to compose the calculated type. /// - public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) + public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, IErrorContainer errors, params DType[] inputSchema) { - return CheckInput(features, callNode, inputNodes[0], inputSchema[0], out typeScope); + var result = CheckInput(features, callNode, inputNodes[0], inputSchema[0], out typeScope); + + if (result && CheckPredicateUsage) + { + CheckPredicates(inputNodes, typeScope, errors); + } + + return result; } // Typecheck an input for this function, and get the cursor type for an invocation with that input. @@ -195,32 +204,25 @@ public virtual bool CheckInput(Features features, CallNode callNode, TexlNode in return CheckInput(features, inputNode, inputSchema, TexlFunction.DefaultErrorContainer, out typeScope); } - protected virtual void CheckPredicates(TexlNode[] inputNodes, DType typeScope, bool wholeScope, IErrorContainer errors = null) + public virtual void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors = null) { - CheckLiteralPredicates(inputNodes, errors); - } - - protected void CheckLiteralPredicates(TexlNode[] args, IErrorContainer errors) - { - Contracts.AssertValue(args); - Contracts.AssertValue(errors); - - if (!AcceptsLiteralPredicates) - { - for (var i = 0; i < args.Length; i++) - { - if (_function.IsLambdaParam(args[i], i)) - { - if (args[i].Kind == NodeKind.BoolLit || - args[i].Kind == NodeKind.NumLit || - args[i].Kind == NodeKind.DecLit || - args[i].Kind == NodeKind.StrLit) - { - errors.EnsureError(DocumentErrorSeverity.Warning, args[i], TexlStrings.WarnLiteralPredicate); - } - } - } - } + GetScopeIdent(inputNodes, out DName[] idents); + var visitor = new ScopePredicateVisitor(typeScope, idents); + + for (var i = 1; i < inputNodes.Length; i++) + { + var arg = inputNodes[i]; + + if (_function.IsLambdaParam(inputNodes[i], i)) + { + arg.Accept(visitor); + } + } + + if (visitor.InUsePredicates.Count == 0) + { + errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[0], TexlStrings.WarnCheckPredicateUsage, idents.First()); + } } /// @@ -296,18 +298,18 @@ internal class FunctionJoinScopeInfo : FunctionScopeInfo public static DName RightRecord => new DName("RightRecord"); public FunctionJoinScopeInfo(TexlFunction function) - : base(function, appliesToArgument: (argIndex) => argIndex > 1) + : base(function, appliesToArgument: (argIndex) => argIndex > 1, checkPredicateUsage: true) { } - public override bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) + public override bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, IErrorContainer errors, params DType[] inputSchema) { var ret = true; var argCount = Math.Min(inputNodes.Length, 2); typeScope = DType.EmptyRecord; - var wholeScope = GetScopeIdent(inputNodes, out DName[] idents); + GetScopeIdent(inputNodes, out DName[] idents); for (var i = 0; i < argCount; i++) { @@ -315,7 +317,10 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode[] typeScope = typeScope.Add(idents[i], type); } - CheckPredicates(inputNodes, typeScope, wholeScope); + if (ret) + { + CheckPredicates(inputNodes, typeScope, errors); + } return ret; } @@ -347,10 +352,17 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) return false; } - protected override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, bool wholeScope, IErrorContainer errors = null) + public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors) { - var visitor = new ScopePredicateVisitor(typeScope, wholeScope); + var wholeScope = GetScopeIdent(inputNodes, out DName[] idents); + var visitor = new ScopePredicateVisitor(typeScope, idents); + inputNodes[2].Accept(visitor); + + if (!visitor.InUsePredicates.Contains(idents[0]) || !visitor.InUsePredicates.Contains(idents[1])) + { + errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[2], TexlStrings.WarnCheckPredicateUsage, string.Join("/", idents.Select(id => id.Value))); + } } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs b/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs index 13d7117790..16cade3de2 100644 --- a/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs +++ b/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs @@ -616,7 +616,8 @@ private static IntermediateNode ConcatenateArgs(IntermediateNode arg1, Intermedi } } - var concatenatedNode = new CallNode(irContext, BuiltinFunctionsCore.Concatenate, concatenateArgs); + var concatenatedNode = new CallNode(irContext, BuiltinFunctionsCore.Concatenate, concatenateArgs); + throw new Exception("CHECKPOINT!"); return concatenatedNode; } diff --git a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs index d85e067f2d..ed92b0c32d 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Localization/Strings.cs @@ -804,7 +804,7 @@ internal static class TexlStrings public static ErrorResourceKey ErrAsNotInContext = new ErrorResourceKey("ErrAsNotInContext"); public static ErrorResourceKey ErrValueMustBeFullyQualified = new ErrorResourceKey("ErrValueMustBeFullyQualified"); public static ErrorResourceKey WarnColumnNameSpecifiedMultipleTimes_Name = new ErrorResourceKey("WarnColumnNameSpecifiedMultipleTimes_Name"); - public static ErrorResourceKey WarnLiteralPredicate = new ErrorResourceKey("WarnLiteralPredicate"); + public static ErrorResourceKey WarnCheckPredicateUsage = new ErrorResourceKey("WarnCheckPredicateUsage"); public static ErrorResourceKey WarnDynamicMetadata = new ErrorResourceKey("WarnDynamicMetadata"); public static ErrorResourceKey WarnDeferredType = new ErrorResourceKey("WarnDeferredType"); public static ErrorResourceKey ErrColRenamedTwice_Name = new ErrorResourceKey("ErrColRenamedTwice_Name"); diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs index 00c185bf20..2cfcee5185 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs @@ -2,6 +2,8 @@ // Licensed under the MIT license. using System; +using System.Collections.Generic; +using System.Linq; using Microsoft.PowerFx.Core.Functions; using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Utils; @@ -12,74 +14,89 @@ namespace Microsoft.PowerFx.Core.Syntax.Visitors internal class ScopePredicateVisitor : TexlVisitor { private readonly DType _typeScope; - private readonly bool _wholeScope; + private readonly DName[] _idents; - public ScopePredicateVisitor(DType typeScope, bool wholeScope) + public HashSet InUsePredicates; + + public ScopePredicateVisitor(DType typeScope, DName[] indents) { _typeScope = typeScope; - _wholeScope = wholeScope; + + InUsePredicates = new HashSet(); + _idents = indents; } public override void Visit(TypeLiteralNode node) { - throw new NotImplementedException(); + return; } public override void Visit(ErrorNode node) { - throw new NotImplementedException(); + return; } public override void Visit(BlankNode node) { - throw new NotImplementedException(); + return; } public override void Visit(BoolLitNode node) { - throw new NotImplementedException(); + return; } public override void Visit(StrLitNode node) { - throw new NotImplementedException(); + return; } public override void Visit(NumLitNode node) { - throw new NotImplementedException(); + return; } public override void Visit(DecLitNode node) { - throw new NotImplementedException(); + return; } public override void Visit(FirstNameNode node) { - if (_wholeScope) + // We should reach this block only if we are working with an implicity 'ThisRecord'. + if (_idents.Length == 1 && _typeScope.TryGetType(node.Ident.Name, out _)) { - throw new NotImplementedException(); + InUsePredicates.Add(_idents.First()); } - else + } + + // We can assume if visiting a DottedNameNode is always a whole scope. + public override bool PreVisit(DottedNameNode node) + { + var left = node.Left.AsFirstName(); + + if (_idents.Contains(left.Ident.Name)) { - var test = _typeScope.TryGetType(node.Ident.Name, out _); - } + InUsePredicates.Add(left.Ident.Name); + } + + // No need to visit left/right separately. + return false; } public override void Visit(ParentNode node) { - throw new NotImplementedException(); + return; } public override void Visit(SelfNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(StrInterpNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(DottedNameNode node) @@ -89,7 +106,7 @@ public override void PostVisit(DottedNameNode node) public override void PostVisit(UnaryOpNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(BinaryOpNode node) @@ -99,32 +116,37 @@ public override void PostVisit(BinaryOpNode node) public override void PostVisit(VariadicOpNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(CallNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(ListNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(RecordNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(TableNode node) { - throw new NotImplementedException(); + return; } public override void PostVisit(AsNode node) { - throw new NotImplementedException(); + return; + } + + public override bool PreVisit(CallNode node) + { + return false; } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs index 09fc51746e..098d36394f 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs @@ -27,7 +27,7 @@ internal sealed class FilterFunction : FilterFunctionBase public FilterFunction() : base("Filter", TexlStrings.AboutFilter, FunctionCategories.Table, DType.EmptyTable, -2, 2, int.MaxValue, DType.EmptyTable) { - ScopeInfo = new FunctionScopeInfo(this, acceptsLiteralPredicates: false); + ScopeInfo = new FunctionScopeInfo(this, checkPredicateUsage: true); } public override IEnumerable GetSignatures() diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs index 1b36068f6e..6eecdb2124 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/StatisticalTableFunction.cs @@ -27,7 +27,7 @@ internal abstract class StatisticalTableFunction : FunctionWithTableInput public StatisticalTableFunction(string name, TexlStrings.StringGetter description, FunctionCategories fc, bool nativeDecimal = false) : base(name, description, fc, DType.Number, 0x02, 2, 2, DType.EmptyTable, DType.Number) { - ScopeInfo = new FunctionScopeInfo(this, usesAllFieldsInScope: false, acceptsLiteralPredicates: false); + ScopeInfo = new FunctionScopeInfo(this, usesAllFieldsInScope: false, checkPredicateUsage: true); _nativeDecimal = nativeDecimal; } diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs b/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs index ce8ad19063..1deb58b0e0 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs @@ -64,7 +64,7 @@ public static async Task EvalAsync(this IExpressionEvaluator expr, /// public static IExpressionEvaluator GetEvaluator(this CheckResult result) { - var stackMarker = new StackDepthCounter(PowerFxConfig.DefaultMaxCallDepth); + var stackMarker = new StackDepthCounter(result.Engine.Config.MaxCallDepth); return GetEvaluator(result, stackMarker); } diff --git a/src/strings/PowerFxResources.en-US.resx b/src/strings/PowerFxResources.en-US.resx index d31cbffae9..46944f1ff5 100644 --- a/src/strings/PowerFxResources.en-US.resx +++ b/src/strings/PowerFxResources.en-US.resx @@ -2439,9 +2439,9 @@ A column named '{0}' was specified more than once. Duplicate columns. - - Warning: This predicate is a literal value and does not reference the input table. - Warning given when a literal predicate is given to a function operating over a table. + + Warning: The expression does not access '{0}' directly and can return unexpected results. + Warning given when an expression with a function operating over a table does not access the predicate directly. Warning: Select "Capture Schema" at the bottom of the expanded formula bar to set and refresh this method's result schema. Otherwise this method will return no result. diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs index 8b6cb2ceed..240ac0ac79 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs @@ -7,7 +7,10 @@ using System.Linq; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Localization; +using Microsoft.PowerFx.Core.Texl.Builtins; +using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Syntax; +using Microsoft.PowerFx.Types; using Xunit; namespace Microsoft.PowerFx.Core.Tests @@ -47,6 +50,40 @@ public void TestWarning() Assert.Equal("Warning 2-5: ouch", error.ToString()); } + [Theory] + [InlineData("Join(t1 As L, t1 As R, L.a = R.a, JoinType.Inner, R.a As AAA)")] + [InlineData("Filter(t1, a > 1)")] + [InlineData("Filter(t1, ThisRecord.a > 1)")] + [InlineData("Filter(t1 As T, T.a > 1)")] + [InlineData("Filter(t1 As T, 1 < T.a)")] + [InlineData("Filter(t1 As T, With({x:1}, x) > T.a)")] + + [InlineData("Filter(t1 As T, With({x:1}, x))", true)] + [InlineData("Filter(t1 As T, true)", true)] + [InlineData("Join(t1 As L, t1 As R, true, JoinType.Inner, R.a As AAA)", true)] + [InlineData("Join(t1 As L, t1 As R, With({L:R, R:R}, L.a = R.a), JoinType.Inner, R.a As RA, L.a As LA)", true)] + public void CheckPredicateUsageTest(string expression, bool raiseWarning = false) + { + var engine = new Engine(); + var symbols = new SymbolTable(); + + DType.TryParse("*[a:w,b:w,c:w]", out var tableType); + + symbols.AddFunction(new JoinFunction()); + symbols.AddVariable("t1", FormulaType.Build(tableType)); + + var check = engine.Check(expression, symbolTable: symbols); + + if (raiseWarning) + { + Assert.Contains(check.Errors, d => d.IsWarning); + } + else + { + Assert.DoesNotContain(check.Errors, d => d.IsWarning); + } + } + [Fact] public void NullSpanToString() { diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/Helpers/TestUtils.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/Helpers/TestUtils.cs index 3f30ef8ea8..d0e5d63aa9 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/Helpers/TestUtils.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/Helpers/TestUtils.cs @@ -284,7 +284,7 @@ public MockSilentDelegableFilterFunction(string name, string runtimeFunctionName : base(name, (l) => "MockFunction", FunctionCategories.Table, DType.EmptyTable, -2, 2, int.MaxValue, DType.EmptyTable) { _runtimeFunctionNameSuffix = runtimeFunctionNameSuffix; - ScopeInfo = new FunctionScopeInfo(this, acceptsLiteralPredicates: false); + ScopeInfo = new FunctionScopeInfo(this, checkPredicateUsage: false); } public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding) diff --git a/src/tests/Microsoft.PowerFx.TexlFunctionExporter.Shared/YamlTexlFunction.cs b/src/tests/Microsoft.PowerFx.TexlFunctionExporter.Shared/YamlTexlFunction.cs index 14a5c547ff..1fb4162430 100644 --- a/src/tests/Microsoft.PowerFx.TexlFunctionExporter.Shared/YamlTexlFunction.cs +++ b/src/tests/Microsoft.PowerFx.TexlFunctionExporter.Shared/YamlTexlFunction.cs @@ -491,7 +491,7 @@ public YamlTexlScopeInfo() internal YamlTexlScopeInfo(FunctionScopeInfo scopeInfo) { - AcceptsLiteralPredicates = scopeInfo.AcceptsLiteralPredicates; + AcceptsLiteralPredicates = scopeInfo.CheckPredicateUsage; CanBeCreatedByRecord = scopeInfo.CanBeCreatedByRecord; HasNondeterministicOperationOrder = scopeInfo.HasNondeterministicOperationOrder; IteratesOverScope = scopeInfo.IteratesOverScope; From 66a1f5dd09527b42f677a3bc80c92c6933c1d83d Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Mon, 10 Mar 2025 10:04:39 -0500 Subject: [PATCH 03/13] Check if predicate contains scope reference --- .../App/ErrorContainers/ErrorContainer.cs | 21 +++ .../Microsoft.PowerFx.Core/Binding/Binder.cs | 2 +- .../Functions/FunctionScopeInfo.cs | 25 +++- .../Microsoft.PowerFx.Core/IR/IRTranslator.cs | 1 - .../Syntax/Visitors/ScopePredicateVisitor.cs | 130 +++--------------- .../Utils/CollectionUtils.cs | 13 +- .../RecalcEngine.cs | 9 +- .../TestDelegationValidation.cs | 6 +- .../IntellisenseTests/SuggestTest.cs | 26 ---- .../TexlTests.cs | 61 +++++++- 10 files changed, 134 insertions(+), 160 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs b/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs index e4a5ea8fe4..e7097187ab 100644 --- a/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs +++ b/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs @@ -37,6 +37,16 @@ public bool HasErrors() return CollectionUtils.Size(_errors) > 0; } + public bool HasErrors(DocumentErrorSeverity severity) + { + if (_errors == null) + { + return false; + } + + return CollectionUtils.Size(_errors.Where(err => err.Severity >= severity)) > 0; + } + public bool HasErrors(TexlNode node, DocumentErrorSeverity severity = DocumentErrorSeverity.Suggestion) { Contracts.AssertValue(node); @@ -124,6 +134,17 @@ public IEnumerable GetErrors() } } + public IEnumerable GetErrors(DocumentErrorSeverity severity) + { + if (_errors != null) + { + foreach (var err in _errors.Where(err => err.Severity >= severity)) + { + yield return err; + } + } + } + public TexlError EnsureError(TexlNode node, ErrorResourceKey errKey, params object[] args) { return EnsureError(DefaultSeverity, node, errKey, args); diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index a036f12cc9..df1b0e1f83 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -5326,7 +5326,7 @@ private void PreVisitBottomUp(CallNode node, int argCountVisited, Scope scopeNew // If PreVisit resulted in errors for the node (and a non-null CallInfo), // we're done -- we have a match and appropriate errors logged already. - if (_txb.ErrorContainer.HasErrors(node) || _txb.ErrorContainer.HasErrors(node.Head.Token)) + if (_txb.ErrorContainer.HasErrors(node, DocumentErrorSeverity.Moderate) || _txb.ErrorContainer.HasErrors(node.Head.Token)) { Contracts.Assert(info != null); diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index 3eea5f50ed..c440d5d72d 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -251,7 +251,7 @@ internal class FunctionThisGroupScopeInfo : FunctionScopeInfo public static DName ThisGroup => new DName("ThisGroup"); public FunctionThisGroupScopeInfo(TexlFunction function) - : base(function, appliesToArgument: (argIndex) => argIndex > 0) + : base(function, appliesToArgument: (argIndex) => argIndex > 0, checkPredicateUsage: true) { } @@ -289,6 +289,26 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode i return ret; } + + public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors) + { + GetScopeIdent(inputNodes, out DName[] idents); + + for (int i = 0; i < inputNodes.Length; i++) + { + if (_function.IsLambdaParam(inputNodes[i], i)) + { + // We create a new visitor object for each iteration since lambda parms can appear at any place in the expression. + var visitor = new ScopePredicateVisitor(typeScope, new[] { ThisGroup }, true); + inputNodes[i].Accept(visitor); + + if (!visitor.InUsePredicates.Contains(ThisGroup)) + { + errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[i], TexlStrings.WarnCheckPredicateUsage, string.Join("/", idents.Select(id => id.Value))); + } + } + } + } } internal class FunctionJoinScopeInfo : FunctionScopeInfo @@ -354,7 +374,8 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors) { - var wholeScope = GetScopeIdent(inputNodes, out DName[] idents); + GetScopeIdent(inputNodes, out DName[] idents); + var visitor = new ScopePredicateVisitor(typeScope, idents); inputNodes[2].Accept(visitor); diff --git a/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs b/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs index 16cade3de2..11925f02fc 100644 --- a/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs +++ b/src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs @@ -617,7 +617,6 @@ private static IntermediateNode ConcatenateArgs(IntermediateNode arg1, Intermedi } var concatenatedNode = new CallNode(irContext, BuiltinFunctionsCore.Concatenate, concatenateArgs); - throw new Exception("CHECKPOINT!"); return concatenatedNode; } diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs index 2cfcee5185..b8ce08db62 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs @@ -11,141 +11,51 @@ namespace Microsoft.PowerFx.Core.Syntax.Visitors { - internal class ScopePredicateVisitor : TexlVisitor + internal class ScopePredicateVisitor : IdentityTexlVisitor { private readonly DType _typeScope; private readonly DName[] _idents; + // The Summarize function creates a 'ThisGroup' indentifier that can only be used inside a call. + private bool _allowAggregateCall; + public HashSet InUsePredicates; - public ScopePredicateVisitor(DType typeScope, DName[] indents) + public ScopePredicateVisitor(DType typeScope, DName[] indents, bool allowAggregateCall = false) { _typeScope = typeScope; - - InUsePredicates = new HashSet(); _idents = indents; - } + _allowAggregateCall = allowAggregateCall; - public override void Visit(TypeLiteralNode node) - { - return; - } - - public override void Visit(ErrorNode node) - { - return; - } - - public override void Visit(BlankNode node) - { - return; - } - - public override void Visit(BoolLitNode node) - { - return; - } - - public override void Visit(StrLitNode node) - { - return; - } - - public override void Visit(NumLitNode node) - { - return; - } - - public override void Visit(DecLitNode node) - { - return; + InUsePredicates = new HashSet(); } public override void Visit(FirstNameNode node) { - // We should reach this block only if we are working with an implicity 'ThisRecord'. - if (_idents.Length == 1 && _typeScope.TryGetType(node.Ident.Name, out _)) + if (_idents.Contains(node.Ident.Name) || _typeScope.TryGetType(node.Ident.Name, out _)) { - InUsePredicates.Add(_idents.First()); + InUsePredicates.Add(node.Ident.Name); } } - // We can assume if visiting a DottedNameNode is always a whole scope. public override bool PreVisit(DottedNameNode node) { - var left = node.Left.AsFirstName(); - - if (_idents.Contains(left.Ident.Name)) - { - InUsePredicates.Add(left.Ident.Name); - } - - // No need to visit left/right separately. + // No need to visit right node. + node.Left.Accept(this); return false; } - public override void Visit(ParentNode node) - { - return; - } - - public override void Visit(SelfNode node) - { - return; - } - - public override void PostVisit(StrInterpNode node) - { - return; - } - - public override void PostVisit(DottedNameNode node) - { - return; - } - - public override void PostVisit(UnaryOpNode node) - { - return; - } - - public override void PostVisit(BinaryOpNode node) - { - return; - } - - public override void PostVisit(VariadicOpNode node) - { - return; - } - - public override void PostVisit(CallNode node) - { - return; - } - - public override void PostVisit(ListNode node) - { - return; - } - - public override void PostVisit(RecordNode node) - { - return; - } - - public override void PostVisit(TableNode node) - { - return; - } - - public override void PostVisit(AsNode node) - { - return; - } - public override bool PreVisit(CallNode node) { + if (_allowAggregateCall) + { + // If `ThisGroup` is placed inside multiple calls, raise a warning. + // Example + // Summarize(t1, Fruit, CountRows( Distinct( ThisGroup, Supplier ) ) As CountOfSuppliers ) + _allowAggregateCall = false; + return true; + } + return false; } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs b/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs index 6760f6e134..0ef7ac566b 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using System.Threading; namespace Microsoft.PowerFx.Core.Utils @@ -36,16 +37,10 @@ public static int Size(T[] rgv) return rgv == null ? 0 : rgv.Length; } - public static int Size(List list) + public static int Size(IEnumerable enumerable) { - Contracts.AssertValueOrNull(list); - return list == null ? 0 : list.Count; - } - - public static int Size(IList list) - { - Contracts.AssertValueOrNull(list); - return list == null ? 0 : list.Count; + Contracts.AssertValueOrNull(enumerable); + return enumerable == null ? 0 : enumerable.Count(); } public static int Size(Stack stack) diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs index f85f9ad6e4..c49abb00ef 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs @@ -8,17 +8,14 @@ using System.Text; using System.Threading; using System.Threading.Tasks; -using Microsoft.PowerFx.Core; using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Functions; using Microsoft.PowerFx.Core.Glue; using Microsoft.PowerFx.Core.Parser; -using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Utils; using Microsoft.PowerFx.Functions; using Microsoft.PowerFx.Interpreter; -using Microsoft.PowerFx.Syntax; using Microsoft.PowerFx.Types; namespace Microsoft.PowerFx @@ -463,9 +460,11 @@ private void AddUserDefinedFunctions(IEnumerable parsedUdfs, ReadOnlySymbol List bindErrors = new List(); - if (binding.ErrorContainer.GetErrors(ref bindErrors)) + var onlyErrors = binding.ErrorContainer.GetErrors(DocumentErrorSeverity.Moderate); + + if (onlyErrors.Count() > 0) { - sb.AppendLine(string.Join(", ", bindErrors.Select(err => err.ToString()))); + sb.AppendLine(string.Join(", ", onlyErrors.Select(err => err.ToString()))); } else { diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs index ebdc59df63..9758817e97 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs @@ -149,8 +149,10 @@ public void TestCountRowsWarningForCachedData(bool isCachedData) // Only shows warning if data source is passed directly to CountRows result = engine.Check("CountRows(Filter(Accounts, IsBlank('Address 1: City')))"); - Assert.True(result.IsSuccess); - Assert.Empty(result.Errors); + Assert.True(result.IsSuccess); + + // Some functions (like 'Filter') can produce a 'WarnCheckPredicateUsage' warning. Any other warnings are unexpected. + Assert.Empty(result.Errors.Where(err => err.MessageKey != "WarnCheckPredicateUsage")); } } } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs index 714f98d4ab..ac243c30c2 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs @@ -314,32 +314,6 @@ public void TestSuggestEscapedEnumName() Assert.Contains("'Name That.Requires!escaping'.Field2", result); } - [Theory] - [InlineData("in", "in_text")] - [InlineData("or", "or_text")] - [InlineData("and", "and_text")] - [InlineData("In", "In_text")] - [InlineData("Or", "Or_text")] - [InlineData("And", "And_text")] - public void TesafdghghffgnfrghhhhhhhhhhhhhhhhhhhhhhhhhhhhpedEnumName(string expr, string varName) - { - var engine = new Engine(); - engine.Config.SymbolTable.AddVariable(varName, FormulaType.String); - - var test = Features.PowerFxV1.GetType(); - var tese = test.GetProperty("SkipExpandableSetSemantics", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); - - foreach (var textFirst in new[] { false, true }) - { - expr = textFirst ? "=" + expr : expr; - - var check = engine.Check(expr, options: new ParserOptions() { TextFirst = textFirst }); - var suggestions = engine.Suggest(check, expr.Length); - - Assert.Equal(textFirst ? 1 : 0, suggestions.ReplacementStartIndex); - } - } - [Theory] [InlineData("SortByColumns(|", 3, "The table to sort.", "SortByColumns(source, column, ...)")] [InlineData("SortByColumns(tbl1,|", 3, "A unique column name.", "SortByColumns(source, column, ...)")] diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs index 40e113ac60..8c287a7af8 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.PowerFx.Core.Entities; +using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Functions; using Microsoft.PowerFx.Core.Functions.Delegation; using Microsoft.PowerFx.Core.Functions.Delegation.DelegationMetadata; @@ -4325,13 +4326,16 @@ public void TestSilentValidDelegatableFilterPredicateNode(string script, bool wa Assert.True(result.IsSuccess); + // Filter function can produce a 'WarnCheckPredicateUsage' warning. Any other warnings are unexpected. + var filteredErrors = result.Errors.Where(err => err.MessageKey != "WarnCheckPredicateUsage").ToList(); + if (warnings) { - Assert.True(result.Errors.Count() > 0, "Expected warnings in original function"); + Assert.True(filteredErrors.Count() > 0, "Expected warnings in original function"); } else { - Assert.False(result.Errors.Count() > 0, "No warnings expected in original function"); + Assert.False(filteredErrors.Count() > 0, "No warnings expected in original function"); } // then run with the mock filter function that does silent delgation checks @@ -4500,7 +4504,7 @@ public void TestBlobFunction(string expression, string expectedType) [InlineData("Table(Search(DS, \"Foo\", Name), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 1)] // existing warning due to sqrt should propagate, no new warnigns on table - [InlineData("Table(Filter(DS, Sqrt(Age) > 5), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 1)] + [InlineData("Table(Filter(DS, Sqrt(Age) > 5), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 2)] public void TexlFunctionTypeSemanticsTable_PageableInputs(string script, string expectedSchema, int errorCount) { var dataSourceSchema = TestUtils.DT("*[Id:n, Name:s, Age:n]"); @@ -4546,6 +4550,53 @@ public void TestTypeLiteralsNegative(string script, string expectedSchema) features: Features.PowerFxV1); } + [Theory] + [InlineData("Filter(t1, a = 1)", false)] + [InlineData("Sum(t1, a)", false)] + [InlineData("Average(t1, a)", false)] + [InlineData("Min(t1, a)", false)] + [InlineData("Max(t1, a)", false)] + [InlineData("Join(t1, t2, LeftRecord.a = RightRecord.x, JoinType.Inner, RightRecord.z As Z)", false)] + [InlineData("Summarize(t1, a)", false)] + [InlineData("Summarize(t1, a, CountRows(ThisGroup) As Counter)", false)] + [InlineData("LookUp(t1, 1=1)", false)] // The 'LookUp' wont produce any warning because it has not been set to analyse the predicate. + + [InlineData("Filter(t1, Abs(a) = 1)", true)] + [InlineData("Summarize(t1, a, CountRows(Filter(ThisGroup, b = \"test\")) As Counter)", true)] + [InlineData("Sum(t1, 1)", true)] + [InlineData("Sum(t1, Abs(a))", true)] + [InlineData("Average(t1, 1)", true)] + [InlineData("Average(t1, Abs(a))", true)] + [InlineData("Min(t1, 1)", true)] + [InlineData("Min(t1, Abs(a))", true)] + [InlineData("Max(t1, 1)", true)] + [InlineData("Max(t1, Abs(a))", true)] + + public void TestScopePredicate(string expression, bool expectingWarning) + { + var symbolTable = new SymbolTable(); + + symbolTable.AddVariable("t1", new TableType(TestUtils.DT("*[a:w,b:s,c:b]"))); + symbolTable.AddVariable("t2", new TableType(TestUtils.DT("*[x:w,y:s,z:b]"))); + symbolTable.AddFunction(new JoinFunction()); + symbolTable.AddFunction(new SummarizeFunction()); + + var engine = new Engine(); + var check = engine.Check(expression, symbolTable: symbolTable); + + // The predicate checking should never fail, but it may produce a warning. + Assert.True(check.IsSuccess, string.Join("\n", check.Errors.Select(err => err.ToString()))); + + if (expectingWarning) + { + Assert.Contains(check.Errors, err => err.MessageKey == "WarnCheckPredicateUsage"); + } + else + { + Assert.DoesNotContain(check.Errors, err => err.MessageKey == "WarnCheckPredicateUsage"); + } + } + private void TestBindingPurity(string script, bool isPure, SymbolTable symbolTable = null) { var config = new PowerFxConfig @@ -4652,8 +4703,10 @@ private static void TestSimpleBindingSuccess(string script, DType expectedType, var opts = new ParserOptions() { NumberIsFloat = numberIsFloat }; var result = engine.Check(script, opts); Assert.Equal(expectedType, result.Binding.ResultType); - Assert.False(result.Binding.ErrorContainer.HasErrors()); Assert.True(result.IsSuccess); + + // Some functions (like Filter) may produce warnings related to the predicate. We don't want to fail the test in that case. + Assert.False(result.Binding.ErrorContainer.HasErrors(DocumentErrorSeverity.Moderate)); } } } From 9212756b9b7685a4a1ce54f0300e9e2def05550f Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Mon, 10 Mar 2025 10:34:49 -0500 Subject: [PATCH 04/13] Warning message fix. --- .../Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index c440d5d72d..a07932ae8c 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -304,7 +304,7 @@ public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IEr if (!visitor.InUsePredicates.Contains(ThisGroup)) { - errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[i], TexlStrings.WarnCheckPredicateUsage, string.Join("/", idents.Select(id => id.Value))); + errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[i], TexlStrings.WarnCheckPredicateUsage, ThisGroup); } } } From f6919743e91a2aa92014bfddf3ff42f78413da21 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Wed, 12 Mar 2025 10:38:06 -0500 Subject: [PATCH 05/13] PR feedback. Pivot. --- .../App/ErrorContainers/ErrorContainer.cs | 21 ---- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 25 +++- .../Functions/FunctionScopeInfo.cs | 116 +++++++++--------- .../IR/Visitors/PredicateIRVisitor.cs | 104 ++++++++++++++++ .../Syntax/Visitors/ScopePredicateVisitor.cs | 35 +++--- .../Utils/CollectionUtils.cs | 13 +- .../ParsedExpression.cs | 2 +- .../RecalcEngine.cs | 6 +- src/strings/PowerFxResources.en-US.resx | 2 +- .../TestDelegationValidation.cs | 7 +- .../TexlTests.cs | 25 ++-- 11 files changed, 227 insertions(+), 129 deletions(-) create mode 100644 src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs diff --git a/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs b/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs index e7097187ab..e4a5ea8fe4 100644 --- a/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs +++ b/src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs @@ -37,16 +37,6 @@ public bool HasErrors() return CollectionUtils.Size(_errors) > 0; } - public bool HasErrors(DocumentErrorSeverity severity) - { - if (_errors == null) - { - return false; - } - - return CollectionUtils.Size(_errors.Where(err => err.Severity >= severity)) > 0; - } - public bool HasErrors(TexlNode node, DocumentErrorSeverity severity = DocumentErrorSeverity.Suggestion) { Contracts.AssertValue(node); @@ -134,17 +124,6 @@ public IEnumerable GetErrors() } } - public IEnumerable GetErrors(DocumentErrorSeverity severity) - { - if (_errors != null) - { - foreach (var err in _errors.Where(err => err.Severity >= severity)) - { - yield return err; - } - } - } - public TexlError EnsureError(TexlNode node, ErrorResourceKey errKey, params object[] args) { return EnsureError(DefaultSeverity, node, errKey, args); diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index df1b0e1f83..8ba1c2325c 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -1124,6 +1124,20 @@ public void CheckAndMarkAsPageable(CallNode node, TexlFunction func) TransitionsFromAsyncToSync = true; } } + } + + public void CheckPredicateUsage(CallNode node, TexlFunction func) + { + Contracts.AssertValue(node); + Contracts.AssertValue(func); + + if (func.ScopeInfo != null && + func.ScopeInfo.CheckPredicateUsage && + !node.Args.ChildNodes.Any(child => child is ErrorNode) && + TryGetCall(node.Id, out var callInfo)) + { + func.ScopeInfo.CheckPredicateFields(GetUsedScopeFields(callInfo), node, GetLambdaParamNames(callInfo.ScopeNest + 1), ErrorContainer); + } } public void CheckAndMarkAsPageable(FirstNameNode node) @@ -2845,7 +2859,9 @@ public override void Visit(FirstNameNode node) _txb.SetType(node, nodeType); _txb.SetInfo(node, info); _txb.SetLambdaScopeLevel(node, info.UpCount); - _txb.AddFieldToQuerySelects(nodeType, nodeName); + _txb.AddFieldToQuerySelects(nodeType, nodeName); + + //_txb.SetScopeNode(node.Id, info); return; } @@ -4494,7 +4510,7 @@ public override bool PreVisit(CallNode node) // Determine the Scope Identifier using the func.ScopeArgs arg required = scopeInfo.GetScopeIdent(node.Args.Children.ToArray(), out scopeIdentifiers); - if (scopeInfo.CheckInput(_txb.Features, node, node.Args.Children.ToArray(), out scope, _txb.ErrorContainer, GetScopeArgsTypes(node.Args.Children, argsCount))) + if (scopeInfo.CheckInput(_txb.Features, node, node.Args.Children.ToArray(), out scope, GetScopeArgsTypes(node.Args.Children, argsCount))) { if (_txb.TryGetEntityInfo(node.Args.Children[0], out expandInfo)) { @@ -4593,7 +4609,7 @@ public override bool PreVisit(CallNode node) args[i].Accept(this); } - fArgsValid = scopeInfo.CheckInput(_txb.Features, node, args, out typeScope, _txb.ErrorContainer, GetScopeArgsTypes(node.Args.Children, maybeFunc.ScopeArgs)); + fArgsValid = scopeInfo.CheckInput(_txb.Features, node, args, out typeScope, GetScopeArgsTypes(node.Args.Children, maybeFunc.ScopeArgs)); // Determine the scope identifier using the first node for lambda params identRequired = scopeInfo.GetScopeIdent(args, out scopeIdent); @@ -4862,7 +4878,8 @@ private void FinalizeCall(CallNode node) } _txb.CheckAndMarkAsDelegatable(node); - _txb.CheckAndMarkAsPageable(node, func); + _txb.CheckAndMarkAsPageable(node, func); + _txb.CheckPredicateUsage(node, func); // A function will produce a constant output (and have no side-effects, which is important for // caching/precomputing the result) iff the function is pure and its arguments are constant. diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index a07932ae8c..ab5462351b 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -2,9 +2,12 @@ // Licensed under the MIT license. using System; +using System.Collections.Generic; using System.Linq; +using System.Xml.Linq; using Microsoft.PowerFx.Core.App.ErrorContainers; using Microsoft.PowerFx.Core.Binding; +using Microsoft.PowerFx.Core.Binding.BindInfo; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Localization; using Microsoft.PowerFx.Core.Syntax.Visitors; @@ -100,15 +103,10 @@ public FunctionScopeInfo( /// /// List of data sources to compose the calculated type. /// - public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, IErrorContainer errors, params DType[] inputSchema) + public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) { var result = CheckInput(features, callNode, inputNodes[0], inputSchema[0], out typeScope); - if (result && CheckPredicateUsage) - { - CheckPredicates(inputNodes, typeScope, errors); - } - return result; } @@ -204,27 +202,6 @@ public virtual bool CheckInput(Features features, CallNode callNode, TexlNode in return CheckInput(features, inputNode, inputSchema, TexlFunction.DefaultErrorContainer, out typeScope); } - public virtual void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors = null) - { - GetScopeIdent(inputNodes, out DName[] idents); - var visitor = new ScopePredicateVisitor(typeScope, idents); - - for (var i = 1; i < inputNodes.Length; i++) - { - var arg = inputNodes[i]; - - if (_function.IsLambdaParam(inputNodes[i], i)) - { - arg.Accept(visitor); - } - } - - if (visitor.InUsePredicates.Count == 0) - { - errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[0], TexlStrings.WarnCheckPredicateUsage, idents.First()); - } - } - /// /// Get the scope identifiers for the function based on the CallNode child args. /// The majority of the functions will have a single scope identifier named ThisRecord. @@ -243,6 +220,21 @@ public virtual bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) } return false; + } + + public virtual void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) + { + if (fields.GetAllNames(DPath.Root).Any()) + { + return; + } + + GetScopeIdent(callNode.Args.ChildNodes.ToArray(), out var idents); + + if (!lambdaNames.Any(name => lambdaNames.Contains(name))) + { + errors.EnsureError(DocumentErrorSeverity.Warning, callNode, TexlStrings.WarnCheckPredicateUsage); + } } } @@ -251,7 +243,7 @@ internal class FunctionThisGroupScopeInfo : FunctionScopeInfo public static DName ThisGroup => new DName("ThisGroup"); public FunctionThisGroupScopeInfo(TexlFunction function) - : base(function, appliesToArgument: (argIndex) => argIndex > 0, checkPredicateUsage: true) + : base(function, appliesToArgument: (argIndex) => argIndex > 0) { } @@ -289,26 +281,6 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode i return ret; } - - public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors) - { - GetScopeIdent(inputNodes, out DName[] idents); - - for (int i = 0; i < inputNodes.Length; i++) - { - if (_function.IsLambdaParam(inputNodes[i], i)) - { - // We create a new visitor object for each iteration since lambda parms can appear at any place in the expression. - var visitor = new ScopePredicateVisitor(typeScope, new[] { ThisGroup }, true); - inputNodes[i].Accept(visitor); - - if (!visitor.InUsePredicates.Contains(ThisGroup)) - { - errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[i], TexlStrings.WarnCheckPredicateUsage, ThisGroup); - } - } - } - } } internal class FunctionJoinScopeInfo : FunctionScopeInfo @@ -322,7 +294,7 @@ public FunctionJoinScopeInfo(TexlFunction function) { } - public override bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, IErrorContainer errors, params DType[] inputSchema) + public override bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) { var ret = true; var argCount = Math.Min(inputNodes.Length, 2); @@ -337,11 +309,6 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode[] typeScope = typeScope.Add(idents[i], type); } - if (ret) - { - CheckPredicates(inputNodes, typeScope, errors); - } - return ret; } @@ -372,18 +339,47 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) return false; } - public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors) + public override void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) { - GetScopeIdent(inputNodes, out DName[] idents); + // If Join call node has less than 5 records, we are possibly looking for suggestions. + if (callNode.Args.ChildNodes.Count < 5) + { + return; + } - var visitor = new ScopePredicateVisitor(typeScope, idents); + GetScopeIdent(callNode.Args.ChildNodes.ToArray(), out var idents); - inputNodes[2].Accept(visitor); + var foundIdents = new HashSet(); + var predicate = callNode.Args.ChildNodes[2]; - if (!visitor.InUsePredicates.Contains(idents[0]) || !visitor.InUsePredicates.Contains(idents[1])) + // In the Join function, arg2 and argN > 3 are lambdas nodes. + // We need to check if scope identifiers are used arg2 (predicate). + foreach (var lambda in lambdaNames) { - errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[2], TexlStrings.WarnCheckPredicateUsage, string.Join("/", idents.Select(id => id.Value))); + var parent = lambda.Node.Parent; + + while (parent != null) + { + if (parent.Id == predicate.Id) + { + foundIdents.Add(lambda.Name); + break; + } + + parent = parent.Parent; + } + } + + var foundIdentsArray = foundIdents.ToArray(); + + if (foundIdents.Count == 2 && + fields.TryGetType(foundIdentsArray[0], out _) && + fields.TryGetType(foundIdentsArray[1], out _)) + { + return; } + + errors.EnsureError(DocumentErrorSeverity.Warning, callNode, TexlStrings.WarnCheckPredicateUsage); } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs new file mode 100644 index 0000000000..55fd3a7f2e --- /dev/null +++ b/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using System; +using Microsoft.PowerFx.Core.IR.Nodes; + +namespace Microsoft.PowerFx.Core.IR +{ + internal class PredicateIRVisitor : IRNodeVisitor + { + public override RetVal Visit(TextLiteralNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(NumberLiteralNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(DecimalLiteralNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(BooleanLiteralNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(ColorLiteralNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(RecordNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(ErrorNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(LazyEvalNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(CallNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(BinaryOpNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(UnaryOpNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(ScopeAccessNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(RecordFieldAccessNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(ResolvedObjectNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(SingleColumnTableAccessNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(ChainingNode node, Context context) + { + throw new NotImplementedException(); + } + + public override RetVal Visit(AggregateCoercionNode node, Context context) + { + throw new NotImplementedException(); + } + + public class RetVal + { + } + + public class Context + { + } + } +} diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs index b8ce08db62..15ba82b035 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Functions; using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Utils; @@ -13,29 +14,29 @@ namespace Microsoft.PowerFx.Core.Syntax.Visitors { internal class ScopePredicateVisitor : IdentityTexlVisitor { - private readonly DType _typeScope; - private readonly DName[] _idents; - - // The Summarize function creates a 'ThisGroup' indentifier that can only be used inside a call. - private bool _allowAggregateCall; + private readonly TexlBinding _binding; public HashSet InUsePredicates; - public ScopePredicateVisitor(DType typeScope, DName[] indents, bool allowAggregateCall = false) + public ScopePredicateVisitor(TexlBinding binding) { - _typeScope = typeScope; - _idents = indents; - _allowAggregateCall = allowAggregateCall; + _binding = binding; InUsePredicates = new HashSet(); } public override void Visit(FirstNameNode node) { - if (_idents.Contains(node.Ident.Name) || _typeScope.TryGetType(node.Ident.Name, out _)) - { - InUsePredicates.Add(node.Ident.Name); - } + var info = _binding.GetInfo(node); + + //_binding.TryGetCall(_node.Id, out var callInfo); + + //if (_idents.Contains(node.Ident.Name) || _typeScope.TryGetType(node.Ident.Name, out _)) + //{ + // InUsePredicates.Add(node.Ident.Name); + //} + + var test = "dasdasd"; } public override bool PreVisit(DottedNameNode node) @@ -47,12 +48,10 @@ public override bool PreVisit(DottedNameNode node) public override bool PreVisit(CallNode node) { - if (_allowAggregateCall) + if (_binding.TryGetCall(node.Id, out var callInfo) && + callInfo.Function.ScopeInfo != null && + callInfo.Function.ScopeInfo.CheckPredicateUsage) { - // If `ThisGroup` is placed inside multiple calls, raise a warning. - // Example - // Summarize(t1, Fruit, CountRows( Distinct( ThisGroup, Supplier ) ) As CountOfSuppliers ) - _allowAggregateCall = false; return true; } diff --git a/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs b/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs index 0ef7ac566b..6760f6e134 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Linq; using System.Threading; namespace Microsoft.PowerFx.Core.Utils @@ -37,10 +36,16 @@ public static int Size(T[] rgv) return rgv == null ? 0 : rgv.Length; } - public static int Size(IEnumerable enumerable) + public static int Size(List list) { - Contracts.AssertValueOrNull(enumerable); - return enumerable == null ? 0 : enumerable.Count(); + Contracts.AssertValueOrNull(list); + return list == null ? 0 : list.Count; + } + + public static int Size(IList list) + { + Contracts.AssertValueOrNull(list); + return list == null ? 0 : list.Count; } public static int Size(Stack stack) diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs b/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs index 1deb58b0e0..ce8ad19063 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/ParsedExpression.cs @@ -64,7 +64,7 @@ public static async Task EvalAsync(this IExpressionEvaluator expr, /// public static IExpressionEvaluator GetEvaluator(this CheckResult result) { - var stackMarker = new StackDepthCounter(result.Engine.Config.MaxCallDepth); + var stackMarker = new StackDepthCounter(PowerFxConfig.DefaultMaxCallDepth); return GetEvaluator(result, stackMarker); } diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs index c49abb00ef..c3035622b8 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs @@ -460,11 +460,9 @@ private void AddUserDefinedFunctions(IEnumerable parsedUdfs, ReadOnlySymbol List bindErrors = new List(); - var onlyErrors = binding.ErrorContainer.GetErrors(DocumentErrorSeverity.Moderate); - - if (onlyErrors.Count() > 0) + if (binding.ErrorContainer.GetErrors(ref bindErrors)) { - sb.AppendLine(string.Join(", ", onlyErrors.Select(err => err.ToString()))); + sb.AppendLine(string.Join(", ", bindErrors.Select(err => err.ToString()))); } else { diff --git a/src/strings/PowerFxResources.en-US.resx b/src/strings/PowerFxResources.en-US.resx index 46944f1ff5..7629fa312e 100644 --- a/src/strings/PowerFxResources.en-US.resx +++ b/src/strings/PowerFxResources.en-US.resx @@ -2440,7 +2440,7 @@ Duplicate columns. - Warning: The expression does not access '{0}' directly and can return unexpected results. + Warning: The expression does not access scope fields directly and can return unexpected results. Warning given when an expression with a function operating over a table does not access the predicate directly. diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs index 9758817e97..31bbefd52b 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs @@ -81,7 +81,7 @@ public void TestDelegableExpressions_ColumnNamesAsLiteralStrings(string expressi [Theory] [InlineData("UDF1()", "UDF1():Accounts = Accounts;", true)] - [InlineData("Filter(UDF1(), \"name\" <> \"\")", "UDF1():Accounts = Accounts;", true)] + [InlineData("Filter(UDF1(), \"name\" <> \"\")", "UDF1():Accounts = Accounts;", false)] public void TestDelegableExpressions_UserDfeinedFunction(string expression, string script, bool isDelegable) { TestDelegableExpressions(Features.PowerFxV1, expression, isDelegable, script); @@ -149,10 +149,7 @@ public void TestCountRowsWarningForCachedData(bool isCachedData) // Only shows warning if data source is passed directly to CountRows result = engine.Check("CountRows(Filter(Accounts, IsBlank('Address 1: City')))"); - Assert.True(result.IsSuccess); - - // Some functions (like 'Filter') can produce a 'WarnCheckPredicateUsage' warning. Any other warnings are unexpected. - Assert.Empty(result.Errors.Where(err => err.MessageKey != "WarnCheckPredicateUsage")); + Assert.True(result.IsSuccess); } } } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs index 8c287a7af8..c633ad197b 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs @@ -4504,7 +4504,7 @@ public void TestBlobFunction(string expression, string expectedType) [InlineData("Table(Search(DS, \"Foo\", Name), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 1)] // existing warning due to sqrt should propagate, no new warnigns on table - [InlineData("Table(Filter(DS, Sqrt(Age) > 5), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 2)] + [InlineData("Table(Filter(DS, Sqrt(Age) > 5), FirstN(LastN(DS, 10), 5))", "*[Id:n, Name:s, Age:n]", 1)] public void TexlFunctionTypeSemanticsTable_PageableInputs(string script, string expectedSchema, int errorCount) { var dataSourceSchema = TestUtils.DT("*[Id:n, Name:s, Age:n]"); @@ -4552,25 +4552,30 @@ public void TestTypeLiteralsNegative(string script, string expectedSchema) [Theory] [InlineData("Filter(t1, a = 1)", false)] + [InlineData("Filter(t1, Abs(a) = 1)", false)] [InlineData("Sum(t1, a)", false)] [InlineData("Average(t1, a)", false)] [InlineData("Min(t1, a)", false)] [InlineData("Max(t1, a)", false)] [InlineData("Join(t1, t2, LeftRecord.a = RightRecord.x, JoinType.Inner, RightRecord.z As Z)", false)] + [InlineData("Join(t1 As X, t2 As Y, X.a = Y.x, JoinType.Inner, Y.z As Z)", false)] [InlineData("Summarize(t1, a)", false)] [InlineData("Summarize(t1, a, CountRows(ThisGroup) As Counter)", false)] [InlineData("LookUp(t1, 1=1)", false)] // The 'LookUp' wont produce any warning because it has not been set to analyse the predicate. - - [InlineData("Filter(t1, Abs(a) = 1)", true)] - [InlineData("Summarize(t1, a, CountRows(Filter(ThisGroup, b = \"test\")) As Counter)", true)] + [InlineData("Max(t1, Abs(a))", false)] + [InlineData("Min(t1, Abs(a))", false)] + [InlineData("Average(t1, Abs(a))", false)] + [InlineData("Sum(t1, Abs(a))", false)] + [InlineData("Filter(t1, With({x:a}, StartsWith(x, \"something\")))", false)] + [InlineData("Filter(t1 As Tbl, With({b:\"hello\"}, StartsWith(Tbl.b, b)))", false)] + + [InlineData("Filter(t1, 1=1)", true)] + [InlineData("Filter(t1, With({b:\"hello\"}, StartsWith(b, \"something\")))", true)] [InlineData("Sum(t1, 1)", true)] - [InlineData("Sum(t1, Abs(a))", true)] [InlineData("Average(t1, 1)", true)] - [InlineData("Average(t1, Abs(a))", true)] [InlineData("Min(t1, 1)", true)] - [InlineData("Min(t1, Abs(a))", true)] [InlineData("Max(t1, 1)", true)] - [InlineData("Max(t1, Abs(a))", true)] + [InlineData("Join(t1, t2, true, JoinType.Inner, RightRecord.z As Z, LeftRecord.a As AAA)", true)] public void TestScopePredicate(string expression, bool expectingWarning) { @@ -4703,10 +4708,8 @@ private static void TestSimpleBindingSuccess(string script, DType expectedType, var opts = new ParserOptions() { NumberIsFloat = numberIsFloat }; var result = engine.Check(script, opts); Assert.Equal(expectedType, result.Binding.ResultType); + Assert.False(result.Binding.ErrorContainer.HasErrors()); Assert.True(result.IsSuccess); - - // Some functions (like Filter) may produce warnings related to the predicate. We don't want to fail the test in that case. - Assert.False(result.Binding.ErrorContainer.HasErrors(DocumentErrorSeverity.Moderate)); } } } From d0447425dd6f093d880bc2b8b2ee0fa475bf229b Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Wed, 12 Mar 2025 14:20:59 -0500 Subject: [PATCH 06/13] Fixes and code clean-up --- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 16 +-- .../Functions/FunctionScopeInfo.cs | 10 +- .../IR/Visitors/PredicateIRVisitor.cs | 104 ------------------ .../Syntax/Visitors/ScopePredicateVisitor.cs | 61 ---------- .../ExpressionErrorTests.cs | 37 ------- .../TexlTests.cs | 8 +- 6 files changed, 14 insertions(+), 222 deletions(-) delete mode 100644 src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs delete mode 100644 src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index 8ba1c2325c..ef7bce8f02 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -2064,10 +2064,14 @@ public DType GetUsedScopeFields(CallInfo call) foreach (var name in GetLambdaParamNames(call.ScopeNest + 1)) { - var fError = false; + var fError = false; + var fieldName = name.Name; + + call.CursorType.DisplayNameProvider?.TryGetLogicalName(name.Name, out fieldName); + if (!name.Node.InTree(arg0) && - name.Node.InTree(call.Node) && - call.CursorType.TryGetType(name.Name, out var lambdaParamType)) + name.Node.InTree(call.Node) && + call.CursorType.TryGetType(fieldName, out var lambdaParamType)) { if (name.Node.Parent is DottedNameNode dotted) { @@ -2859,9 +2863,7 @@ public override void Visit(FirstNameNode node) _txb.SetType(node, nodeType); _txb.SetInfo(node, info); _txb.SetLambdaScopeLevel(node, info.UpCount); - _txb.AddFieldToQuerySelects(nodeType, nodeName); - - //_txb.SetScopeNode(node.Id, info); + _txb.AddFieldToQuerySelects(nodeType, nodeName); return; } @@ -5343,7 +5345,7 @@ private void PreVisitBottomUp(CallNode node, int argCountVisited, Scope scopeNew // If PreVisit resulted in errors for the node (and a non-null CallInfo), // we're done -- we have a match and appropriate errors logged already. - if (_txb.ErrorContainer.HasErrors(node, DocumentErrorSeverity.Moderate) || _txb.ErrorContainer.HasErrors(node.Head.Token)) + if (_txb.ErrorContainer.HasErrors(node) || _txb.ErrorContainer.HasErrors(node.Head.Token)) { Contracts.Assert(info != null); diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index ab5462351b..b58ec0b733 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -4,13 +4,11 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Xml.Linq; using Microsoft.PowerFx.Core.App.ErrorContainers; using Microsoft.PowerFx.Core.Binding; using Microsoft.PowerFx.Core.Binding.BindInfo; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Localization; -using Microsoft.PowerFx.Core.Syntax.Visitors; using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Utils; using Microsoft.PowerFx.Syntax; @@ -105,9 +103,7 @@ public FunctionScopeInfo( /// public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) { - var result = CheckInput(features, callNode, inputNodes[0], inputSchema[0], out typeScope); - - return result; + return CheckInput(features, callNode, inputNodes[0], inputSchema[0], out typeScope); } // Typecheck an input for this function, and get the cursor type for an invocation with that input. @@ -224,14 +220,14 @@ public virtual bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) public virtual void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) { - if (fields.GetAllNames(DPath.Root).Any()) + if (fields == DType.Error || fields.GetAllNames(DPath.Root).Any()) { return; } GetScopeIdent(callNode.Args.ChildNodes.ToArray(), out var idents); - if (!lambdaNames.Any(name => lambdaNames.Contains(name))) + if (!lambdaNames.Any(lambdaName => idents.Contains(lambdaName.Name))) { errors.EnsureError(DocumentErrorSeverity.Warning, callNode, TexlStrings.WarnCheckPredicateUsage); } diff --git a/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs deleted file mode 100644 index 55fd3a7f2e..0000000000 --- a/src/libraries/Microsoft.PowerFx.Core/IR/Visitors/PredicateIRVisitor.cs +++ /dev/null @@ -1,104 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -using System; -using Microsoft.PowerFx.Core.IR.Nodes; - -namespace Microsoft.PowerFx.Core.IR -{ - internal class PredicateIRVisitor : IRNodeVisitor - { - public override RetVal Visit(TextLiteralNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(NumberLiteralNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(DecimalLiteralNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(BooleanLiteralNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(ColorLiteralNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(RecordNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(ErrorNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(LazyEvalNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(CallNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(BinaryOpNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(UnaryOpNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(ScopeAccessNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(RecordFieldAccessNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(ResolvedObjectNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(SingleColumnTableAccessNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(ChainingNode node, Context context) - { - throw new NotImplementedException(); - } - - public override RetVal Visit(AggregateCoercionNode node, Context context) - { - throw new NotImplementedException(); - } - - public class RetVal - { - } - - public class Context - { - } - } -} diff --git a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs b/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs deleted file mode 100644 index 15ba82b035..0000000000 --- a/src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -using System; -using System.Collections.Generic; -using System.Linq; -using Microsoft.PowerFx.Core.Binding; -using Microsoft.PowerFx.Core.Functions; -using Microsoft.PowerFx.Core.Types; -using Microsoft.PowerFx.Core.Utils; -using Microsoft.PowerFx.Syntax; - -namespace Microsoft.PowerFx.Core.Syntax.Visitors -{ - internal class ScopePredicateVisitor : IdentityTexlVisitor - { - private readonly TexlBinding _binding; - - public HashSet InUsePredicates; - - public ScopePredicateVisitor(TexlBinding binding) - { - _binding = binding; - - InUsePredicates = new HashSet(); - } - - public override void Visit(FirstNameNode node) - { - var info = _binding.GetInfo(node); - - //_binding.TryGetCall(_node.Id, out var callInfo); - - //if (_idents.Contains(node.Ident.Name) || _typeScope.TryGetType(node.Ident.Name, out _)) - //{ - // InUsePredicates.Add(node.Ident.Name); - //} - - var test = "dasdasd"; - } - - public override bool PreVisit(DottedNameNode node) - { - // No need to visit right node. - node.Left.Accept(this); - return false; - } - - public override bool PreVisit(CallNode node) - { - if (_binding.TryGetCall(node.Id, out var callInfo) && - callInfo.Function.ScopeInfo != null && - callInfo.Function.ScopeInfo.CheckPredicateUsage) - { - return true; - } - - return false; - } - } -} diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs index 240ac0ac79..8b6cb2ceed 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionErrorTests.cs @@ -7,10 +7,7 @@ using System.Linq; using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Localization; -using Microsoft.PowerFx.Core.Texl.Builtins; -using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Syntax; -using Microsoft.PowerFx.Types; using Xunit; namespace Microsoft.PowerFx.Core.Tests @@ -50,40 +47,6 @@ public void TestWarning() Assert.Equal("Warning 2-5: ouch", error.ToString()); } - [Theory] - [InlineData("Join(t1 As L, t1 As R, L.a = R.a, JoinType.Inner, R.a As AAA)")] - [InlineData("Filter(t1, a > 1)")] - [InlineData("Filter(t1, ThisRecord.a > 1)")] - [InlineData("Filter(t1 As T, T.a > 1)")] - [InlineData("Filter(t1 As T, 1 < T.a)")] - [InlineData("Filter(t1 As T, With({x:1}, x) > T.a)")] - - [InlineData("Filter(t1 As T, With({x:1}, x))", true)] - [InlineData("Filter(t1 As T, true)", true)] - [InlineData("Join(t1 As L, t1 As R, true, JoinType.Inner, R.a As AAA)", true)] - [InlineData("Join(t1 As L, t1 As R, With({L:R, R:R}, L.a = R.a), JoinType.Inner, R.a As RA, L.a As LA)", true)] - public void CheckPredicateUsageTest(string expression, bool raiseWarning = false) - { - var engine = new Engine(); - var symbols = new SymbolTable(); - - DType.TryParse("*[a:w,b:w,c:w]", out var tableType); - - symbols.AddFunction(new JoinFunction()); - symbols.AddVariable("t1", FormulaType.Build(tableType)); - - var check = engine.Check(expression, symbolTable: symbols); - - if (raiseWarning) - { - Assert.Contains(check.Errors, d => d.IsWarning); - } - else - { - Assert.DoesNotContain(check.Errors, d => d.IsWarning); - } - } - [Fact] public void NullSpanToString() { diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs index c633ad197b..55f2d579d8 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs @@ -9,7 +9,6 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.PowerFx.Core.Entities; -using Microsoft.PowerFx.Core.Errors; using Microsoft.PowerFx.Core.Functions; using Microsoft.PowerFx.Core.Functions.Delegation; using Microsoft.PowerFx.Core.Functions.Delegation.DelegationMetadata; @@ -4326,16 +4325,13 @@ public void TestSilentValidDelegatableFilterPredicateNode(string script, bool wa Assert.True(result.IsSuccess); - // Filter function can produce a 'WarnCheckPredicateUsage' warning. Any other warnings are unexpected. - var filteredErrors = result.Errors.Where(err => err.MessageKey != "WarnCheckPredicateUsage").ToList(); - if (warnings) { - Assert.True(filteredErrors.Count() > 0, "Expected warnings in original function"); + Assert.True(result.Errors.Count() > 0, "Expected warnings in original function"); } else { - Assert.False(filteredErrors.Count() > 0, "No warnings expected in original function"); + Assert.False(result.Errors.Count() > 0, "No warnings expected in original function"); } // then run with the mock filter function that does silent delgation checks From 4e08185d2cc8d15a80cbf2e56f76ea9b190e68e6 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Wed, 12 Mar 2025 16:33:23 -0500 Subject: [PATCH 07/13] Fixes. --- src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs | 2 +- .../AssociatedDataSourcesTests/TestDelegationValidation.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs index 098d36394f..0163f2ce4c 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs @@ -145,7 +145,7 @@ public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding) Contracts.AssertValue(callNode); Contracts.AssertValue(binding); - if (!CheckArgsCount(callNode, binding)) + if (!CheckArgsCount(callNode, binding, DocumentErrorSeverity.Moderate)) { return false; } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs index 31bbefd52b..fa10121da5 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs @@ -81,7 +81,7 @@ public void TestDelegableExpressions_ColumnNamesAsLiteralStrings(string expressi [Theory] [InlineData("UDF1()", "UDF1():Accounts = Accounts;", true)] - [InlineData("Filter(UDF1(), \"name\" <> \"\")", "UDF1():Accounts = Accounts;", false)] + [InlineData("Filter(UDF1(), \"name\" <> \"\")", "UDF1():Accounts = Accounts;", true)] public void TestDelegableExpressions_UserDfeinedFunction(string expression, string script, bool isDelegable) { TestDelegableExpressions(Features.PowerFxV1, expression, isDelegable, script); From b6647f87e181ec85539fc920c9ba9889d7350844 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Wed, 12 Mar 2025 16:51:03 -0500 Subject: [PATCH 08/13] Fixes. --- .../Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index b58ec0b733..8af9ac40b9 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -98,7 +98,6 @@ public FunctionScopeInfo( /// Caller call node. /// ArgN node. /// Calculated DType type. - /// /// List of data sources to compose the calculated type. /// public virtual bool CheckInput(Features features, CallNode callNode, TexlNode[] inputNodes, out DType typeScope, params DType[] inputSchema) From 7b5e3c3a449e41a61d2c9b932862c1261f45c49f Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Thu, 13 Mar 2025 11:11:09 -0500 Subject: [PATCH 09/13] Fixes. --- src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs | 8 +++++++- .../Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index ef7bce8f02..ee697db376 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -2067,7 +2067,13 @@ public DType GetUsedScopeFields(CallInfo call) var fError = false; var fieldName = name.Name; - call.CursorType.DisplayNameProvider?.TryGetLogicalName(name.Name, out fieldName); + var usesDisplayName = + DType.TryGetConvertedDisplayNameAndLogicalNameForColumn(call.CursorType, name.Name.Value, out var maybeLogicalName, out _) || + DType.TryGetLogicalNameForColumn(call.CursorType, name.Name.Value, out maybeLogicalName); + if (usesDisplayName) + { + fieldName = new DName(maybeLogicalName); + } if (!name.Node.InTree(arg0) && name.Node.InTree(call.Node) && diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index 8af9ac40b9..95a6b3f7e1 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -337,7 +337,7 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) public override void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) { // If Join call node has less than 5 records, we are possibly looking for suggestions. - if (callNode.Args.ChildNodes.Count < 5) + if (callNode.Args.ChildNodes.Count < 5 || fields == DType.Error) { return; } From 34d8f69431bcdb64b880e5560544c4058620757a Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Thu, 13 Mar 2025 16:14:39 -0500 Subject: [PATCH 10/13] Fixes. --- .../AssociatedDataSourcesTests/TestDelegationValidation.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs index fa10121da5..d012963ccc 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs @@ -149,7 +149,8 @@ public void TestCountRowsWarningForCachedData(bool isCachedData) // Only shows warning if data source is passed directly to CountRows result = engine.Check("CountRows(Filter(Accounts, IsBlank('Address 1: City')))"); - Assert.True(result.IsSuccess); + Assert.True(result.IsSuccess); + Assert.Empty(result.Errors); } } } From 2cabed11786675e6bdc76d18c7202a58ea5e76b5 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Fri, 14 Mar 2025 09:27:08 -0500 Subject: [PATCH 11/13] Adding LookUp function --- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 23 ++++------------- .../Texl/Builtins/Lookup.cs | 2 +- .../Types/DTypeExtensionsCore.cs | 25 +++++++++++++++++++ .../TexlTests.cs | 5 +++- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index ee697db376..a24da8879d 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -2065,15 +2065,8 @@ public DType GetUsedScopeFields(CallInfo call) foreach (var name in GetLambdaParamNames(call.ScopeNest + 1)) { var fError = false; - var fieldName = name.Name; - var usesDisplayName = - DType.TryGetConvertedDisplayNameAndLogicalNameForColumn(call.CursorType, name.Name.Value, out var maybeLogicalName, out _) || - DType.TryGetLogicalNameForColumn(call.CursorType, name.Name.Value, out maybeLogicalName); - if (usesDisplayName) - { - fieldName = new DName(maybeLogicalName); - } + call.CursorType.TryGetLogicalName(name.Name, out var fieldName); if (!name.Node.InTree(arg0) && name.Node.InTree(call.Node) && @@ -3138,8 +3131,8 @@ private bool IsRowScopeField(FirstNameNode node, out Scope scope, out bool fErro scope = default; return false; } - - var nodeName = node.Ident.Name; + + var nodeName = node.Ident.Name; // Look up the name in the current scopes, innermost to outermost. // The logic here is as follows: @@ -3161,14 +3154,8 @@ private bool IsRowScopeField(FirstNameNode node, out Scope scope, out bool fErro // If scope type is a data source, the node may be a display name instead of logical. // Attempt to get the logical name to use for type checking. // If this is executed amidst a metadata refresh then the reference may refer to an old - // display name, so we need to check the old mapping as well as the current mapping. - var usesDisplayName = - DType.TryGetConvertedDisplayNameAndLogicalNameForColumn(scope.Type, nodeName.Value, out var maybeLogicalName, out _) || - DType.TryGetLogicalNameForColumn(scope.Type, nodeName.Value, out maybeLogicalName); - if (usesDisplayName) - { - nodeName = new DName(maybeLogicalName); - } + // display name, so we need to check the old mapping as well as the current mapping. + scope.Type.TryGetLogicalName(node.Ident.Name, out nodeName); if (scope.Type.TryGetType(nodeName, out var typeTmp)) { diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Lookup.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Lookup.cs index 2440d591c9..1929b7ef0a 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Lookup.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Lookup.cs @@ -25,7 +25,7 @@ internal sealed class LookUpFunction : FilterFunctionBase public LookUpFunction() : base("LookUp", TexlStrings.AboutLookUp, FunctionCategories.Table, DType.Unknown, 0x6, 2, 3, DType.EmptyTable, DType.Boolean) { - ScopeInfo = new FunctionScopeInfo(this); + ScopeInfo = new FunctionScopeInfo(this, checkPredicateUsage: true); } public override IEnumerable GetSignatures() diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs index 2837e17c0a..d750ab227b 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs @@ -81,5 +81,30 @@ public static bool ContainsDataEntityType(this DType self, DPath path, int curre return self.GetNames(path).Any(n => n.Type.IsExpandEntity || (n.Type.IsAggregate && n.Type.ContainsDataEntityType(DPath.Root, currentDepth - 1))); } + + /// + /// Try to get the column logical name. + /// + /// + /// + /// + /// + public static bool TryGetLogicalName(this DType self, DName displayName, out DName logicalName) + { + var usesDisplayName = + DType.TryGetConvertedDisplayNameAndLogicalNameForColumn(self, displayName.Value, out var maybeLogicalName, out _) || + DType.TryGetLogicalNameForColumn(self, displayName.Value, out maybeLogicalName); + + if (usesDisplayName) + { + logicalName = new DName(maybeLogicalName); + } + else + { + logicalName = displayName; + } + + return usesDisplayName; + } } } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs index 55f2d579d8..6316c2a51e 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/TexlTests.cs @@ -4557,13 +4557,14 @@ public void TestTypeLiteralsNegative(string script, string expectedSchema) [InlineData("Join(t1 As X, t2 As Y, X.a = Y.x, JoinType.Inner, Y.z As Z)", false)] [InlineData("Summarize(t1, a)", false)] [InlineData("Summarize(t1, a, CountRows(ThisGroup) As Counter)", false)] - [InlineData("LookUp(t1, 1=1)", false)] // The 'LookUp' wont produce any warning because it has not been set to analyse the predicate. + [InlineData("LookUp(t1, a = 1)", false)] [InlineData("Max(t1, Abs(a))", false)] [InlineData("Min(t1, Abs(a))", false)] [InlineData("Average(t1, Abs(a))", false)] [InlineData("Sum(t1, Abs(a))", false)] [InlineData("Filter(t1, With({x:a}, StartsWith(x, \"something\")))", false)] [InlineData("Filter(t1 As Tbl, With({b:\"hello\"}, StartsWith(Tbl.b, b)))", false)] + [InlineData("If(LookUp(t1, a = 1).a = 1, 1)", false)] [InlineData("Filter(t1, 1=1)", true)] [InlineData("Filter(t1, With({b:\"hello\"}, StartsWith(b, \"something\")))", true)] @@ -4572,6 +4573,8 @@ public void TestTypeLiteralsNegative(string script, string expectedSchema) [InlineData("Min(t1, 1)", true)] [InlineData("Max(t1, 1)", true)] [InlineData("Join(t1, t2, true, JoinType.Inner, RightRecord.z As Z, LeftRecord.a As AAA)", true)] + [InlineData("LookUp(t1, 1=1)", true)] + [InlineData("LookUp(t1, With({a:99}, a) = 99)", true)] public void TestScopePredicate(string expression, bool expectingWarning) { From a5feab4fe3c320b3e33a1892f4e9d172cba8fdf1 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Fri, 14 Mar 2025 10:43:19 -0500 Subject: [PATCH 12/13] PR feedback --- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 14 ++++++++++++-- .../Types/DTypeExtensionsCore.cs | 6 ++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index a24da8879d..2e0144b3d8 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -2065,8 +2065,13 @@ public DType GetUsedScopeFields(CallInfo call) foreach (var name in GetLambdaParamNames(call.ScopeNest + 1)) { var fError = false; + var fieldName = name.Name; + var foundLogicalName = call.CursorType.TryGetLogicalName(name.Name, out var logicalName); - call.CursorType.TryGetLogicalName(name.Name, out var fieldName); + if (foundLogicalName) + { + fieldName = logicalName; + } if (!name.Node.InTree(arg0) && name.Node.InTree(call.Node) && @@ -3155,7 +3160,12 @@ private bool IsRowScopeField(FirstNameNode node, out Scope scope, out bool fErro // Attempt to get the logical name to use for type checking. // If this is executed amidst a metadata refresh then the reference may refer to an old // display name, so we need to check the old mapping as well as the current mapping. - scope.Type.TryGetLogicalName(node.Ident.Name, out nodeName); + var foundLoficalName = scope.Type.TryGetLogicalName(node.Ident.Name, out var logicalName); + + if (foundLoficalName) + { + nodeName = logicalName; + } if (scope.Type.TryGetType(nodeName, out var typeTmp)) { diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs index d750ab227b..beb3becc22 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs @@ -95,14 +95,12 @@ public static bool TryGetLogicalName(this DType self, DName displayName, out DNa DType.TryGetConvertedDisplayNameAndLogicalNameForColumn(self, displayName.Value, out var maybeLogicalName, out _) || DType.TryGetLogicalNameForColumn(self, displayName.Value, out maybeLogicalName); + logicalName = default; + if (usesDisplayName) { logicalName = new DName(maybeLogicalName); } - else - { - logicalName = displayName; - } return usesDisplayName; } From fbe58c8259009b7bc447b742d4b18109bbf8c2ab Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Mon, 17 Mar 2025 21:23:14 -0500 Subject: [PATCH 13/13] Handling view argument --- .../Microsoft.PowerFx.Core/Binding/Binder.cs | 3 +- .../Functions/FunctionScopeInfo.cs | 62 ++++++++++++++----- .../Texl/Builtins/Filter.cs | 2 +- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs index 2e0144b3d8..b5b2effcf1 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs @@ -1136,7 +1136,8 @@ public void CheckPredicateUsage(CallNode node, TexlFunction func) !node.Args.ChildNodes.Any(child => child is ErrorNode) && TryGetCall(node.Id, out var callInfo)) { - func.ScopeInfo.CheckPredicateFields(GetUsedScopeFields(callInfo), node, GetLambdaParamNames(callInfo.ScopeNest + 1), ErrorContainer); + //func.ScopeInfo.CheckPredicateFields(GetUsedScopeFields(callInfo), node, GetLambdaParamNames(callInfo.ScopeNest + 1), ErrorContainer); + func.ScopeInfo.CheckPredicateFields(this, callInfo); } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs index 95a6b3f7e1..0ab032ef76 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs @@ -217,23 +217,52 @@ public virtual bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) return false; } - public virtual void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) + public virtual void CheckPredicateFields(TexlBinding binding, CallInfo callInfo) { + var fields = binding.GetUsedScopeFields(callInfo); + var lambdaParamNames = binding.GetLambdaParamNames(callInfo.ScopeNest + 1); + if (fields == DType.Error || fields.GetAllNames(DPath.Root).Any()) { return; } - GetScopeIdent(callNode.Args.ChildNodes.ToArray(), out var idents); + GetScopeIdent(callInfo.Node.Args.ChildNodes.ToArray(), out var idents); - if (!lambdaNames.Any(lambdaName => idents.Contains(lambdaName.Name))) + if (!lambdaParamNames.Any(lambdaName => idents.Contains(lambdaName.Name))) { - errors.EnsureError(DocumentErrorSeverity.Warning, callNode, TexlStrings.WarnCheckPredicateUsage); + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Warning, callInfo.Node, TexlStrings.WarnCheckPredicateUsage); } } } - internal class FunctionThisGroupScopeInfo : FunctionScopeInfo + internal class FunctionFilterScopeInfo : FunctionScopeInfo + { + public FunctionFilterScopeInfo(TexlFunction function) + : base(function, checkPredicateUsage: true) + { + } + + public override void CheckPredicateFields(TexlBinding binding, CallInfo callInfo) + { + // Filter can also accept a view as argument. + // In the event of any argN (where N > 1) is a view, the binder will validate if the view is valid or not. + // We check if there is any view as argument. If there is, we just skip the predicate checking. + foreach (var childNode in callInfo.Node.Args.ChildNodes) + { + var argType = binding.GetType(childNode); + + if (argType.Kind == DKind.ViewValue) + { + return; + } + } + + base.CheckPredicateFields(binding, callInfo); + } + } + + internal class FunctionThisGroupScopeInfo : FunctionScopeInfo { public static DName ThisGroup => new DName("ThisGroup"); @@ -278,7 +307,7 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode i } } - internal class FunctionJoinScopeInfo : FunctionScopeInfo + internal class FunctionJoinScopeInfo : FunctionScopeInfo { public static DName LeftRecord => new DName("LeftRecord"); @@ -319,7 +348,7 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) { scopeIdents[0] = LeftRecord; } - + if (nodes.Length > 1 && nodes[1] is AsNode rightAsNode) { scopeIdents[1] = rightAsNode.Right.Name; @@ -327,29 +356,32 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents) else { scopeIdents[1] = RightRecord; - } + } // Returning false to indicate that the scope is not a whole scope. - // Meaning that the scope is a record type and we are accessing the fields directly. + // Meaning that the scope is a record type and we are accessing the fields directly. return false; } - public override void CheckPredicateFields(DType fields, CallNode callNode, IEnumerable lambdaNames, ErrorContainer errors) + public override void CheckPredicateFields(TexlBinding binding, CallInfo callInfo) { + var fields = binding.GetUsedScopeFields(callInfo); + var lambdaParamNames = binding.GetLambdaParamNames(callInfo.ScopeNest + 1); + // If Join call node has less than 5 records, we are possibly looking for suggestions. - if (callNode.Args.ChildNodes.Count < 5 || fields == DType.Error) + if (callInfo.Node.Args.ChildNodes.Count < 5 || fields == DType.Error) { return; } - GetScopeIdent(callNode.Args.ChildNodes.ToArray(), out var idents); + GetScopeIdent(callInfo.Node.Args.ChildNodes.ToArray(), out var idents); var foundIdents = new HashSet(); - var predicate = callNode.Args.ChildNodes[2]; + var predicate = callInfo.Node.Args.ChildNodes[2]; // In the Join function, arg2 and argN > 3 are lambdas nodes. // We need to check if scope identifiers are used arg2 (predicate). - foreach (var lambda in lambdaNames) + foreach (var lambda in lambdaParamNames) { var parent = lambda.Node.Parent; @@ -374,7 +406,7 @@ public override void CheckPredicateFields(DType fields, CallNode callNode, IEnum return; } - errors.EnsureError(DocumentErrorSeverity.Warning, callNode, TexlStrings.WarnCheckPredicateUsage); + binding.ErrorContainer.EnsureError(DocumentErrorSeverity.Warning, callInfo.Node, TexlStrings.WarnCheckPredicateUsage); } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs index 0163f2ce4c..e9a62f0f8f 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Filter.cs @@ -27,7 +27,7 @@ internal sealed class FilterFunction : FilterFunctionBase public FilterFunction() : base("Filter", TexlStrings.AboutFilter, FunctionCategories.Table, DType.EmptyTable, -2, 2, int.MaxValue, DType.EmptyTable) { - ScopeInfo = new FunctionScopeInfo(this, checkPredicateUsage: true); + ScopeInfo = new FunctionFilterScopeInfo(this); } public override IEnumerable GetSignatures()