Page MenuHomePhabricator

[clang-format] Align chained method invocations for C# code
AbandonedPublic

Authored by jbcoe on Feb 21 2020, 3:32 AM.

Details

Reviewers
krasimir
Summary

Align chained methods invocations on the period before the method name for C# code.

Diff Detail

Event Timeline

jbcoe created this revision.Feb 21 2020, 3:32 AM
jbcoe updated this revision to Diff 245811.Feb 21 2020, 3:35 AM

Correct patch content

jbcoe retitled this revision from Align chained method invocations for C# code to [clang-format] Align chained method invocations for C# code.Feb 24 2020, 6:09 AM
jbcoe planned changes to this revision.Feb 24 2020, 6:27 AM

Found an issue:

string base_path = ModuleDirectory.Substring(0, first);
PublicIncludePaths .AddRange(new string[] {

is misformatted. Will fix.

This works nice for many examples, however I want to point out some limitations of using WhitespaceManager for this.
The alignment code in WhitespaceManager is conceptually at the end of the formatting pipeline, where all of the formatting decisions (where to break a line and what indentation to use for the following line) have already been fixed.
The code here attempts to do a post-processing pass that re-arranges some tokens, without being able to "deeply" affect / fix formatting.
As such is usually used either for non-compound constructs (aligning trailing comments) or in cases where the alignment is best-effort.
The tricky part of this is that, if not careful, aligning using the WhitespaceManager could introduce instances of lines that start going over the column limit as a result of alignment.
If you attempt to re-format the resulting code then, clang-format might take different formatting decisions for such lines, resulting in unstable formatting.

Consider these two contrasting behaviors (with this patched in):
First, an instance where the first in a chain of methods cannot be formatted in-place without exceeding the column limit.
This results in it being put on a new line, with the following chained instances aligned:

-fffffffffffffffffffffffffffffffffffffffffff.Select(number => { return number * number * number * number * number * number; })
-                                           .Where(number => true)
-                                           .OrderBy(number => number);
+fffffffffffffffffffffffffffffffffffffffffff
+    .Select(number => {
+      return number * number * number * number * number * number;
+    })
+    .Where(number => true)
+    .OrderBy(number => number);

I think this is good behavior.

Now consider an instance where the first in a chain can be formatted in-place without exceeding the column limit, but the second in a chain of methods exceeds the column limit if aligned with the first:

//                                              last column (80) here ---------.
//                                                                             V

fffffffffffffffffffffffffffffffffffffffffff.Select(number => number * number)
                                           .Where(number => number * number * number * number)
                                           .OrderBy(number => number);

Without this patched in, this results in an instance where the first call is left on the same line, and the other two are indented +4. Note that this produces formatting that stays under the column limit:

 //                                              last column (80) here ---------.
 //                                                                             V
 fffffffffffffffffffffffffffffffffffffffffff.Select(number => number * number)
-                                           .Where(number => number * number * number)
-                                           .OrderBy(number => number);
+    .Where(number => number * number * number)
+    .OrderBy(number => number);

With this patched in, effectively WhitespaceManager takes the last diff and indents the last two chained method invocations to align them with the first one, pushing the second one over the column limit, producing result that is the same as the input.

I think what should happen in this case should be similar to the first instance: we should put the .Select method call on a new line, with +4 indentation.
Note that this decision has implications for the formatting decisions taken to format the bodies of the lambdas in the method calls and so on, so I think it needs to be incorporated more deeply into clang-format than at the WhitespaceManager level.

Later I'll try to look at similar cases in the codebase and try to come up with a suggestion around where can we put such logic.

I still think it will be very useful to precisely define this, since maybe we can get away with a small change somewhere else that gets us all the benefits:

  • What do we consider a method chain?
    • where does the method chain start? In this example, someValues.someNumbers. is not part of the method chain, it seems.
    • can the methods in a method chain take more than one arguments? No arguments? Be fields?
    • is a method chain only the "trailing part" in an expression? Can it be followed by a non-method-chain stuff? For example, (not sure if that's valid C#), suppose a method chain ends in a delegate or something, and we invoke it: f.a(...).b(...).c(...)(...)
  • Why does the default behavior we got right now does not work for C#? Especially if there is a reference to documentation about this, that would be awesome.

Depending on these, maybe one way to approach this would be:
Detect the instances where we have a maximal trailing chain of at least 2 method calls, and force a line break before the . of the first such method chain.
This will effectively disallow patterns like:

heada.expression.first(...)
                .second(...);

and expand them into:

heada.expression
    .first(...)
    .second(...);

We can probably implement some scheme like this in the formatting pipeline without too much work.

Note: these are mostly just suggestions and my gut feelings, please flag anything that you find suspicious.

jbcoe abandoned this revision.Apr 22 2020, 3:21 AM