Skip to content

Commit 8188547

Browse files
macsuxmacsux
committed
Add fixes for NullSafeExpression that caused stackoverflow if indexer was used
1 parent 7e4c615 commit 8188547

File tree

8 files changed

+634
-548
lines changed

8 files changed

+634
-548
lines changed

‎Rewrite/src/Rewrite.CSharp/Marker/MemberBinding.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,14 @@
22

33
public record MemberBinding(Guid Id) : Core.Marker.Marker
44
{
5+
public Guid Id { get; init; } = Id;
6+
7+
public MemberBinding() : this(Core.Tree.RandomId())
8+
{
9+
}
10+
511
public virtual bool Equals(Core.Marker.Marker? other)
612
{
713
return other is MemberBinding && other.Id == Id;
814
}
9-
1015
}

‎Rewrite/src/Rewrite.CSharp/Parser/CSharpParserVisitor.cs‎

Lines changed: 47 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1507,16 +1507,25 @@ private J.Identifier MapIdentifier(SyntaxToken identifier, JavaType? type)
15071507
.FirstOrDefault(x => x.Markers.Contains<MemberBinding>());
15081508
if (bindingNode != null)
15091509
{
1510-
var newMarkers = bindingNode.Markers.WithMarkers(bindingNode.Markers.MarkerList.Where(x => x is not MemberBinding).ToList());
1510+
var newMarkers = bindingNode.Markers
1511+
.WithMarkers(bindingNode.Markers.MarkerList.Where(x => x is not MemberBinding).ToList());
15111512
if (bindingNode is J.MethodInvocation methodNode)
15121513
{
1513-
var newMethod = methodNode.WithSelect(currentExpression).WithMarkers(newMarkers);
1514+
var newMethod = methodNode
1515+
.WithSelect(currentExpression)
1516+
.WithMarkers(newMarkers);
15141517
lstNode = methodNode.Equals(lstNode) ? newMethod : lstNode.ReplaceNode(methodNode, newMethod);
15151518
}
15161519
else if (bindingNode is J.FieldAccess fieldAccess)
15171520
{
1518-
var newFieldAccess = fieldAccess.WithTarget(currentExpression).WithMarkers(newMarkers);;
1521+
var newFieldAccess = fieldAccess.WithTarget(currentExpression).WithMarkers(newMarkers);
15191522
lstNode = fieldAccess.Equals(lstNode) ? newFieldAccess : lstNode.ReplaceNode(fieldAccess, newFieldAccess);
1523+
} else if (bindingNode is J.ArrayAccess arrayAccess)
1524+
{
1525+
var newArrayAccess = arrayAccess
1526+
.WithIndexed(currentExpression)
1527+
.WithMarkers(newMarkers);
1528+
lstNode = newArrayAccess.Equals(lstNode) ? newArrayAccess : lstNode.ReplaceNode(lstNode, newArrayAccess);
15201529
}
15211530
}
15221531

@@ -1546,17 +1555,28 @@ private J.Identifier MapIdentifier(SyntaxToken identifier, JavaType? type)
15461555
// return base.VisitConditionalAccessExpression(node);
15471556
}
15481557

1558+
15491559
/// <summary>
15501560
/// Very similar to MemberAccessExpression, but doesn't have an expression portion - just identifier
15511561
/// Used in ConditionalAccessExpression since they are constructed left to right, then right to left like normal field access
15521562
/// </summary>
15531563
public override J? VisitMemberBindingExpression(MemberBindingExpressionSyntax node)
15541564
{
1565+
return new J.FieldAccess(
1566+
Core.Tree.RandomId(),
1567+
Format(Leading(node)),
1568+
Markers.Create(new MemberBinding()),
1569+
Convert<Expression>(node.Name)!,
1570+
Convert<J.Identifier>(node.Name)!.AsLeftPadded(Format(Leading(node.OperatorToken))),
1571+
MapType(node)
1572+
);
1573+
}
15551574

1556-
1557-
// due to the fact that the `ConditionalAccessExpressionSyntax` is at the root of an expression like `foo?.Bar.Baz`
1558-
// we need to find that root here, as the containment hierarchy using `J.FieldAccess` and `Cs.NullSafeExpression`
1559-
// ends up being very different
1575+
public override J? VisitElementBindingExpression(ElementBindingExpressionSyntax node)
1576+
{
1577+
// // due to the fact that the `ConditionalAccessExpressionSyntax` is at the root of an expression like `foo?.Bar.Baz`
1578+
// // we need to find that root here, as the containment hierarchy using `J.FieldAccess` and `Cs.NullSafeExpression`
1579+
// // ends up being very different
15601580
// ExpressionSyntax? parent = node;
15611581
// while (parent is not ConditionalAccessExpressionSyntax)
15621582
// if ((parent = parent.Parent as ExpressionSyntax) == null)
@@ -1575,53 +1595,34 @@ private J.Identifier MapIdentifier(SyntaxToken identifier, JavaType? type)
15751595
// )
15761596
// );
15771597

1578-
return new J.FieldAccess(
1598+
var placeholderExpression = new J.Empty(
15791599
Core.Tree.RandomId(),
15801600
Format(Leading(node)),
1581-
new Markers(
1582-
Core.Tree.RandomId(),
1583-
new List<Core.Marker.Marker>
1584-
{
1585-
new MemberBinding(Core.Tree.RandomId())
1586-
}),
1587-
Convert<Expression>(node.Name)!,
1588-
new JLeftPadded<J.Identifier>(
1589-
Format(Leading(node.OperatorToken)),
1590-
Convert<J.Identifier>(node.Name)!,
1591-
Markers.EMPTY
1592-
),
1593-
MapType(node)
1594-
);
1595-
}
1596-
1597-
public override J? VisitElementBindingExpression(ElementBindingExpressionSyntax node)
1598-
{
1599-
// due to the fact that the `ConditionalAccessExpressionSyntax` is at the root of an expression like `foo?.Bar.Baz`
1600-
// we need to find that root here, as the containment hierarchy using `J.FieldAccess` and `Cs.NullSafeExpression`
1601-
// ends up being very different
1602-
ExpressionSyntax? parent = node;
1603-
while (parent is not ConditionalAccessExpressionSyntax)
1604-
if ((parent = parent.Parent as ExpressionSyntax) == null)
1605-
throw new InvalidOperationException(
1606-
"Cannot find a `ConditionalAccessExpressionSyntax` in the containment hierarchy.");
1601+
Markers.Create(new MemberBinding()));
16071602

1608-
var conditionalAccess = (ConditionalAccessExpressionSyntax)parent;
1609-
var lhs = new Cs.NullSafeExpression(
1610-
Core.Tree.RandomId(),
1611-
Format(Leading(node)),
1612-
Markers.EMPTY,
1613-
new JRightPadded<Expression>(
1614-
Convert<Expression>(conditionalAccess.Expression)!,
1615-
Format(Leading(conditionalAccess.OperatorToken)),
1616-
Markers.EMPTY
1617-
)
1618-
);
1603+
// return new J.FieldAccess(
1604+
// Core.Tree.RandomId(),
1605+
// Format(Leading(node)),
1606+
// new Markers(
1607+
// Core.Tree.RandomId(),
1608+
// new List<Core.Marker.Marker>
1609+
// {
1610+
// new MemberBinding(Core.Tree.RandomId())
1611+
// }),
1612+
// Convert<Expression>(node.Name)!,
1613+
// new JLeftPadded<J.Identifier>(
1614+
// Format(Leading(node.OperatorToken)),
1615+
// Convert<J.Identifier>(node.Name)!,
1616+
// Markers.EMPTY
1617+
// ),
1618+
// MapType(node)
1619+
// );
16191620

16201621
return new J.ArrayAccess(
16211622
Core.Tree.RandomId(),
16221623
Format(Leading(node)),
16231624
Markers.EMPTY,
1624-
lhs,
1625+
placeholderExpression,
16251626
new J.ArrayDimension(
16261627
Core.Tree.RandomId(),
16271628
Format(Leading(node.ArgumentList.OpenBracketToken)),

‎Rewrite/src/Rewrite.Core/Marker/Markers.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ public record Markers(Guid Id, IList<Marker> MarkerList) : IReadOnlyCollection<M
77
{
88
public static readonly Markers EMPTY = new(Tree.RandomId(), ImmutableList<Marker>.Empty);
99

10+
public static Markers Create(params Marker[] markers)
11+
{
12+
return new Markers( Core.Tree.RandomId(), markers.ToImmutableList());
13+
}
14+
1015
public Markers WithId(Guid id)
1116
{
1217
return id == Id ? this : this with { Id = id };

‎Rewrite/src/Rewrite.Java/Tree/JLeftPadded.cs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,23 @@ public JLeftPadded<T> WithMarkers(Markers newMarkers)
4242
return ReferenceEquals(newMarkers, markers) ? this : new JLeftPadded<T>(before, element, newMarkers);
4343
}
4444
}
45-
45+
[PublicAPI]
4646
public static class JLeftPadded
4747
{
48+
[PublicAPI]
49+
public static JLeftPadded<T> AsLeftPadded<T>(this T element, Space before) => element.AsLeftPadded(before, Markers.EMPTY);
50+
51+
[PublicAPI]
52+
public static JLeftPadded<T> AsLeftPadded<T>(this T element, Space before, Markers markers)
53+
{
54+
return new JLeftPadded<T>(before, element, markers);
55+
}
56+
57+
[PublicAPI]
58+
public static JLeftPadded<T> Create<T>(T element, Space before, Markers markers)
59+
{
60+
return new JLeftPadded<T>(before, element, markers);
61+
}
4862
public record Location(Space.Location BeforeLocation)
4963
{
5064
public static readonly Location ASSERT_DETAIL = new(Space.Location.ASSERT_DETAIL_PREFIX);

‎Rewrite/tests/Rewrite.CSharp.Tests/PlayTests.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,32 @@ namespace Rewrite.CSharp.Tests.Tree;
1212
[Exploratory]
1313
public class PlayTests(ITestOutputHelper _output) : RewriteTest
1414
{
15+
/// <summary>
16+
/// Some pretty print tests for string delta report
17+
/// </summary>
18+
[Theory]
19+
[Exploratory]
20+
[InlineData("abc\n123","abc123")]
21+
[InlineData("abc123","abc134")]
22+
[InlineData("""
23+
one
24+
two
25+
three
26+
four
27+
NOT five
28+
""",
29+
"""
30+
one
31+
two
32+
four
33+
five
34+
six
35+
""")]
36+
public void Delta(string before, string after)
37+
{
38+
after.ShouldBeSameAs(before);
39+
}
40+
1541
[Fact]
1642
public void MyTest()
1743
{

‎Rewrite/tests/Rewrite.CSharp.Tests/RoslynTestCases/ParseTests.cs‎

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,29 +22,6 @@ public ParseTests(ITestOutputHelper output)
2222
_parser = new CSharpParser.Builder().Build();
2323
}
2424

25-
[Theory]
26-
[Exploratory]
27-
[InlineData("abc\n123","abc123")]
28-
[InlineData("abc123","abc134")]
29-
[InlineData("""
30-
one
31-
two
32-
three
33-
four
34-
NOT five
35-
""",
36-
"""
37-
one
38-
two
39-
four
40-
five
41-
six
42-
""")]
43-
public void Delta(string before, string after)
44-
{
45-
after.ShouldBeSameAs(before);
46-
}
47-
4825
[Fact]
4926
[Exploratory]
5027
public void TestReport()
@@ -58,7 +35,6 @@ public void TestReport()
5835
if (roslynDiagnostics.Any())
5936
{
6037
badTestCases.Add((testCase,roslynDiagnostics));
61-
// report.Add(testCase.Fail("Original.Parse.RoslynIssues"));
6238
continue;
6339
}
6440

@@ -111,11 +87,11 @@ public void TestReport()
11187
_output.WriteLine($" {failureReason}: {count}");
11288
}
11389

90+
_output.WriteLine("The following input testcases were skipped because they are not a fully valid syntax");
11491
foreach (var (testCase, diagnostics) in badTestCases)
11592
{
11693
_output.WriteLine($"\"{testCase.Name}\",");
117-
// _output.WriteLine(testCase.SourceText);
118-
// _output.WriteLine(string.Join("\n", diagnostics.Select(x => x.ToString())));
94+
_output.WriteLine(diagnostics.Select(x => x.GetMessage()).FirstOrDefault());
11995
}
12096
}
12197

0 commit comments

Comments
 (0)