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

[jbcoe]

Summary

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

[jbcoe]

Correct patch content

[jbcoe]

Found an issue:

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

is misformatted. Will fix.

[krasimir]

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.

clang/lib/Format/WhitespaceManager.h

private:
   /// Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
   /// \brief Align consecutive C/C++ preprocessor macros over all \c Changes.
   void alignConsecutiveMacros();
 
+  /// Align consecutive C# method invocations on the '.' before the method name.
+  void alignChainedCSharpMethodInvocations();
+
   /// Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
   /// Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
   /// Align trailing comments over all \c Changes.
   void alignTrailingComments();

clang/lib/Format/WhitespaceManager.cpp

 
 const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   llvm::sort(Changes, Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
   alignConsecutiveMacros();
+  alignChainedCSharpMethodInvocations();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;
 }
for (unsigned E = Changes.size(); I != E; ++I) {
     MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
   }
 
   EndOfSequence = I;
   AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn,
                      FoundMatchOnLine, AlignMacrosMatches, Changes);
 }
 
+void WhitespaceManager::alignChainedCSharpMethodInvocations() {
+  if (!Style.isCSharp())
+    return;
+
+  AlignTokens(
+      Style, [&](const Change &C) { return C.Tok->is(tok::period); }, Changes,
+      /*StartAt=*/0);
+}
+
 void WhitespaceManager::alignConsecutiveAssignments() {
   if (!Style.AlignConsecutiveAssignments)
     return;
 
   AlignTokens(
       Style,
       [&](const Change &C) {
         // Do not align on equal signs that are first on a line.

clang/unittests/Format/FormatTestCSharp.cpp

PrintOrderDetails(orderNum: 31, productName: "Red Mug",
 
   // Ensure that trailing comments do not cause problems.
   verifyFormat(R"(//
 PrintOrderDetails(orderNum: 31, productName: "Red Mug",  // comment
                   sellerName: "Gift Shop");)",
                Style);
 }
 
+TEST_F(FormatTestCSharp, CSharpChainedMethodInvocations) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat(R"(//
+var processed = someNumbers.Select(number => number * 2)
+                           .Where(number => number != 6)
+                           .OrderBy(number => number);)",
+               Style);
+
+  // See if additional .'s cause issues.
+  verifyFormat(R"(//
+var processed = someValues.someNumbers.Select(number => number.value * 2)
+                                      .Where(number => number.value != 6)
+                                      .OrderBy(number => number.value);)",
+               Style);
+}
 } // namespace format
 } // end namespace clang