Skip to content

Commit cd436ac

Browse files
anderson-joyleanderson-joyle
committed
Check if predicate contains scope reference
1 parent 7760e40 commit cd436ac

File tree

10 files changed

+134
-160
lines changed

10 files changed

+134
-160
lines changed

‎src/libraries/Microsoft.PowerFx.Core/App/ErrorContainers/ErrorContainer.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ public bool HasErrors()
3737
return CollectionUtils.Size(_errors) > 0;
3838
}
3939

40+
public bool HasErrors(DocumentErrorSeverity severity)
41+
{
42+
if (_errors == null)
43+
{
44+
return false;
45+
}
46+
47+
return CollectionUtils.Size(_errors.Where(err => err.Severity >= severity)) > 0;
48+
}
49+
4050
public bool HasErrors(TexlNode node, DocumentErrorSeverity severity = DocumentErrorSeverity.Suggestion)
4151
{
4252
Contracts.AssertValue(node);
@@ -124,6 +134,17 @@ public IEnumerable<TexlError> GetErrors()
124134
}
125135
}
126136

137+
public IEnumerable<TexlError> GetErrors(DocumentErrorSeverity severity)
138+
{
139+
if (_errors != null)
140+
{
141+
foreach (var err in _errors.Where(err => err.Severity >= severity))
142+
{
143+
yield return err;
144+
}
145+
}
146+
}
147+
127148
public TexlError EnsureError(TexlNode node, ErrorResourceKey errKey, params object[] args)
128149
{
129150
return EnsureError(DefaultSeverity, node, errKey, args);

‎src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5326,7 +5326,7 @@ private void PreVisitBottomUp(CallNode node, int argCountVisited, Scope scopeNew
53265326

53275327
// If PreVisit resulted in errors for the node (and a non-null CallInfo),
53285328
// we're done -- we have a match and appropriate errors logged already.
5329-
if (_txb.ErrorContainer.HasErrors(node) || _txb.ErrorContainer.HasErrors(node.Head.Token))
5329+
if (_txb.ErrorContainer.HasErrors(node, DocumentErrorSeverity.Moderate) || _txb.ErrorContainer.HasErrors(node.Head.Token))
53305330
{
53315331
Contracts.Assert(info != null);
53325332

‎src/libraries/Microsoft.PowerFx.Core/Functions/FunctionScopeInfo.cs‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ internal class FunctionThisGroupScopeInfo : FunctionScopeInfo
251251
public static DName ThisGroup => new DName("ThisGroup");
252252

253253
public FunctionThisGroupScopeInfo(TexlFunction function)
254-
: base(function, appliesToArgument: (argIndex) => argIndex > 0)
254+
: base(function, appliesToArgument: (argIndex) => argIndex > 0, checkPredicateUsage: true)
255255
{
256256
}
257257

@@ -289,6 +289,26 @@ public override bool CheckInput(Features features, CallNode callNode, TexlNode i
289289

290290
return ret;
291291
}
292+
293+
public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors)
294+
{
295+
GetScopeIdent(inputNodes, out DName[] idents);
296+
297+
for (int i = 0; i < inputNodes.Length; i++)
298+
{
299+
if (_function.IsLambdaParam(inputNodes[i], i))
300+
{
301+
// We create a new visitor object for each iteration since lambda parms can appear at any place in the expression.
302+
var visitor = new ScopePredicateVisitor(typeScope, new[] { ThisGroup }, true);
303+
inputNodes[i].Accept(visitor);
304+
305+
if (!visitor.InUsePredicates.Contains(ThisGroup))
306+
{
307+
errors.EnsureError(DocumentErrorSeverity.Warning, inputNodes[i], TexlStrings.WarnCheckPredicateUsage, string.Join("/", idents.Select(id => id.Value)));
308+
}
309+
}
310+
}
311+
}
292312
}
293313

294314
internal class FunctionJoinScopeInfo : FunctionScopeInfo
@@ -354,7 +374,8 @@ public override bool GetScopeIdent(TexlNode[] nodes, out DName[] scopeIdents)
354374

355375
public override void CheckPredicates(TexlNode[] inputNodes, DType typeScope, IErrorContainer errors)
356376
{
357-
var wholeScope = GetScopeIdent(inputNodes, out DName[] idents);
377+
GetScopeIdent(inputNodes, out DName[] idents);
378+
358379
var visitor = new ScopePredicateVisitor(typeScope, idents);
359380

360381
inputNodes[2].Accept(visitor);

‎src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,6 @@ private static IntermediateNode ConcatenateArgs(IntermediateNode arg1, Intermedi
617617
}
618618

619619
var concatenatedNode = new CallNode(irContext, BuiltinFunctionsCore.Concatenate, concatenateArgs);
620-
throw new Exception("CHECKPOINT!");
621620
return concatenatedNode;
622621
}
623622

‎src/libraries/Microsoft.PowerFx.Core/Syntax/Visitors/ScopePredicateVisitor.cs‎

Lines changed: 20 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -11,141 +11,51 @@
1111

1212
namespace Microsoft.PowerFx.Core.Syntax.Visitors
1313
{
14-
internal class ScopePredicateVisitor : TexlVisitor
14+
internal class ScopePredicateVisitor : IdentityTexlVisitor
1515
{
1616
private readonly DType _typeScope;
1717
private readonly DName[] _idents;
1818

19+
// The Summarize function creates a 'ThisGroup' indentifier that can only be used inside a call.
20+
private bool _allowAggregateCall;
21+
1922
public HashSet<DName> InUsePredicates;
2023

21-
public ScopePredicateVisitor(DType typeScope, DName[] indents)
24+
public ScopePredicateVisitor(DType typeScope, DName[] indents, bool allowAggregateCall = false)
2225
{
2326
_typeScope = typeScope;
24-
25-
InUsePredicates = new HashSet<DName>();
2627
_idents = indents;
27-
}
28+
_allowAggregateCall = allowAggregateCall;
2829

29-
public override void Visit(TypeLiteralNode node)
30-
{
31-
return;
32-
}
33-
34-
public override void Visit(ErrorNode node)
35-
{
36-
return;
37-
}
38-
39-
public override void Visit(BlankNode node)
40-
{
41-
return;
42-
}
43-
44-
public override void Visit(BoolLitNode node)
45-
{
46-
return;
47-
}
48-
49-
public override void Visit(StrLitNode node)
50-
{
51-
return;
52-
}
53-
54-
public override void Visit(NumLitNode node)
55-
{
56-
return;
57-
}
58-
59-
public override void Visit(DecLitNode node)
60-
{
61-
return;
30+
InUsePredicates = new HashSet<DName>();
6231
}
6332

6433
public override void Visit(FirstNameNode node)
6534
{
66-
// We should reach this block only if we are working with an implicity 'ThisRecord'.
67-
if (_idents.Length == 1 && _typeScope.TryGetType(node.Ident.Name, out _))
35+
if (_idents.Contains(node.Ident.Name) || _typeScope.TryGetType(node.Ident.Name, out _))
6836
{
69-
InUsePredicates.Add(_idents.First());
37+
InUsePredicates.Add(node.Ident.Name);
7038
}
7139
}
7240

73-
// We can assume if visiting a DottedNameNode is always a whole scope.
7441
public override bool PreVisit(DottedNameNode node)
7542
{
76-
var left = node.Left.AsFirstName();
77-
78-
if (_idents.Contains(left.Ident.Name))
79-
{
80-
InUsePredicates.Add(left.Ident.Name);
81-
}
82-
83-
// No need to visit left/right separately.
43+
// No need to visit right node.
44+
node.Left.Accept(this);
8445
return false;
8546
}
8647

87-
public override void Visit(ParentNode node)
88-
{
89-
return;
90-
}
91-
92-
public override void Visit(SelfNode node)
93-
{
94-
return;
95-
}
96-
97-
public override void PostVisit(StrInterpNode node)
98-
{
99-
return;
100-
}
101-
102-
public override void PostVisit(DottedNameNode node)
103-
{
104-
return;
105-
}
106-
107-
public override void PostVisit(UnaryOpNode node)
108-
{
109-
return;
110-
}
111-
112-
public override void PostVisit(BinaryOpNode node)
113-
{
114-
return;
115-
}
116-
117-
public override void PostVisit(VariadicOpNode node)
118-
{
119-
return;
120-
}
121-
122-
public override void PostVisit(CallNode node)
123-
{
124-
return;
125-
}
126-
127-
public override void PostVisit(ListNode node)
128-
{
129-
return;
130-
}
131-
132-
public override void PostVisit(RecordNode node)
133-
{
134-
return;
135-
}
136-
137-
public override void PostVisit(TableNode node)
138-
{
139-
return;
140-
}
141-
142-
public override void PostVisit(AsNode node)
143-
{
144-
return;
145-
}
146-
14748
public override bool PreVisit(CallNode node)
14849
{
50+
if (_allowAggregateCall)
51+
{
52+
// If `ThisGroup` is placed inside multiple calls, raise a warning.
53+
// Example
54+
// Summarize(t1, Fruit, CountRows( Distinct( ThisGroup, Supplier ) ) As CountOfSuppliers )
55+
_allowAggregateCall = false;
56+
return true;
57+
}
58+
14959
return false;
15060
}
15161
}

‎src/libraries/Microsoft.PowerFx.Core/Utils/CollectionUtils.cs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System;
55
using System.Collections.Concurrent;
66
using System.Collections.Generic;
7+
using System.Linq;
78
using System.Threading;
89

910
namespace Microsoft.PowerFx.Core.Utils
@@ -36,16 +37,10 @@ public static int Size<T>(T[] rgv)
3637
return rgv == null ? 0 : rgv.Length;
3738
}
3839

39-
public static int Size<T>(List<T> list)
40+
public static int Size<T>(IEnumerable<T> enumerable)
4041
{
41-
Contracts.AssertValueOrNull(list);
42-
return list == null ? 0 : list.Count;
43-
}
44-
45-
public static int Size<T>(IList<T> list)
46-
{
47-
Contracts.AssertValueOrNull(list);
48-
return list == null ? 0 : list.Count;
42+
Contracts.AssertValueOrNull(enumerable);
43+
return enumerable == null ? 0 : enumerable.Count();
4944
}
5045

5146
public static int Size<T>(Stack<T> stack)

‎src/libraries/Microsoft.PowerFx.Interpreter/RecalcEngine.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,14 @@
88
using System.Text;
99
using System.Threading;
1010
using System.Threading.Tasks;
11-
using Microsoft.PowerFx.Core;
1211
using Microsoft.PowerFx.Core.Binding;
1312
using Microsoft.PowerFx.Core.Errors;
1413
using Microsoft.PowerFx.Core.Functions;
1514
using Microsoft.PowerFx.Core.Glue;
1615
using Microsoft.PowerFx.Core.Parser;
17-
using Microsoft.PowerFx.Core.Types;
1816
using Microsoft.PowerFx.Core.Utils;
1917
using Microsoft.PowerFx.Functions;
2018
using Microsoft.PowerFx.Interpreter;
21-
using Microsoft.PowerFx.Syntax;
2219
using Microsoft.PowerFx.Types;
2320

2421
namespace Microsoft.PowerFx
@@ -463,9 +460,11 @@ private void AddUserDefinedFunctions(IEnumerable<UDF> parsedUdfs, ReadOnlySymbol
463460

464461
List<TexlError> bindErrors = new List<TexlError>();
465462

466-
if (binding.ErrorContainer.GetErrors(ref bindErrors))
463+
var onlyErrors = binding.ErrorContainer.GetErrors(DocumentErrorSeverity.Moderate);
464+
465+
if (onlyErrors.Count() > 0)
467466
{
468-
sb.AppendLine(string.Join(", ", bindErrors.Select(err => err.ToString())));
467+
sb.AppendLine(string.Join(", ", onlyErrors.Select(err => err.ToString())));
469468
}
470469
else
471470
{

‎src/tests/Microsoft.PowerFx.Core.Tests.Shared/AssociatedDataSourcesTests/TestDelegationValidation.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,10 @@ public void TestCountRowsWarningForCachedData(bool isCachedData)
149149

150150
// Only shows warning if data source is passed directly to CountRows
151151
result = engine.Check("CountRows(Filter(Accounts, IsBlank('Address 1: City')))");
152-
Assert.True(result.IsSuccess);
153-
Assert.Empty(result.Errors);
152+
Assert.True(result.IsSuccess);
153+
154+
// Some functions (like 'Filter') can produce a 'WarnCheckPredicateUsage' warning. Any other warnings are unexpected.
155+
Assert.Empty(result.Errors.Where(err => err.MessageKey != "WarnCheckPredicateUsage"));
154156
}
155157
}
156158
}

‎src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -314,32 +314,6 @@ public void TestSuggestEscapedEnumName()
314314
Assert.Contains("'Name That.Requires!escaping'.Field2", result);
315315
}
316316

317-
[Theory]
318-
[InlineData("in", "in_text")]
319-
[InlineData("or", "or_text")]
320-
[InlineData("and", "and_text")]
321-
[InlineData("In", "In_text")]
322-
[InlineData("Or", "Or_text")]
323-
[InlineData("And", "And_text")]
324-
public void TesafdghghffgnfrghhhhhhhhhhhhhhhhhhhhhhhhhhhhpedEnumName(string expr, string varName)
325-
{
326-
var engine = new Engine();
327-
engine.Config.SymbolTable.AddVariable(varName, FormulaType.String);
328-
329-
var test = Features.PowerFxV1.GetType();
330-
var tese = test.GetProperty("SkipExpandableSetSemantics", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
331-
332-
foreach (var textFirst in new[] { false, true })
333-
{
334-
expr = textFirst ? "=" + expr : expr;
335-
336-
var check = engine.Check(expr, options: new ParserOptions() { TextFirst = textFirst });
337-
var suggestions = engine.Suggest(check, expr.Length);
338-
339-
Assert.Equal(textFirst ? 1 : 0, suggestions.ReplacementStartIndex);
340-
}
341-
}
342-
343317
[Theory]
344318
[InlineData("SortByColumns(|", 3, "The table to sort.", "SortByColumns(source, column, ...)")]
345319
[InlineData("SortByColumns(tbl1,|", 3, "A unique column name.", "SortByColumns(source, column, ...)")]

0 commit comments

Comments
 (0)