From 50d85020b170adb3d99a59cc37d0ca61364dc249 Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Tue, 16 Jan 2024 19:39:20 -0600 Subject: [PATCH 1/5] Adding test case and tracking the code that caused the problem closes #1130 --- .../TestFiles/cs/MemberChains.test | 4 ++ .../InvocationExpression.cs | 58 ++++++++++++++----- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test index 86d8d5412..de22b143a 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test @@ -245,5 +245,9 @@ class ClassName IEnumerable valueProviderFactories = new ModelBinderAttribute_______().GetValueProviderFactories(config); + + var something________________________________________ = x.SomeProperty + .CallMethod(longParameter_____________, longParameter_____________) + .CallMethod(); } } diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index 0cc74f751..9b2ce0850 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -229,16 +229,14 @@ List printedNodes // will be grouped as // [ // [Identifier, InvocationExpression], - // [MemberAccessExpression] - // [MemberAccessExpression, InvocationExpression], + // [MemberAccessExpression], [MemberAccessExpression, InvocationExpression], // [MemberAccessExpression, InvocationExpression], // [MemberAccessExpression], // ] // so that we can print it as // a() - // .b - // .c() + // .b.c() // .d() // .e @@ -260,16 +258,45 @@ List printedNodes } } - if ( - printedNodes[0].Node is not (InvocationExpressionSyntax or PostfixUnaryExpressionSyntax) - && index < printedNodes.Count - && printedNodes[index].Node - is ElementAccessExpressionSyntax - or PostfixUnaryExpressionSyntax - ) + if (printedNodes[0].Node is not InvocationExpressionSyntax) { - currentGroup.Add(printedNodes[index]); - index++; + for (; index + 1 < printedNodes.Count; ++index) + { + /* this handles the special case where we want ?.Property on the same line + someThing_______________________?.Property + .CallMethod__________________() + .CallMethod__________________(); + */ + if ( + printedNodes[index].Node is ConditionalAccessExpressionSyntax + && printedNodes[index + 1].Node + is MemberBindingExpressionSyntax { Parent: MemberAccessExpressionSyntax } + ) + { + currentGroup.Add(printedNodes[index]); + currentGroup.Add(printedNodes[index + 1]); + index++; + continue; + } + + if ( + ( + IsMemberish(printedNodes[index].Node) + && ( + IsMemberish(printedNodes[index + 1].Node) + || printedNodes[index + 1].Node is PostfixUnaryExpressionSyntax + ) + ) + || printedNodes[index].Node is PostfixUnaryExpressionSyntax + ) + { + currentGroup.Add(printedNodes[index]); + } + else + { + break; + } + } } groups.Add(currentGroup); @@ -312,6 +339,11 @@ or MemberBindingExpressionSyntax return groups; } + private static bool IsMemberish(CSharpSyntaxNode node) + { + return node is MemberAccessExpressionSyntax or ConditionalAccessExpressionSyntax; + } + private static Doc PrintIndentedGroup(ExpressionSyntax node, IList> groups) { if (groups.Count == 0) From 6260db7391486eb4a282b525f0827607f0862488 Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Sun, 21 Jan 2024 14:35:27 -0600 Subject: [PATCH 2/5] Fixing regression with invocations --- .../cs/MemberChain_PropertiesConsistent.test | 7 +- .../TestFiles/cs/MemberChains.test | 7 +- .../InvocationExpression.cs | 82 +++++++------------ 3 files changed, 35 insertions(+), 61 deletions(-) diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test index 22452adda..20fca8f39 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test @@ -28,9 +28,10 @@ var someVariable = this.Property.CallMethod(someValue => var someVariable = this.Property() .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); -var someVariable = this.Property.CallMethod(someValue => - someValue.SomeProperty == someOtherValue___________________________ -) +var someVariable = this + .Property.CallMethod(someValue => + someValue.SomeProperty == someOtherValue___________________________ + ) .CallMethod(); var someVariable = this.Property() diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test index de22b143a..e3e13cfe1 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test @@ -235,8 +235,7 @@ class ClassName .CallMethod__________________(); someThing_______________________ - ?.Property - .CallMethod__________________() + ?.Property.CallMethod__________________() .CallMethod__________________(); someThing_______________________ @@ -246,8 +245,8 @@ class ClassName IEnumerable valueProviderFactories = new ModelBinderAttribute_______().GetValueProviderFactories(config); - var something________________________________________ = x.SomeProperty - .CallMethod(longParameter_____________, longParameter_____________) + var something________________________________________ = x + .SomeProperty.CallMethod(longParameter_____________, longParameter_____________) .CallMethod(); } } diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index 9b2ce0850..a7bd80e53 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -198,7 +198,7 @@ private static List> GroupPrintedNodesOnLines(List(); + currentGroup = []; groups.Add(currentGroup); } else if ( @@ -209,7 +209,7 @@ or IdentifierNameSyntax && printedNodes[index + -1].Node is not ConditionalAccessExpressionSyntax ) { - currentGroup = new List(); + currentGroup = []; groups.Add(currentGroup); } @@ -258,49 +258,20 @@ List printedNodes } } - if (printedNodes[0].Node is not InvocationExpressionSyntax) + if ( + printedNodes[0].Node is not (InvocationExpressionSyntax or PostfixUnaryExpressionSyntax) + && index < printedNodes.Count + && printedNodes[index].Node + is ElementAccessExpressionSyntax + or PostfixUnaryExpressionSyntax + ) { - for (; index + 1 < printedNodes.Count; ++index) - { - /* this handles the special case where we want ?.Property on the same line - someThing_______________________?.Property - .CallMethod__________________() - .CallMethod__________________(); - */ - if ( - printedNodes[index].Node is ConditionalAccessExpressionSyntax - && printedNodes[index + 1].Node - is MemberBindingExpressionSyntax { Parent: MemberAccessExpressionSyntax } - ) - { - currentGroup.Add(printedNodes[index]); - currentGroup.Add(printedNodes[index + 1]); - index++; - continue; - } - - if ( - ( - IsMemberish(printedNodes[index].Node) - && ( - IsMemberish(printedNodes[index + 1].Node) - || printedNodes[index + 1].Node is PostfixUnaryExpressionSyntax - ) - ) - || printedNodes[index].Node is PostfixUnaryExpressionSyntax - ) - { - currentGroup.Add(printedNodes[index]); - } - else - { - break; - } - } + currentGroup.Add(printedNodes[index]); + index++; } groups.Add(currentGroup); - currentGroup = new List(); + currentGroup = []; var hasSeenNodeThatRequiresBreak = false; for (; index < printedNodes.Count; index++) @@ -313,17 +284,13 @@ or ConditionalAccessExpressionSyntax ) { groups.Add(currentGroup); - currentGroup = new List(); + currentGroup = []; hasSeenNodeThatRequiresBreak = false; } if ( printedNodes[index].Node - is ( - InvocationExpressionSyntax - or ElementAccessExpressionSyntax - or MemberBindingExpressionSyntax - ) + is (InvocationExpressionSyntax or ElementAccessExpressionSyntax) ) { hasSeenNodeThatRequiresBreak = true; @@ -339,11 +306,6 @@ or MemberBindingExpressionSyntax return groups; } - private static bool IsMemberish(CSharpSyntaxNode node) - { - return node is MemberAccessExpressionSyntax or ConditionalAccessExpressionSyntax; - } - private static Doc PrintIndentedGroup(ExpressionSyntax node, IList> groups) { if (groups.Count == 0) @@ -383,11 +345,23 @@ private static bool ShouldMergeFirstTwoGroups(List> groups) var firstNode = groups[0][0].Node; - if (firstNode is IdentifierNameSyntax { Identifier.Text.Length: <= 4 }) + if ( + firstNode + is IdentifierNameSyntax { Identifier.Text.Length: <= 4 } + or ThisExpressionSyntax + or PredefinedTypeSyntax + or BaseExpressionSyntax + && ( + groups[1].Count == 1 + || groups[1].Skip(1).First().Node + is InvocationExpressionSyntax + or PostfixUnaryExpressionSyntax + ) + ) { return true; } - return firstNode is ThisExpressionSyntax or PredefinedTypeSyntax or BaseExpressionSyntax; + return false; } } From 2c62f4fd0e8bc2439e3ab239f5b232117fcbd64e Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Sun, 21 Jan 2024 16:46:09 -0600 Subject: [PATCH 3/5] Fixing some edge cases --- .../cs/MemberChain_PropertiesConsistent.test | 11 ++- .../TestFiles/cs/MemberChains.test | 11 +++ .../TestFiles/cs/VariableDeclarations.test | 5 +- .../InvocationExpression.cs | 73 ++++++++++++------- 4 files changed, 67 insertions(+), 33 deletions(-) diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test index 20fca8f39..a11de94bc 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test @@ -25,7 +25,10 @@ var someVariable = this.Property.CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________ ); -var someVariable = this.Property() +var someVariable = this.CallMethod() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); + +var someVariable = this.Array[1] .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); var someVariable = this @@ -34,6 +37,10 @@ var someVariable = this ) .CallMethod(); -var someVariable = this.Property() +var someVariable = this.CallMethod() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) + .CallMethod(); + +var someVariable = this.Array[1] .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) .CallMethod(); diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test index e3e13cfe1..2ed8a7153 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test @@ -248,5 +248,16 @@ class ClassName var something________________________________________ = x .SomeProperty.CallMethod(longParameter_____________, longParameter_____________) .CallMethod(); + + CallMethod(o => + o.Property.CallMethod_____________________________________________________() + .CallMethod_____________________________________________________() + ); + + CallMethod( + o, + o.Property.CallMethod_____________________________________________________() + .CallMethod_____________________________________________________() + ); } } diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/VariableDeclarations.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/VariableDeclarations.test index 86b0fa116..a9396cf94 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/VariableDeclarations.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/VariableDeclarations.test @@ -80,9 +80,8 @@ class ClassName elementAccessExpression ].theMember______________________________; - var someLongValue_________________ = memberAccessExpression[ - elementAccessExpression - ].theMember______________________________(); + var someLongValue_________________ = memberAccessExpression[elementAccessExpression] + .theMember______________________________(); var someLongValue_________________ = memberAccessExpression( elementAccessExpression diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index a7bd80e53..e83e53ee3 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -18,6 +18,7 @@ public static Doc Print(InvocationExpressionSyntax node, FormattingContext conte public static Doc PrintMemberChain(ExpressionSyntax node, FormattingContext context) { + var parent = node.Parent; var printedNodes = new List(); FlattenAndPrintNodes(node, printedNodes, context); @@ -28,7 +29,7 @@ public static Doc PrintMemberChain(ExpressionSyntax node, FormattingContext cont var oneLine = groups.SelectMany(o => o).Select(o => o.Doc).ToArray(); - var shouldMergeFirstTwoGroups = ShouldMergeFirstTwoGroups(groups); + var shouldMergeFirstTwoGroups = ShouldMergeFirstTwoGroups(groups, parent); var cutoff = shouldMergeFirstTwoGroups ? 3 : 2; @@ -37,15 +38,17 @@ public static Doc PrintMemberChain(ExpressionSyntax node, FormattingContext cont && ( groups .Skip(shouldMergeFirstTwoGroups ? 1 : 0) - .Any(o => - o.Last().Node - is not ( - InvocationExpressionSyntax - or PostfixUnaryExpressionSyntax - { - Operand: InvocationExpressionSyntax - } - ) + .Any( + o => + o.Last().Node + is not ( + InvocationExpressionSyntax + or ElementAccessExpressionSyntax + or PostfixUnaryExpressionSyntax + { + Operand: InvocationExpressionSyntax + } + ) ) // if the last group contains just a !, make sure it doesn't end up on a new line || ( @@ -69,10 +72,11 @@ or PostfixUnaryExpressionSyntax return oneLine.Skip(1).Any(DocUtilities.ContainsBreak) - || groups[0].Any(o => - o.Node - is ArrayCreationExpressionSyntax - or ObjectCreationExpressionSyntax { Initializer: not null } + || groups[0].Any( + o => + o.Node + is ArrayCreationExpressionSyntax + or ObjectCreationExpressionSyntax { Initializer: not null } ) ? expanded : Doc.ConditionalGroup(Doc.Concat(oneLine), expanded); @@ -198,7 +202,7 @@ private static List> GroupPrintedNodesOnLines(List> groups) + private static bool ShouldMergeFirstTwoGroups( + List> groups, + SyntaxNode? parent + ) { if (groups.Count < 2 || groups[0].Count != 1) { @@ -347,17 +354,27 @@ private static bool ShouldMergeFirstTwoGroups(List> groups) if ( firstNode - is IdentifierNameSyntax { Identifier.Text.Length: <= 4 } - or ThisExpressionSyntax - or PredefinedTypeSyntax - or BaseExpressionSyntax - && ( - groups[1].Count == 1 - || groups[1].Skip(1).First().Node - is InvocationExpressionSyntax - or PostfixUnaryExpressionSyntax + is not ( + IdentifierNameSyntax { Identifier.Text.Length: <= 4 } + or ThisExpressionSyntax + or PredefinedTypeSyntax + or BaseExpressionSyntax ) ) + { + return false; + } + + // TODO maybe some things to fix in here + // https://github.com/belav/csharpier-repos/pull/100/files + if ( + groups[1].Count == 1 + || parent is SimpleLambdaExpressionSyntax or ArgumentSyntax + || groups[1].Skip(1).First().Node + is InvocationExpressionSyntax + or ElementAccessExpressionSyntax + or PostfixUnaryExpressionSyntax + ) { return true; } From 049225998168e260812047b54c31222fdd99cd34 Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Sun, 21 Jan 2024 17:03:15 -0600 Subject: [PATCH 4/5] One last edge case --- .../FormattingTests/TestFiles/cs/MemberChains.test | 5 +++++ .../SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test index 2ed8a7153..0bde1c590 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test @@ -259,5 +259,10 @@ class ClassName o.Property.CallMethod_____________________________________________________() .CallMethod_____________________________________________________() ); + + var someValue = + someValue + && o.Property.CallMethod_____________________________________________________() + .CallMethod_____________________________________________________(); } } diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index e83e53ee3..bffa4f609 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -369,7 +369,7 @@ or BaseExpressionSyntax // https://github.com/belav/csharpier-repos/pull/100/files if ( groups[1].Count == 1 - || parent is SimpleLambdaExpressionSyntax or ArgumentSyntax + || parent is SimpleLambdaExpressionSyntax or ArgumentSyntax or BinaryExpressionSyntax || groups[1].Skip(1).First().Node is InvocationExpressionSyntax or ElementAccessExpressionSyntax From e9a2176e0c1304a4f4f42b3b6315861d190f962b Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Sun, 21 Jan 2024 17:04:41 -0600 Subject: [PATCH 5/5] format file --- .../InvocationExpression.cs | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index bffa4f609..c2305d59d 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -38,17 +38,16 @@ public static Doc PrintMemberChain(ExpressionSyntax node, FormattingContext cont && ( groups .Skip(shouldMergeFirstTwoGroups ? 1 : 0) - .Any( - o => - o.Last().Node - is not ( - InvocationExpressionSyntax - or ElementAccessExpressionSyntax - or PostfixUnaryExpressionSyntax - { - Operand: InvocationExpressionSyntax - } - ) + .Any(o => + o.Last().Node + is not ( + InvocationExpressionSyntax + or ElementAccessExpressionSyntax + or PostfixUnaryExpressionSyntax + { + Operand: InvocationExpressionSyntax + } + ) ) // if the last group contains just a !, make sure it doesn't end up on a new line || ( @@ -72,11 +71,10 @@ or PostfixUnaryExpressionSyntax return oneLine.Skip(1).Any(DocUtilities.ContainsBreak) - || groups[0].Any( - o => - o.Node - is ArrayCreationExpressionSyntax - or ObjectCreationExpressionSyntax { Initializer: not null } + || groups[0].Any(o => + o.Node + is ArrayCreationExpressionSyntax + or ObjectCreationExpressionSyntax { Initializer: not null } ) ? expanded : Doc.ConditionalGroup(Doc.Concat(oneLine), expanded); @@ -202,7 +200,7 @@ private static List> GroupPrintedNodesOnLines(List