[clang-format] Improve Incomplete detection for (text) protos

[krasimir]

Summary

This patch improves detection of incomplete code for protos and text protos.
This is especially important for text protos in raw string literals, since they
might be partial strings concatenated, and we'd like to disable formatting in
these cases.

[sammccall]

I'm not familiar with clang-format internals, sorry if questions are dumb.

My first inclination is that doing something reasonable for incomplete inputs makes sense, but it doesn't seem to be worth spending complexity on things that are actually intended to not be protos - people should just not use R"pb in this case.

I recall you had a reason for wanting to try this out anyway though, but I can't remember what it is. (Can you add it to the description?)

lib/Format/TokenAnnotator.cpp

716

I don't really understand what this is doing.
It seems to be detecting some particular sequence of invalid tokens }:,]:,>:. Why are these special?

What's the contract of consumeToken()? It's hard to see from the callsites.

822–823

In what sense of "line" is this true?

[krasimir]

• Address review comments

lib/Format/TokenAnnotator.cpp

716

Thank you very much! I guess I got carried away. These don't make sense. I'll update it to only keep dangling : as in key:.

Returning false from consumeToken has the desired effect that (eventually) marks the line as incomplete here:
https://github.com/llvm-mirror/clang/blob/master/lib/Format/TokenAnnotator.cpp#L1055

822–823

This is the UnwrappedLine concept of clang-format: roughly a maximal sequence of tokens that we could have put on the same line if there was no column limit.

[sammccall]

lib/Format/TokenAnnotator.cpp

822–823

I still don't understand why it's true for say C or JS then.
(kinda relevant here because I would like to understand why proto is an exception)

unittests/Format/FormatTestProto.cpp

21

DoNotCheck is never used, consider removing it

21

(nit: is the SC_ prefix really needed in tests?)

[krasimir]

• Address review comments

[krasimir]

lib/Format/TokenAnnotator.cpp

822–823

In C++ stuff like global functions or namespaces result in multiple unwrapped lines, like:

line 1: namespace a {
line 2: int x = 3;
line 3: }

In text protos, the top-level essentially can only contain key-value pairs, where the value can be a submessage, which is like an init list, which is put on a single unwrapped line, like:

line 1: key: { item: data }

I don't know too much more about why this is the case.

unittests/Format/FormatTestProto.cpp

21

The SC_ prefix is consistent with other test files here, like:
https://github.com/llvm-mirror/clang/blob/456e35cbb384bd7fb97cf995163fd9d17e319b77/unittests/Format/FormatTest.cpp#L34

lib/Format/TokenAnnotator.cpp

if (CurrentToken) {
         }
         if (CurrentToken->is(tok::comma) &&
             Style.Language == FormatStyle::LK_JavaScript)
           Left->Type = TT_DictLiteral;
         if (!consumeToken())
           return false;
       }
     }
-    return true;
+    // There are no top-level unbalanced braces in text protos.
+    return Style.Language != FormatStyle::LK_TextProto;
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
     if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)
       ++Left->BlockParameterCount;
     if (Left->Type == TT_ObjCMethodExpr) {
       if (Current->is(tok::colon))
         ++Left->ParameterCount;
case tok::colon:
                  (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
                   Tok->Next->startsSequence(tok::identifier, tok::colon))) {
         // This handles a special macro in ObjC code where selectors including
         // the colon are passed as macro arguments.
         Tok->Type = TT_ObjCMethodExpr;
       } else if (Contexts.back().ContextKind == tok::l_paren) {
         Tok->Type = TT_InlineASMColon;
       }
+      // Detects trailing pieces like key:
+      if ((Style.Language == FormatStyle::LK_Proto ||
+           Style.Language == FormatStyle::LK_TextProto) &&
+          !CurrentToken)
+        return false;
       break;
     case tok::pipe:
     case tok::amp:
       // | and & in declarations/type expressions represent union and
       // intersection types, respectively.
       if (Style.Language == FormatStyle::LK_JavaScript &&
           !Contexts.back().IsExpression)
         Tok->Type = TT_JsTypeOperator;
case tok::less:
         if (Style.Language == FormatStyle::LK_TextProto ||
             (Style.Language == FormatStyle::LK_Proto && Tok->Previous &&
              Tok->Previous->isOneOf(TT_SelectorName, TT_DictLiteral))) {
           Tok->Type = TT_DictLiteral;
           FormatToken *Previous = Tok->getPreviousNonComment();
           if (Previous && Previous->Type != TT_DictLiteral)
             Previous->Type = TT_SelectorName;
         }
+      } else if (Style.Language == FormatStyle::LK_TextProto ||
+                 Style.Language == FormatStyle::LK_Proto) {
+        // In TT_Proto and TT_TextProto, tok::less cannot be a binary operator.
+        return false;
       } else {
         Tok->Type = TT_BinaryOperator;
         NonTemplateLess.insert(Tok);
         CurrentToken = Tok;
         next();
       }
       break;
     case tok::r_paren:
     case tok::r_square:
       return false;
     case tok::r_brace:
-      // Lines can start with '}'.
-      if (Tok->Previous)
+      // Lines can start with '}' except in text protos.
+      if (Tok->Previous || Style.Language == FormatStyle::LK_TextProto)
         return false;
       break;
     case tok::greater:
-      if (Style.Language != FormatStyle::LK_TextProto)
-        Tok->Type = TT_BinaryOperator;
+      // In protos and text protos tok::greater cannot be a binary operator.
+      if (Style.Language == FormatStyle::LK_Proto ||
+          Style.Language == FormatStyle::LK_TextProto)
+        return false;
       break;
     case tok::kw_operator:
       if (Style.Language == FormatStyle::LK_TextProto ||
           Style.Language == FormatStyle::LK_Proto)
         break;
       while (CurrentToken &&
              !CurrentToken->isOneOf(tok::l_paren, tok::semi, tok::r_paren)) {
         if (CurrentToken->isOneOf(tok::star, tok::amp))

unittests/Format/FormatTestProto.cpp

 #include "gtest/gtest.h"
 
 #define DEBUG_TYPE "format-test"
 
 namespace clang {
 namespace format {
 
 class FormatTestProto : public ::testing::Test {
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete };
+
 protected:
   static std::string format(llvm::StringRef Code, unsigned Offset,
-                            unsigned Length, const FormatStyle &Style) {
+                            unsigned Length, const FormatStyle &Style,
+                            StatusCheck CheckComplete = SC_ExpectComplete) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(Offset, Length));
-    tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+    FormattingAttemptStatus Status;
+    tooling::Replacements Replaces =
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
+    bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
+    EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
+        << Code << "\n\n";
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 
   static std::string format(llvm::StringRef Code) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_Proto);
     Style.ColumnLimit = 60; // To make writing tests easier.
     return format(Code, 0, Code.size(), Style);
   }
 
   static void verifyFormat(llvm::StringRef Code) {
     EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
+
+  static void verifyIncompleteFormat(llvm::StringRef Code) {
+    FormatStyle Style = getGoogleStyle(FormatStyle::LK_Proto);
+    EXPECT_EQ(Code.str(),
+              format(Code, 0, Code.size(), Style, SC_ExpectIncomplete));
+  }
 };
 
 TEST_F(FormatTestProto, FormatsMessages) {
   verifyFormat("message SomeMessage {\n"
                "  required int32 field1 = 1;\n"
                "}");
   verifyFormat("message SomeMessage {\n"
                "  required .absolute.Reference field1 = 1;\n"
verifyFormat("option (MyProto.options) = {\n"
                "      operator: 1\n"
                "      operator: 2\n"
                "      operator { key: value }\n"
                "    >\n"
                "  >\n"
                "};");
 }
 
+TEST_F(FormatTestProto, IncompleteFormat) {
+  verifyIncompleteFormat("option (");
+  verifyIncompleteFormat("option (MyProto.options) = { bbbbbbbbb:");
+  verifyIncompleteFormat("option (MyProto.options) = { bbbbbbbbb: <\n");
+  verifyIncompleteFormat("option (MyProto.options) = { bbbbbbbbb: [\n");
+}
+
 } // end namespace tooling
 } // end namespace clang

unittests/Format/FormatTestTextProto.cpp

 
 #define DEBUG_TYPE "format-test"
 
 namespace clang {
 namespace format {
 
 class FormatTestTextProto : public ::testing::Test {
 protected:
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete };
+
   static std::string format(llvm::StringRef Code, unsigned Offset,
-                            unsigned Length, const FormatStyle &Style) {
+                            unsigned Length, const FormatStyle &Style,
+                            StatusCheck CheckComplete = SC_ExpectComplete) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(Offset, Length));
-    tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+    FormattingAttemptStatus Status;
+    tooling::Replacements Replaces =
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
+    bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
+    EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
+        << Code << "\n\n";
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 
   static std::string format(llvm::StringRef Code, const FormatStyle &Style) {
     return format(Code, 0, Code.size(), Style);
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle &Style) {
     EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
   static void verifyFormat(llvm::StringRef Code) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_TextProto);
     Style.ColumnLimit = 60; // To make writing tests easier.
     verifyFormat(Code, Style);
   }
+
+  static void verifyIncompleteFormat(llvm::StringRef Code) {
+    FormatStyle Style = getGoogleStyle(FormatStyle::LK_TextProto);
+    EXPECT_EQ(Code.str(),
+              format(Code, 0, Code.size(), Style, SC_ExpectIncomplete));
+  }
 };
 
 TEST_F(FormatTestTextProto, KeepsTopLevelEntriesFittingALine) {
   verifyFormat("field_a: OK field_b: OK field_c: OK field_d: OK field_e: OK");
 }
 
 TEST_F(FormatTestTextProto, SupportsMessageFields) {
   verifyFormat("msg_field: {}");
verifyFormat("pppppppppp: {\n"
                "    begin: 24\n"
                "    end: 252\n"
                "    key: value\n"
                "    key: value\n"
                "  }\n"
                "}", Style);
 }
 
+TEST_F(FormatTestTextProto, IncompleteFormat) {
+  verifyIncompleteFormat("data {");
+  verifyIncompleteFormat("data <");
+  verifyIncompleteFormat("data [");
+  verifyIncompleteFormat("data: {");
+  verifyIncompleteFormat("data: <");
+  verifyIncompleteFormat("data: [");
+  verifyIncompleteFormat("key:");
+  verifyIncompleteFormat("key:}");
+  verifyIncompleteFormat("key: ]");
+  verifyIncompleteFormat("key: >");
+  verifyIncompleteFormat(": value");
+  verifyIncompleteFormat(": {}");
+  verifyIncompleteFormat(": <>");
+  verifyIncompleteFormat(": []");
+  verifyIncompleteFormat("}\n"
+                         "key: value");
+  verifyIncompleteFormat("]\n"
+                         "key: value");
+  verifyIncompleteFormat("> key: value");
+  verifyIncompleteFormat("data { key: {");
+  verifyIncompleteFormat("data < key: [");
+  verifyIncompleteFormat("data [ key: {");
+  verifyIncompleteFormat("> key: value {");
+  verifyIncompleteFormat("> key: [");
+  verifyIncompleteFormat("}\n"
+                         "key: {");
+  verifyIncompleteFormat("data { key: 1 id:");
+  verifyIncompleteFormat("}\n"
+                         "key {");
+  verifyIncompleteFormat("> <");
+}
+
 } // end namespace tooling
 } // end namespace clang