Implements a way to retrieve information about whether some lines were not formatted due to syntax errors.

[djasper]

include/clang/Format/Format.h

550

I would not call this "skipped lines" as that might lead to confusion. Especially, if you format:

"int a; f (;"

the "line" will not be skipped entirely. And at some stage we actually might decide that we can't fix everything in an unwrapped line, but might still be able to do something.

I think the one bit we are trying to get out here is something like "FoundUnrecoverableError".

lib/Format/UnwrappedLineFormatter.cpp

481

Don't use else after return.

lib/Format/UnwrappedLineFormatter.h

73

I don't see the point in renaming this, but ok.

76

I think this is a bit of an anti-pattern:

• You are passing in a lot of values, most of them 1:1 from local variables.
• We can't come up with a better name than that of the only function calling this.
• This function can only ever be usefully called from the other formatLine function.

All of these hint at this not being a useful separation.

If you want to decrease indentation and want to use early exits, I think a local lambda might be the better choice here. If you are solely trying to make this function shorter, I don't think it is worth it.

unittests/Format/FormatTest.cpp

70

I don't think it makes sense to allow for combinations of style and ExpectSkippedLines. Skipped lines should be independent of the style. Lets instead introduce a separate function. Maybe veryFormatWithError or something?

91

I don't think it makes sense to test this. No crash tests are rare and whether we also discover an unrecoverable syntax error or not isn't really of interest.

[klimek]

Replaced SkippedLines with IncomleteFormat.
Undid refactoring. Will do larger refactoring in subsequent change.

Updating D9532: Implements a way to retrieve information about whether some lines were

not formatted due to syntax errors.

[djasper]

Looks good.

lib/Format/UnwrappedLineFormatter.cpp

479

No braces.

unittests/Format/FormatTest.cpp

25–27

IFA?

40

I suspect that this looks a bit complicated in the test output when it actually triggers. Might be better to pull out the comparison into a local variable.

[klimek]

lib/Format/UnwrappedLineFormatter.cpp

479

Done.

unittests/Format/FormatTest.cpp

25–27

Done.

40

Done.

include/clang/Format/Format.h

 /// \brief Reformats the given \p Ranges in the file \p ID.
 ///
 /// Each range is extended on either end to its next bigger logic unit, i.e.
 /// everything that might influence its formatting or might be influenced by its
 /// formatting.
 ///
 /// Returns the \c Replacements necessary to make all \p Ranges comply with
 /// \p Style.
+///
+/// If \c IncompleteFormat is non-null, its value will be set to true if any
+/// of the affected ranges were not formatted due to a non-recoverable syntax
+/// error.
 tooling::Replacements reformat(const FormatStyle &Style,
                                SourceManager &SourceMgr, FileID ID,
-                               ArrayRef<CharSourceRange> Ranges);
+                               ArrayRef<CharSourceRange> Ranges,
+                               bool *IncompleteFormat = nullptr);
 
 /// \brief Reformats the given \p Ranges in \p Code.
 ///
-/// Otherwise identical to the reformat() function consuming a \c Lexer.
+/// Otherwise identical to the reformat() function using a file ID.
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
-                               StringRef FileName = "<stdin>");
+                               StringRef FileName = "<stdin>",
+                               bool *IncompleteFormat = nullptr);
 
 /// \brief Returns the \c LangOpts that the formatter expects you to set.
 ///
 /// \param Style determines specific settings for lexing mode.
 LangOptions getFormattingLangOpts(const FormatStyle &Style = getLLVMStyle());
 
 /// \brief Description to be used for help text for a llvm::cl option for
 /// specifying format style. The description is closely related to the operation

lib/Format/Format.cpp

Formatter(const FormatStyle &Style, SourceManager &SourceMgr, FileID ID,
     DEBUG(llvm::dbgs() << "File encoding: "
                        << (Encoding == encoding::Encoding_UTF8 ? "UTF8"
                                                                : "unknown")
                        << "\n");
     DEBUG(llvm::dbgs() << "Language: " << getLanguageName(Style.Language)
                        << "\n");
   }
 
-  tooling::Replacements format() {
+  tooling::Replacements format(bool *IncompleteFormat) {
     tooling::Replacements Result;
     FormatTokenLexer Tokens(SourceMgr, ID, Style, Encoding);
 
     UnwrappedLineParser Parser(Style, Tokens.getKeywords(), Tokens.lex(),
                                *this);
     Parser.parse();
     assert(UnwrappedLines.rbegin()->empty());
     for (unsigned Run = 0, RunE = UnwrappedLines.size(); Run + 1 != RunE;
          ++Run) {
       DEBUG(llvm::dbgs() << "Run " << Run << "...\n");
       SmallVector<AnnotatedLine *, 16> AnnotatedLines;
       for (unsigned i = 0, e = UnwrappedLines[Run].size(); i != e; ++i) {
         AnnotatedLines.push_back(new AnnotatedLine(UnwrappedLines[Run][i]));
       }
-      tooling::Replacements RunResult = format(AnnotatedLines, Tokens);
+      tooling::Replacements RunResult =
+          format(AnnotatedLines, Tokens, IncompleteFormat);
       DEBUG({
         llvm::dbgs() << "Replacements for run " << Run << ":\n";
         for (tooling::Replacements::iterator I = RunResult.begin(),
                                              E = RunResult.end();
              I != E; ++I) {
           llvm::dbgs() << I->toString() << "\n";
         }
       });
       for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
         delete AnnotatedLines[i];
       }
       Result.insert(RunResult.begin(), RunResult.end());
       Whitespaces.reset();
     }
     return Result;
   }
 
   tooling::Replacements format(SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
-                               FormatTokenLexer &Tokens) {
+                               FormatTokenLexer &Tokens, bool *IncompleteFormat) {
     TokenAnnotator Annotator(Style, Tokens.getKeywords());
     for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
       Annotator.annotate(*AnnotatedLines[i]);
     }
     deriveLocalStyle(AnnotatedLines);
     for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
       Annotator.calculateFormattingInformation(*AnnotatedLines[i]);
     }
     computeAffectedLines(AnnotatedLines.begin(), AnnotatedLines.end());
 
     Annotator.setCommentLineLevels(AnnotatedLines);
     ContinuationIndenter Indenter(Style, Tokens.getKeywords(), SourceMgr,
                                   Whitespaces, Encoding,
                                   BinPackInconclusiveFunctions);
     UnwrappedLineFormatter Formatter(&Indenter, &Whitespaces, Style,
-                                     Tokens.getKeywords());
+                                     Tokens.getKeywords(), IncompleteFormat);
     Formatter.format(AnnotatedLines, /*DryRun=*/false);
     return Whitespaces.generateReplacements();
   }
 
 private:
   // Determines which lines are affected by the SourceRanges given as input.
   // Returns \c true if at least one line between I and E or one of their
   // children is affected.
private:
   encoding::Encoding Encoding;
   bool BinPackInconclusiveFunctions;
 };
 
 } // end anonymous namespace
 
 tooling::Replacements reformat(const FormatStyle &Style,
                                SourceManager &SourceMgr, FileID ID,
-                               ArrayRef<CharSourceRange> Ranges) {
+                               ArrayRef<CharSourceRange> Ranges,
+                               bool *IncompleteFormat) {
   if (Style.DisableFormat)
     return tooling::Replacements();
   Formatter formatter(Style, SourceMgr, ID, Ranges);
-  return formatter.format();
+  return formatter.format(IncompleteFormat);
 }
 
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
-                               StringRef FileName) {
+                               StringRef FileName,
+                               bool *IncompleteFormat) {
   if (Style.DisableFormat)
     return tooling::Replacements();
 
   FileManager Files((FileSystemOptions()));
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs),
       new DiagnosticOptions);
   SourceManager SourceMgr(Diagnostics, Files);
   std::unique_ptr<llvm::MemoryBuffer> Buf =
       llvm::MemoryBuffer::getMemBuffer(Code, FileName);
   const clang::FileEntry *Entry =
       Files.getVirtualFile(FileName, Buf->getBufferSize(), 0);
   SourceMgr.overrideFileContents(Entry, std::move(Buf));
   FileID ID =
       SourceMgr.createFileID(Entry, SourceLocation(), clang::SrcMgr::C_User);
   SourceLocation StartOfFile = SourceMgr.getLocForStartOfFile(ID);
   std::vector<CharSourceRange> CharRanges;
   for (const tooling::Range &Range : Ranges) {
     SourceLocation Start = StartOfFile.getLocWithOffset(Range.getOffset());
     SourceLocation End = Start.getLocWithOffset(Range.getLength());
     CharRanges.push_back(CharSourceRange::getCharRange(Start, End));
   }
-  return reformat(Style, SourceMgr, ID, CharRanges);
+  return reformat(Style, SourceMgr, ID, CharRanges, IncompleteFormat);
 }
 
 LangOptions getFormattingLangOpts(const FormatStyle &Style) {
   LangOptions LangOpts;
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.CPlusPlus14 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.LineComment = 1;

lib/Format/UnwrappedLineFormatter.h

 class ContinuationIndenter;
 class WhitespaceManager;
 
 class UnwrappedLineFormatter {
 public:
   UnwrappedLineFormatter(ContinuationIndenter *Indenter,
                          WhitespaceManager *Whitespaces,
                          const FormatStyle &Style,
-                         const AdditionalKeywords &Keywords)
+                         const AdditionalKeywords &Keywords,
+                         bool *IncompleteFormat)
       : Indenter(Indenter), Whitespaces(Whitespaces), Style(Style),
-        Keywords(Keywords) {}
+        Keywords(Keywords), IncompleteFormat(IncompleteFormat) {}
 
   unsigned format(const SmallVectorImpl<AnnotatedLine *> &Lines, bool DryRun,
                   int AdditionalIndent = 0, bool FixBadIndentation = false);
 
 
   /// \brief If the \p State's next token is an r_brace closing a nested block,
   /// format the nested block before it.
   ///
public:
   bool formatChildren(LineState &State, bool NewLine, bool DryRun,
                       unsigned &Penalty);
 
 private:
   /// \brief Formats an \c AnnotatedLine and returns the penalty.
   ///
   /// If \p DryRun is \c false, directly applies the changes.
   unsigned format(const AnnotatedLine &Line, unsigned FirstIndent,
                   bool DryRun);
 
   /// \brief An edge in the solution space from \c Previous->State to \c State,
   /// inserting a newline dependent on the \c NewLine.
   struct StateNode {
     StateNode(const LineState &State, bool NewLine, StateNode *Previous)
         : State(State), NewLine(NewLine), Previous(Previous) {}
     LineState State;
     bool NewLine;
     StateNode *Previous;
   };
 
private:
 
   llvm::SpecificBumpPtrAllocator<StateNode> Allocator;
 
   // Cache to store the penalty of formatting a vector of AnnotatedLines
   // starting from a specific additional offset. Improves performance if there
   // are many nested blocks.
   std::map<std::pair<const SmallVectorImpl<AnnotatedLine *> *, unsigned>,
            unsigned> PenaltyCache;
+
+  bool *IncompleteFormat;
 };
 } // end namespace format
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEFORMATTER_H

lib/Format/UnwrappedLineFormatter.cpp

if (!DryRun) {
       for (unsigned i = 0; i < MergedLines; ++i) {
         join(*I[i], *I[i + 1]);
       }
     }
     I += MergedLines;
 
     bool FixIndentation =
         FixBadIndentation && (LevelIndent != FirstTok->OriginalColumn);
+    bool ShouldFormat = TheLine.Affected || FixIndentation;
     if (TheLine.First->is(tok::eof)) {
       if (PreviousLine && PreviousLine->Affected && !DryRun) {
         // Remove the file's trailing whitespace.
         unsigned Newlines = std::min(FirstTok->NewlinesBefore, 1u);
         Whitespaces->replaceWhitespace(*TheLine.First, Newlines,
                                        /*IndentLevel=*/0, /*Spaces=*/0,
                                        /*TargetColumn=*/0);
       }
-    } else if (TheLine.Type != LT_Invalid &&
-               (TheLine.Affected || FixIndentation)) {
+    } else if (TheLine.Type != LT_Invalid && ShouldFormat) {
       if (FirstTok->WhitespaceRange.isValid()) {
         if (!DryRun)
           formatFirstToken(*TheLine.First, PreviousLine, TheLine.Level, Indent,
                            TheLine.InPPDirective);
       } else {
         Indent = LevelIndent = FirstTok->OriginalColumn;
       }
 
if (TheLine.First->is(tok::eof)) {
                IndentForLevel[TheLine.Level] == -1) &&
               !TheLine.InPPDirective)
             IndentForLevel[TheLine.Level] = LevelIndent;
         } else if (!DryRun) {
           Whitespaces->addUntouchableToken(*Tok, TheLine.InPPDirective);
         }
       }
     }
+    if (TheLine.Type == LT_Invalid && ShouldFormat && IncompleteFormat) {
+      *IncompleteFormat = true;
+    }
     if (!DryRun)
       markFinalized(TheLine.First);
     PreviousLine = *I;
   }
   PenaltyCache[CacheKey] = Penalty;
   return Penalty;
 }
 

unittests/Format/FormatTest.cpp

 
 namespace clang {
 namespace format {
 
 FormatStyle getGoogleStyle() { return getGoogleStyle(FormatStyle::LK_Cpp); }
 
 class FormatTest : public ::testing::Test {
 protected:
+  enum IncompleteCheck {
+    IFA_ExpectComplete,
+    IFA_ExpectIncomplete,
+    IFA_DoNotCheck
+  };
+
   std::string format(llvm::StringRef Code, unsigned Offset, unsigned Length,
-                     const FormatStyle &Style) {
+                     const FormatStyle &Style,
+                     IncompleteCheck CheckIncomplete = IFA_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);
+    bool IncompleteFormat = false;
+    tooling::Replacements Replaces =
+        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
+    if (CheckIncomplete != IFA_DoNotCheck)
+      EXPECT_EQ(CheckIncomplete == IFA_ExpectIncomplete, IncompleteFormat)
+          << Code << "\n\n";
     ReplacementCount = Replaces.size();
     std::string Result = applyAllReplacements(Code, Replaces);
     EXPECT_NE("", Result);
     DEBUG(llvm::errs() << "\n" << Result << "\n\n");
     return Result;
   }
 
   std::string format(llvm::StringRef Code,
-                     const FormatStyle &Style = getLLVMStyle()) {
-    return format(Code, 0, Code.size(), Style);
+                     const FormatStyle &Style = getLLVMStyle(),
+                     IncompleteCheck CheckIncomplete = IFA_ExpectComplete) {
+    return format(Code, 0, Code.size(), Style, CheckIncomplete);
   }
 
   FormatStyle getLLVMStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getLLVMStyle();
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   FormatStyle getGoogleStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getGoogleStyle();
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   void verifyFormat(llvm::StringRef Code,
                     const FormatStyle &Style = getLLVMStyle()) {
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
+  void verifyIncompleteFormat(llvm::StringRef Code,
+                              const FormatStyle &Style = getLLVMStyle()) {
+    EXPECT_EQ(Code.str(),
+              format(test::messUp(Code), Style, IFA_ExpectIncomplete));
+  }
+
   void verifyGoogleFormat(llvm::StringRef Code) {
     verifyFormat(Code, getGoogleStyle());
   }
 
   void verifyIndependentOfContext(llvm::StringRef text) {
     verifyFormat(text);
     verifyFormat(llvm::Twine("void f() { " + text + " }").str());
   }
 
   /// \brief Verify that clang-format does not crash on the given input.
   void verifyNoCrash(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
-    format(Code, Style);
+    format(Code, Style, IFA_DoNotCheck);
   }
 
   int ReplacementCount;
 };
 
 TEST_F(FormatTest, MessUp) {
   EXPECT_EQ("1 2 3", test::messUp("1 2 3"));
   EXPECT_EQ("1 2 3\n", test::messUp("1\n2\n3\n"));
   EXPECT_EQ("a\n//b\nc", test::messUp("a\n//b\nc"));
verifyFormat("class A {\n"
                "  int a;\n"
                "  A() try : a(0) {\n"
                "  } catch (...) {\n"
                "    throw;\n"
                "  }\n"
                "};\n");
 
   // Incomplete try-catch blocks.
-  verifyFormat("try {} catch (");
+  verifyIncompleteFormat("try {} catch (");
 }
 
 TEST_F(FormatTest, FormatSEHTryCatch) {
   verifyFormat("__try {\n"
                "  int a = b * c;\n"
                "} __except (EXCEPTION_EXECUTE_HANDLER) {\n"
                "  // Do nothing.\n"
                "}");
EXPECT_EQ("#define A \\\n"
             format("#define A \\\n"
                    "  a;      \\\n"
                    "          \\\n"
                    "  b;",
                    getLLVMStyleWithColumns(11)));
 }
 
 TEST_F(FormatTest, MacroDefinitionsWithIncompleteCode) {
-  verifyFormat("#define A :");
+  verifyIncompleteFormat("#define A :");
   verifyFormat("#define SOMECASES  \\\n"
                "  case 1:          \\\n"
                "  case 2\n",
                getLLVMStyleWithColumns(20));
   verifyFormat("#define A template <typename T>");
-  verifyFormat("#define STR(x) #x\n"
+  verifyIncompleteFormat("#define STR(x) #x\n"
                          "f(STR(this_is_a_string_literal{));");
   verifyFormat("#pragma omp threadprivate( \\\n"
                "    y)), // expected-warning",
                getLLVMStyleWithColumns(28));
   verifyFormat("#d, = };");
   verifyFormat("#if \"a");
-  verifyFormat("({\n"
+  verifyIncompleteFormat("({\n"
                          "#define b }\\\n"
                          "  a\n"
                          "a");
   verifyFormat("#define A     \\\n"
                "  {           \\\n"
                "    {\n"
                "#define B     \\\n"
                "  }           \\\n"
                "  }",
                getLLVMStyleWithColumns(15));
-
   verifyNoCrash("#if a\na(\n#else\n#endif\n{a");
   verifyNoCrash("a={0,1\n#if a\n#else\n;\n#endif\n}");
   verifyNoCrash("#if a\na(\n#else\n#endif\n) a {a,b,c,d,f,g};");
   verifyNoCrash("#ifdef A\n a(\n #else\n #endif\n) = []() {      \n)}");
 }
 
 TEST_F(FormatTest, MacrosWithoutTrailingSemicolon) {
   verifyFormat("SOME_TYPE_NAME abc;"); // Gated on the newline.
TEST_F(FormatTest, LayoutStatementsAroundPreprocessorDirectives) {
   verifyFormat("DEBUG({\n"
                "  return aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n"
                "         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;\n"
                "});\n"
                "#if a\n"
                "#else\n"
                "#endif");
 
-  verifyFormat("void f(\n"
+  verifyIncompleteFormat("void f(\n"
                          "#if A\n"
                          "    );\n"
                          "#else\n"
                          "#endif");
 }
 
 TEST_F(FormatTest, GraciouslyHandleIncorrectPreprocessorConditions) {
   verifyFormat("#endif\n"
                "#if B");
 }
 
 TEST_F(FormatTest, FormatsJoinedLinesOnSubsequentRuns) {
TEST_F(FormatTest, IncorrectCodeDoNoWhile) {
   verifyFormat("do {\n"
                "  f();\n"
                "}");
 }
 
 TEST_F(FormatTest, IncorrectCodeMissingParens) {
   verifyFormat("if {\n  foo;\n  foo();\n}");
   verifyFormat("switch {\n  foo;\n  foo();\n}");
-  verifyFormat("for {\n  foo;\n  foo();\n}");
+  verifyIncompleteFormat("for {\n  foo;\n  foo();\n}");
   verifyFormat("while {\n  foo;\n  foo();\n}");
   verifyFormat("do {\n  foo;\n  foo();\n} while;");
 }
 
 TEST_F(FormatTest, DoesNotTouchUnwrappedLinesWithErrors) {
-  verifyFormat("namespace {\n"
+  verifyIncompleteFormat("namespace {\n"
                          "class Foo { Foo (\n"
                          "};\n"
                          "} // comment");
 }
 
 TEST_F(FormatTest, IncorrectCodeErrorDetection) {
   EXPECT_EQ("{\n  {}\n", format("{\n{\n}\n"));
   EXPECT_EQ("{\n  {}\n", format("{\n  {\n}\n"));
   EXPECT_EQ("{\n  {}\n", format("{\n  {\n  }\n"));
   EXPECT_EQ("{\n  {}\n}\n}\n", format("{\n  {\n    }\n  }\n}\n"));
 
TEST_F(FormatTest, ObjCDictLiterals) {
   verifyFormat("@{@\"one\" : @1}");
 
   verifyFormat("@{@\"one\" : @{@2 : @1}}");
   verifyFormat("@{\n"
                "  @\"one\" : @{@2 : @1},\n"
                "}");
 
   verifyFormat("@{1 > 2 ? @\"one\" : @\"two\" : 1 > 2 ? @1 : @2}");
-  verifyFormat("[self setDict:@{}");
-  verifyFormat("[self setDict:@{@1 : @2}");
+  verifyIncompleteFormat("[self setDict:@{}");
+  verifyIncompleteFormat("[self setDict:@{@1 : @2}");
   verifyFormat("NSLog(@\"%@\", @{@1 : @2, @2 : @3}[@1]);");
   verifyFormat(
       "NSDictionary *masses = @{@\"H\" : @1.0078, @\"He\" : @4.0026};");
   verifyFormat(
       "NSDictionary *settings = @{AVEncoderKey : @(AVAudioQualityMax)};");
 
   verifyFormat("NSDictionary *d = @{\n"
                "  @\"nam\" : NSUserNam(),\n"
verifyFormat("NSMutableDictionary *dictionary =\n"
                "    [NSMutableDictionary dictionaryWithDictionary:@{\n"
                "      aaaaaaaaaaaaaaaaaaaaa : aaaaaaaaaaaaa,\n"
                "      bbbbbbbbbbbbbbbbbb : bbbbb,\n"
                "      cccccccccccccccc : ccccccccccccccc\n"
                "    }];");
 }
 
 TEST_F(FormatTest, ObjCArrayLiterals) {
-  verifyFormat("@[");
+  verifyIncompleteFormat("@[");
   verifyFormat("@[]");
   verifyFormat(
       "NSArray *array = @[ @\" Hey \", NSApp, [NSNumber numberWithInt:42] ];");
   verifyFormat("return @[ @3, @[], @[ @4, @5 ] ];");
   verifyFormat("NSArray *array = @[ [foo description] ];");
 
   verifyFormat(
       "NSArray *some_variable = @[\n"