Skip to content

Commit abcf5d8

Browse files
ogaken-1ogaken-1
committed
fix: Null coalescing not own its line with complex left-hand side
There was an issue where if the left-hand side of `??` was even slightly complex, the right-hand side would become part of the group and could not have its own line. The grouping conditions were reviewed and changed from a threshold-based determination of left-hand side complexity to condition limited to cases where: - "The left-hand side is an InvocationExpression and consists solely of MemberAccessExpressions that do not contain InvocationExpressions" OR - "The left-hand side is a ParenthesizedExpression".
1 parent c47ec3c commit abcf5d8

File tree

2 files changed

+298
-57
lines changed

2 files changed

+298
-57
lines changed

‎Src/CSharpier.Core/CSharp/SyntaxPrinter/SyntaxNodePrinters/BinaryExpression.cs‎

Lines changed: 111 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,11 @@ private static List<Doc> PrintBinaryExpression(SyntaxNode node, PrintingContext
7373

7474
// This group ensures that something like == 0 does not end up on its own line
7575
var shouldGroup =
76-
binaryExpressionSyntax.Kind() != binaryExpressionSyntax.Parent!.Kind()
76+
(
77+
binaryExpressionSyntax.Kind() != SyntaxKind.CoalesceExpression
78+
|| ShouldGroupNullCoalescingSyntax(binaryExpressionSyntax)
79+
)
80+
&& binaryExpressionSyntax.Kind() != binaryExpressionSyntax.Parent!.Kind()
7781
&& GetPrecedence(binaryExpressionSyntax)
7882
!= GetPrecedence(binaryExpressionSyntax.Parent!)
7983
&& binaryExpressionSyntax.Left.GetType() != binaryExpressionSyntax.GetType()
@@ -84,62 +88,12 @@ private static List<Doc> PrintBinaryExpression(SyntaxNode node, PrintingContext
8488
var binaryOnTheRight = binaryExpressionSyntax.Kind() == SyntaxKind.CoalesceExpression;
8589
if (binaryOnTheRight)
8690
{
87-
var chain = 0;
88-
var possibleInvocation = binaryExpressionSyntax.Left;
89-
while (possibleInvocation is not null)
90-
{
91-
if (possibleInvocation is InvocationExpressionSyntax invocationExpression)
92-
{
93-
possibleInvocation = invocationExpression.Expression;
94-
chain++;
95-
}
96-
else if (
97-
possibleInvocation
98-
is MemberAccessExpressionSyntax memberAccessExpressionSyntax
99-
)
100-
{
101-
possibleInvocation = memberAccessExpressionSyntax.Expression;
102-
chain++;
103-
}
104-
else if (
105-
possibleInvocation
106-
is ConditionalAccessExpressionSyntax conditionalAccessExpressionSyntax
107-
)
108-
{
109-
possibleInvocation = conditionalAccessExpressionSyntax.Expression;
110-
chain++;
111-
}
112-
else if (
113-
possibleInvocation
114-
is ElementAccessExpressionSyntax elementAccessExpressionSyntax
115-
)
116-
{
117-
possibleInvocation = elementAccessExpressionSyntax.Expression;
118-
chain++;
119-
}
120-
else
121-
{
122-
possibleInvocation = null;
123-
}
124-
}
125-
var leftDoc = Node.Print(binaryExpressionSyntax.Left, context);
126-
if (chain > 3)
127-
{
128-
docs.Add(
129-
Doc.Group(leftDoc, Doc.Line),
130-
Token.Print(binaryExpressionSyntax.OperatorToken, context),
131-
" "
132-
);
133-
}
134-
else
135-
{
136-
docs.Add(
137-
leftDoc,
138-
Doc.Line,
139-
Token.Print(binaryExpressionSyntax.OperatorToken, context),
140-
" "
141-
);
142-
}
91+
docs.Add(
92+
Node.Print(binaryExpressionSyntax.Left, context),
93+
Doc.Line,
94+
Token.Print(binaryExpressionSyntax.OperatorToken, context),
95+
" "
96+
);
14397
}
14498

14599
var possibleBinary = binaryOnTheRight
@@ -187,6 +141,106 @@ possibleBinary is BinaryExpressionSyntax childBinary
187141
}
188142
}
189143

144+
private static bool ShouldGroupNullCoalescingSyntax(
145+
BinaryExpressionSyntax binaryExpressionSyntax
146+
)
147+
{
148+
if (binaryExpressionSyntax.Kind() != SyntaxKind.CoalesceExpression)
149+
{
150+
return false;
151+
}
152+
153+
var left = binaryExpressionSyntax.Left;
154+
return IsTerminalInvocation(left)
155+
|| left is ParenthesizedExpressionSyntax
156+
|| left is CastExpressionSyntax { Expression: ParenthesizedExpressionSyntax }
157+
|| left is SwitchExpressionSyntax;
158+
159+
static bool IsTerminalInvocation(ExpressionSyntax expression)
160+
{
161+
return expression switch
162+
{
163+
InvocationExpressionSyntax invocation =>
164+
!HasInvocationExcludingLeftmostParenthesized(invocation.Expression),
165+
ConditionalAccessExpressionSyntax conditionalAccess =>
166+
IsTerminalInvocationInWhenNotNull(conditionalAccess.WhenNotNull)
167+
&& !HasInvocationExcludingLeftmostParenthesized(
168+
conditionalAccess.Expression
169+
),
170+
AwaitExpressionSyntax awaitExpression => IsTerminalInvocation(
171+
awaitExpression.Expression
172+
),
173+
_ => false,
174+
};
175+
}
176+
177+
static bool IsTerminalInvocationInWhenNotNull(ExpressionSyntax whenNotNull)
178+
{
179+
var rightmost = GetRightmostExpression(whenNotNull);
180+
if (rightmost is not InvocationExpressionSyntax)
181+
{
182+
return false;
183+
}
184+
return !whenNotNull
185+
.DescendantNodesAndSelf()
186+
.OfType<InvocationExpressionSyntax>()
187+
.Where(invocation => invocation != rightmost)
188+
.Any();
189+
}
190+
191+
static ExpressionSyntax GetRightmostExpression(ExpressionSyntax expression)
192+
{
193+
return expression switch
194+
{
195+
InvocationExpressionSyntax invocation => invocation,
196+
ConditionalAccessExpressionSyntax conditional => GetRightmostExpression(
197+
conditional.WhenNotNull
198+
),
199+
MemberAccessExpressionSyntax memberAccess => GetRightmostExpression(
200+
memberAccess.Expression
201+
),
202+
_ => expression,
203+
};
204+
}
205+
206+
static bool HasInvocationExcludingLeftmostParenthesized(ExpressionSyntax expression)
207+
{
208+
var leftmost = GetLeftmostExpression(expression);
209+
return expression
210+
.DescendantNodesAndSelf()
211+
.Where(node =>
212+
leftmost is not ParenthesizedExpressionSyntax excludedNode
213+
|| !IsDescendantOrSelf(node, excludedNode)
214+
)
215+
.Any(node =>
216+
node is InvocationExpressionSyntax or ConditionalAccessExpressionSyntax
217+
);
218+
}
219+
220+
static ExpressionSyntax GetLeftmostExpression(ExpressionSyntax expression)
221+
{
222+
return expression switch
223+
{
224+
MemberAccessExpressionSyntax memberAccess => GetLeftmostExpression(
225+
memberAccess.Expression
226+
),
227+
InvocationExpressionSyntax invocation => GetLeftmostExpression(
228+
invocation.Expression
229+
),
230+
ConditionalAccessExpressionSyntax conditional => GetLeftmostExpression(
231+
conditional.Expression
232+
),
233+
AwaitExpressionSyntax awaitExpr => GetLeftmostExpression(awaitExpr.Expression),
234+
_ => expression,
235+
};
236+
}
237+
238+
static bool IsDescendantOrSelf(SyntaxNode node, SyntaxNode ancestor)
239+
{
240+
return node == ancestor || node.Ancestors().Contains(ancestor);
241+
}
242+
}
243+
190244
private static bool ShouldFlatten(SyntaxToken parentToken, SyntaxToken nodeToken)
191245
{
192246
return GetPrecedence(parentToken) == GetPrecedence(nodeToken);

‎Src/CSharpier.Tests/FormattingTests/TestFiles/cs/BinaryExpressions.test‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,12 +200,163 @@ class TestClass
200200
someLongValue__________________________________________
201201
) ?? someOtherValue;
202202

203+
var x =
204+
someValue
205+
.Property.CallLongMethod_____________________________________()
206+
.CallMethod(
207+
someLongValue__________________________________________,
208+
someLongValue__________________________________________
209+
)
210+
?? someOtherValue;
211+
212+
var x =
213+
someValue.CallMethod(
214+
someLongValue__________________________________________,
215+
someLongValue__________________________________________
216+
) ?? someOtherValue;
217+
218+
var x =
219+
someValue?.CallMethod(
220+
someLongValue__________________________________________,
221+
someLongValue__________________________________________
222+
) ?? someOtherValue;
223+
224+
var x =
225+
(
226+
CallMethod(
227+
someLongValue__________________________________________,
228+
someLongValue__________________________________________
229+
)
230+
) ?? someOtherValue;
231+
232+
var x =
233+
(
234+
CallMethod(
235+
someLongValue__________________________________________,
236+
someLongValue__________________________________________
237+
)
238+
).CallMethod() ?? someOtherValue;
239+
240+
var x =
241+
(
242+
CallMethod(
243+
someLongValue__________________________________________,
244+
someLongValue__________________________________________
245+
)
246+
)?.CallMethod() ?? someOtherValue;
247+
248+
var x =
249+
(
250+
CallMethod(
251+
someLongValue__________________________________________,
252+
someLongValue__________________________________________
253+
)
254+
)?.CallMethod(
255+
someLongValue__________________________________________,
256+
someLongValue__________________________________________
257+
) ?? someOtherValue;
258+
259+
var x =
260+
(
261+
CallMethod(
262+
someLongValue__________________________________________,
263+
someLongValue__________________________________________
264+
)
265+
)
266+
.CallMethod()
267+
.CallMethod()
268+
?? someOtherValue;
269+
270+
var x =
271+
(
272+
CallMethod(
273+
someLongValue__________________________________________,
274+
someLongValue__________________________________________
275+
)
276+
).SomeProperty.CallMethod() ?? someOtherValue;
277+
278+
var x =
279+
(
280+
CallMethod(
281+
someLongValue__________________________________________,
282+
someLongValue__________________________________________
283+
)
284+
)?.SomeProperty.CallMethod() ?? someOtherValue;
285+
286+
var x =
287+
(
288+
CallMethod(
289+
someLongValue__________________________________________,
290+
someLongValue__________________________________________
291+
)
292+
)?.SomeProperty?.CallMethod() ?? someOtherValue;
293+
294+
var x =
295+
(
296+
CallMethod(
297+
someLongValue__________________________________________,
298+
someLongValue__________________________________________
299+
)
300+
)
301+
.SomeProperty.CallMethod()
302+
.CallMethod()
303+
?? someOtherValue;
304+
305+
var x =
306+
(
307+
CallMethod(
308+
someLongValue__________________________________________,
309+
someLongValue__________________________________________
310+
)
311+
)
312+
?.SomeProperty.CallMethod()
313+
.CallMethod()
314+
?? someOtherValue;
315+
316+
var x =
317+
await CallMethodAsync(
318+
someLongValue__________________________________________,
319+
someLongValue__________________________________________
320+
) ?? someOtherValue;
321+
322+
var x =
323+
await SomeObject.CallMethodAsync(
324+
someLongValue__________________________________________,
325+
someLongValue__________________________________________
326+
) ?? someOtherValue;
327+
328+
var x =
329+
await SomeObject
330+
.CallMethod()
331+
.CallMethodAsync(
332+
someLongValue__________________________________________,
333+
someLongValue__________________________________________
334+
)
335+
?? someOtherValue;
336+
337+
var x =
338+
(IEnumerable<SomeClass>?)
339+
someValue.CallLongMethod____________________________________________________()
340+
?? someOtherValue;
341+
342+
var x =
343+
(IEnumerable<SomeClass>?)(
344+
someLongValue____________________________________________
345+
?? someLongValue____________________________________________
346+
) ?? someOtherValue;
347+
203348
var x =
204349
someValue
205350
.Property.CallLongMethod_____________________________________()
206351
.CallMethod__________()
207352
?? throw new Exception();
208353

354+
var x =
355+
someValue
356+
.Property.CallLongMethod_____________________________________()
357+
.CallMethod__________(someParameter)
358+
?? throw new Exception();
359+
209360
var x =
210361
someValue
211362
.Property.CallLongMethod_____________________________________()
@@ -225,6 +376,42 @@ class TestClass
225376
return parameterDescriptor.BindingInfo.Binder
226377
?? Binders.GetBinder(parameterDescriptor.ParameterType);
227378

379+
var x =
380+
someValue.SomeCall()?.SomeCall().SomeProperty
381+
?? someValue.SomeCall()?.SomeCall().SomeProperty;
382+
383+
var x =
384+
someValue.SomeCall().SomeProperty.SomeProperty
385+
?? someValue.SomeCall().SomeProperty.SomeProperty;
386+
387+
var x =
388+
someValue.SomeCall().SomeProperty?.SomeCall().SomeProperty
389+
?? someValue.SomeCall().SomeProperty?.SomeCall().SomeProperty;
390+
391+
var x =
392+
someValue.SomeCall()?.SomeProperty_______________________
393+
?? someValue.SomeCall()?.SomeProperty_______________________;
394+
395+
var x =
396+
someValue.SomeCall().SomeProperty_______________________
397+
?? someValue.SomeCall().SomeProperty_______________________;
398+
399+
var x =
400+
someValue.SomeCall()?.A______().B______().C______()
401+
?? someValue.SomeCall()?.A______().B______().C______();
402+
403+
var x =
404+
someValue.SomeCall().A_______.B_______.C_______
405+
?? someValue.SomeCall().A_______.B_______.C_______;
406+
407+
var x =
408+
someValue switch
409+
{
410+
"" => someLongValue_________________________________,
411+
null => someLongValue_________________________________,
412+
_ => someLongValue_________________________________,
413+
} ?? someOtherValue;
414+
228415
var notIdealSee355 =
229416
variable
230417
.Replace(someParameter_______________________, '.')

0 commit comments

Comments
 (0)