From 233a31a8c4dedacbfb34b02c813ab47e40c1086f Mon Sep 17 00:00:00 2001 From: Anderson Silva Date: Fri, 5 Jul 2024 08:36:02 -0500 Subject: [PATCH] Stackoverflow when calling DType.ContainsDataEntityType (#2521) We are extending the range for https://github.com/microsoft/Power-Fx/issues/2456. --- .../Texl/Builtins/Collect.cs | 4 +- .../Texl/Intellisense/IntellisenseHelper.cs | 15 +---- .../Types/DTypeExtensionsCore.cs | 12 ++++ .../IntellisenseTests/SuggestTest.cs | 55 +--------------- .../LazyRecursiveRecordTypeHelper.cs | 62 +++++++++++++++++++ .../RecursiveRecordTypeTest.cs | 27 ++++++++ 6 files changed, 105 insertions(+), 70 deletions(-) create mode 100644 src/tests/Microsoft.PowerFx.Core.Tests.Shared/LazyRecursiveRecordTypeHelper.cs create mode 100644 src/tests/Microsoft.PowerFx.Core.Tests.Shared/RecursiveRecordTypeTest.cs diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs index 1626bb3a8..297315def 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs @@ -164,7 +164,7 @@ namespace Microsoft.PowerFx.Core.Texl.Builtins } // We only support accessing entities in collections if the collection has only 1 argument that contributes to it's type - if (argc != 2 && itemType.ContainsDataEntityType(DPath.Root)) + if (argc != 2 && itemType.ContainsDataEntityType(DPath.Root, itemType.MaxDepth)) { fValid &= DropAllOfKindNested(ref itemType, errors, args[i], DKind.DataEntity); } @@ -237,7 +237,7 @@ namespace Microsoft.PowerFx.Core.Texl.Builtins } // We only support accessing entities in collections if the collection has only 1 argument that contributes to it's type - if (argc != 2 && itemType.ContainsDataEntityType(DPath.Root)) + if (argc != 2 && itemType.ContainsDataEntityType(DPath.Root, itemType.MaxDepth)) { fValid &= DropAllOfKindNested(ref itemType, errors, args[i], DKind.DataEntity); } diff --git a/src/libraries/Microsoft.PowerFx.Core/Texl/Intellisense/IntellisenseHelper.cs b/src/libraries/Microsoft.PowerFx.Core/Texl/Intellisense/IntellisenseHelper.cs index c99d45257..4fab6d36a 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Texl/Intellisense/IntellisenseHelper.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Texl/Intellisense/IntellisenseHelper.cs @@ -343,7 +343,7 @@ namespace Microsoft.PowerFx.Intellisense { if (info.Function.IsLambdaParam(null, argPosition) && !info.Function.HasSuggestionsForParam(argPosition) && type.IsValid) { - if (ContainsDataEntityType(type, type.MaxDepth)) + if (type.ContainsDataEntityType(DPath.Root, type.MaxDepth)) { var error = false; type = type.DropAllOfTableRelationships(ref error, DPath.Root); @@ -898,18 +898,5 @@ namespace Microsoft.PowerFx.Intellisense var colonLen = TexlLexer.PunctuatorColon.Length; return script.Length >= cursorPos + colonLen && script.Substring(cursorPos, colonLen) == TexlLexer.PunctuatorColon; } - - internal static bool ContainsDataEntityType(DType type, int currentDepth) - { - Contracts.AssertValid(type); - - if (currentDepth < 1) - { - return false; - } - - return type.GetNames(DPath.Root).Any(n => n.Type.IsExpandEntity || - (n.Type.IsAggregate && ContainsDataEntityType(n.Type, currentDepth - 1))); - } } } diff --git a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs index dd7d9ed9e..2837e17c0 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Types/DTypeExtensionsCore.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +using System.Linq; using Microsoft.PowerFx.Core.Entities; using Microsoft.PowerFx.Core.Entities.QueryOptions; using Microsoft.PowerFx.Core.Utils; @@ -69,5 +70,16 @@ namespace Microsoft.PowerFx.Core.Types metaFieldType = control; return true; } + + public static bool ContainsDataEntityType(this DType self, DPath path, int currentDepth) + { + if (currentDepth < 1) + { + return false; + } + + return self.GetNames(path).Any(n => n.Type.IsExpandEntity || + (n.Type.IsAggregate && n.Type.ContainsDataEntityType(DPath.Root, currentDepth - 1))); + } } } 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 018849f08..bc346dab9 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/IntellisenseTests/SuggestTest.cs @@ -7,6 +7,7 @@ using System.Globalization; using System.Linq; using Microsoft.PowerFx.Core; using Microsoft.PowerFx.Core.Functions; +using Microsoft.PowerFx.Core.Tests; using Microsoft.PowerFx.Core.Texl; using Microsoft.PowerFx.Core.Types; using Microsoft.PowerFx.Core.Types.Enums; @@ -555,59 +556,5 @@ namespace Microsoft.PowerFx.Tests.IntellisenseTests // Just check that the execution didn't stack overflow. Assert.True(suggestions.Any()); } - - private class LazyRecursiveRecordType : RecordType - { - public override IEnumerable FieldNames => GetFieldNames(); - - public bool EnumerableIterated = false; - - public LazyRecursiveRecordType() - : base() - { - } - - public override bool TryGetFieldType(string name, out FormulaType type) - { - switch (name) - { - case "SomeString": - type = FormulaType.String; - return true; - case "TableLoop": - type = ToTable(); - return true; - case "Loop": - type = this; - return true; - case "Record": - type = RecordType.Empty().Add("Foo", FormulaType.Number); - return true; - default: - type = FormulaType.Blank; - return false; - } - } - - private IEnumerable GetFieldNames() - { - EnumerableIterated = true; - - yield return "SomeString"; - yield return "Loop"; - yield return "Record"; - yield return "TableLoop"; - } - - public override bool Equals(object other) - { - return other is LazyRecursiveRecordType; // All the same - } - - public override int GetHashCode() - { - return 1; - } - } } } diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/LazyRecursiveRecordTypeHelper.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/LazyRecursiveRecordTypeHelper.cs new file mode 100644 index 000000000..96291d7e7 --- /dev/null +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/LazyRecursiveRecordTypeHelper.cs @@ -0,0 +1,62 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using System.Collections.Generic; +using Microsoft.PowerFx.Types; + +namespace Microsoft.PowerFx.Core.Tests +{ + public class LazyRecursiveRecordType : RecordType + { + public override IEnumerable FieldNames => GetFieldNames(); + + public bool EnumerableIterated = false; + + public LazyRecursiveRecordType() + : base() + { + } + + public override bool TryGetFieldType(string name, out FormulaType type) + { + switch (name) + { + case "SomeString": + type = FormulaType.String; + return true; + case "TableLoop": + type = ToTable(); + return true; + case "Loop": + type = this; + return true; + case "Record": + type = RecordType.Empty().Add("Foo", FormulaType.Number); + return true; + default: + type = FormulaType.Blank; + return false; + } + } + + private IEnumerable GetFieldNames() + { + EnumerableIterated = true; + + yield return "SomeString"; + yield return "Loop"; + yield return "Record"; + yield return "TableLoop"; + } + + public override bool Equals(object other) + { + return other is LazyRecursiveRecordType; // All the same + } + + public override int GetHashCode() + { + return 1; + } + } +} diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/RecursiveRecordTypeTest.cs b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/RecursiveRecordTypeTest.cs new file mode 100644 index 000000000..662a7dac5 --- /dev/null +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/RecursiveRecordTypeTest.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using Microsoft.CodeAnalysis; +using Microsoft.PowerFx.Core.Texl.Builtins; +using Microsoft.PowerFx.Syntax; +using Xunit; + +namespace Microsoft.PowerFx.Core.Tests +{ + public class RecursiveTypeTest : PowerFxTest + { + [Theory] + [InlineData("Collect(TableLoop, First(TableLoop), First(TableLoop))")] + public void NoStackoverflowTest(string expression) + { + var symbols = ReadOnlySymbolTable.NewFromRecord(new LazyRecursiveRecordType()); + var engine = new Engine(); + engine.Config.SymbolTable.AddFunction(new CollectFunction()); + + var check = engine.Check(expression, symbolTable: symbols, options: new ParserOptions() { AllowsSideEffects = true }); + + // No stackoverflow has been thrown. + Assert.False(check.IsSuccess); + } + } +}