Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use next sibling in SyntaxNode.GetChildPosition() if available #66876

Merged
merged 14 commits into from
Mar 8, 2023
Merged

Use next sibling in SyntaxNode.GetChildPosition() if available #66876

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6d82cde
Use next sibling in SyntaxNode.GetChildPosition() if available
cston Feb 14, 2023
86786a2
Update test
cston Feb 14, 2023
4bea78e
Add test
cston Feb 14, 2023
050f570
Fix build
cston Feb 14, 2023
14c639e
Verify child positions
cston Feb 15, 2023
95880f0
Add verification without cache
cston Feb 15, 2023
8f18f02
Update comments
cston Feb 15, 2023
9e8e9ce
Add comment
cston Feb 16, 2023
a98cb9a
Add tests with trailing separator
cston Feb 16, 2023
bff2282
Remove comment
cston Feb 16, 2023
528a2a8
Add comment
cston Feb 16, 2023
80f0ab8
Use GetCachedSlot()
cston Feb 16, 2023
724aecd
Call GetCachedSlot(index)
cston Feb 17, 2023
c187e3d
Update tests: add comments; increase iterations
cston Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/Compilers/CSharp/Test/Syntax/Syntax/SyntaxListTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,5 +305,44 @@ public void WithLotsOfChildrenTest()
}
}
}

[Fact]
public void EnumerateWithManyChildren_Forward()
{
const int n = 100000;
var builder = new StringBuilder();
builder.Append("int[] values = new[] { ");
for (int i = 0; i < n; i++) builder.Append("0, ");
builder.AppendLine("};");

var tree = CSharpSyntaxTree.ParseText(builder.ToString());
// Do not descend into InitializerExpressionSyntax since that will populate SeparatedWithManyChildren._children.
var node = tree.GetRoot().DescendantNodes().OfType<InitializerExpressionSyntax>().First();

foreach (var child in node.ChildNodesAndTokens())
{
_ = child.ToString();
}
}

[WorkItem(66475, "https://github.com/dotnet/roslyn/issues/66475")]
[Fact]
public void EnumerateWithManyChildren_Reverse()
{
const int n = 100000;
var builder = new StringBuilder();
builder.Append("int[] values = new[] { ");
for (int i = 0; i < n; i++) builder.Append("0, ");
builder.AppendLine("};");

var tree = CSharpSyntaxTree.ParseText(builder.ToString());
// Do not descend into InitializerExpressionSyntax since that will populate SeparatedWithManyChildren._children.
var node = tree.GetRoot().DescendantNodes().OfType<InitializerExpressionSyntax>().First();

foreach (var child in node.ChildNodesAndTokens().Reverse())
{
_ = child.ToString();
}
}
}
}
16 changes: 13 additions & 3 deletions src/Compilers/Core/Portable/Syntax/SyntaxList.SeparatedWithManyChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;

namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class SeparatedWithManyChildren : SyntaxList
internal sealed class SeparatedWithManyChildren : SyntaxList
{
private readonly ArrayElement<SyntaxNode?>[] _children;

Expand Down Expand Up @@ -39,6 +37,18 @@ internal SeparatedWithManyChildren(InternalSyntax.SyntaxList green, SyntaxNode?

return _children[i >> 1].Value;
}

internal override int GetChildPosition(int index)
{
// If the previous sibling (ignoring separator) is not cached, but the next sibling
// (ignoring separator) is cached, use the next sibling to determine position.
int valueIndex = (index & 1) != 0 ? index - 1 : index;
bool useNextNotPrevious = valueIndex > 1
&& _children[(valueIndex - 2) >> 1].Value is null
&& (valueIndex >= Green.SlotCount - 2 || _children[(valueIndex + 2) >> 1].Value is { });

return GetChildPosition(index, useNextNotPrevious);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i legit do not understand any of this :D

Copy link
Member Author

@cston cston Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decide whether to look at previous siblings or following siblings in GetChildPosition() by checking whether the nearest siblings are included in the _children cache. If the nearest previous sibling is not cached, but the nearest following sibling is cached, we'll look at the following siblings; otherwise, we'll use the existing behavior of looking at the previous siblings.

To check for the nearest siblings though, we need to ignore separators, because separators are not represented in the cache.

}
}
}
}
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxList.SeparatedWithManyWeakChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class SeparatedWithManyWeakChildren : SyntaxList
internal sealed class SeparatedWithManyWeakChildren : SyntaxList
{
private readonly ArrayElement<WeakReference<SyntaxNode>?>[] _children;

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxList.WithManyChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class WithManyChildren : SyntaxList
internal sealed class WithManyChildren : SyntaxList
{
private readonly ArrayElement<SyntaxNode?>[] _children;

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxList.WithManyWeakChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class WithManyWeakChildren : SyntaxList
internal sealed class WithManyWeakChildren : SyntaxList
{
private readonly ArrayElement<WeakReference<SyntaxNode>?>[] _children;

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxList.WithThreeChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class WithThreeChildren : SyntaxList
internal sealed class WithThreeChildren : SyntaxList
{
private SyntaxNode? _child0;
private SyntaxNode? _child1;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxList.WithTwoChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis.Syntax
{
internal partial class SyntaxList
{
internal class WithTwoChildren : SyntaxList
internal sealed class WithTwoChildren : SyntaxList
{
private SyntaxNode? _child0;
private SyntaxNode? _child1;
Expand Down
47 changes: 37 additions & 10 deletions src/Compilers/Core/Portable/Syntax/SyntaxNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,25 +611,52 @@ internal int GetChildIndex(int slot)
/// efficient implementations.
/// </summary>
internal virtual int GetChildPosition(int index)
{
return GetChildPosition(index, useNextNotPrevious: false);
}

internal int GetChildPosition(int index, bool useNextNotPrevious)
{
int offset = 0;
var green = this.Green;
while (index > 0)

if (!useNextNotPrevious)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: flip?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used !useNextNotPrevious to minimize the differences in the PR, and also because !useNextNotPrevious is the common case.

{
index--;
var prevSibling = this.GetCachedSlot(index);
if (prevSibling != null)
while (index > 0)
{
return prevSibling.EndPosition + offset;
index--;
var prevSibling = this.GetCachedSlot(index);
if (prevSibling != null)
{
return prevSibling.EndPosition + offset;
}
var greenChild = green.GetSlot(index);
if (greenChild != null)
{
offset += greenChild.FullWidth;
}
}
var greenChild = green.GetSlot(index);
if (greenChild != null)
return this.Position + offset;
}
else
{
int slotCount = green.SlotCount;
while (index < slotCount - 1)
{
offset += greenChild.FullWidth;
index++;
var prevSibling = this.GetCachedSlot(index);
if (prevSibling != null)
{
return prevSibling.EndPosition - offset;
}
var greenChild = green.GetSlot(index);
if (greenChild != null)
{
offset += greenChild.FullWidth;
}
}
return this.EndPosition - offset;
}

return this.Position + offset;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, i have no issue with the approach. but i think we def need tests around ensuring that reverse iterating produces nodes/tokens in the correct location. can you add a bunch of tests that show that if we reverse iterate the positions of things are all correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added verifying results from GetChildPosition() and GetChildPositionFromEnd() to tests in IOperation verification.


public Location GetLocation()
Expand Down
48 changes: 48 additions & 0 deletions src/Compilers/VisualBasic/Test/Syntax/Syntax/SyntaxListTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
' See the LICENSE file in the project root for more information.

Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
Imports Roslyn.Test.Utilities

Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
Public Class SyntaxListTests
Expand Down Expand Up @@ -241,5 +242,52 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
Next
End Sub

<Fact>
Public Sub EnumerateWithManyChildren_Forward()
Const n = 100000
Dim builder As New System.Text.StringBuilder()
builder.AppendLine("Module M")
builder.AppendLine(" Sub Main")
builder.Append(" Dim values As Integer() = {")
For i = 0 To n - 1
builder.Append("0, ")
Next
builder.AppendLine("}")
builder.AppendLine(" End Sub")
builder.AppendLine("End Module")

Dim tree = VisualBasicSyntaxTree.ParseText(builder.ToString())
' Do not descend into CollectionInitializerSyntax since that will populate SeparatedWithManyChildren._children.
Dim node = tree.GetRoot().DescendantNodes().OfType(Of CollectionInitializerSyntax)().First()

For Each child In node.ChildNodesAndTokens()
child.ToString()
Next
End Sub

<WorkItem(66475, "https://github.com/dotnet/roslyn/issues/66475")>
<Fact>
Public Sub EnumerateWithManyChildren_Reverse()
Const n = 100000
Dim builder As New System.Text.StringBuilder()
builder.AppendLine("Module M")
builder.AppendLine(" Sub Main")
builder.Append(" Dim values As Integer() = {")
For i = 0 To n - 1
builder.Append("0, ")
Next
builder.AppendLine("}")
builder.AppendLine(" End Sub")
builder.AppendLine("End Module")

Dim tree = VisualBasicSyntaxTree.ParseText(builder.ToString())
' Do not descend into CollectionInitializerSyntax since that will populate SeparatedWithManyChildren._children.
Dim node = tree.GetRoot().DescendantNodes().OfType(Of CollectionInitializerSyntax)().First()

For Each child In node.ChildNodesAndTokens()
child.ToString()
Next
End Sub

End Class
End Namespace