[clang-format] Replace IncompleteFormat by a struct with Line

[krasimir]

Summary

This patch replaces the boolean IncompleteFormat that is used to notify the client if an unrecoverable syntax error occurred by a struct that also contains a line number.

[krasimir]

• Updated string escaping

[krasimir]

• Removed comments

[djasper]

include/clang/Format/Format.h

1538

This is a public interface function that possibly we shouldn't just change in case people use clang-format as a library.

Can you leave the old one, with a comment say that it is being deprecated. That one can then just call this new version and set the boolean depending on whether the string is empty.

lib/Format/UnwrappedLineFormatter.cpp

845

I wonder whether this might be confusing when an unwrapped line spans over multiple physical lines. But I also don't have a much better idea on how to phrase this.

tools/clang-format/ClangFormat.cpp

317–321

I think we should not change the JSON here in such a backwards incompatible way. That will break all existing editor integrations that use it. How about adding an additional field "Message:" that gets set when the format is incomplete?

[krasimir]

• Address review comments

[krasimir]

• Removed double declaration

lib/Format/UnwrappedLineFormatter.cpp

845

We may wait and see how much people will be confused.

[krasimir]

• Introduce a proper abstraction

[djasper]

include/clang/Format/Format.h

1519

Maybe we should invert this and make this FormatComplete or something? Otherwise this is bound to lead to an unnecessary double-negative.

1522

s/when/at which/

1523

no comma, I think.

1524

Hm. Something we might need to be thinking about here is whether this should be the line before or after formatting. So specifically, if I format:

int a
    = 1;
f(

Then, this gets reformatted to:

int a = 1;
f(

Would we want the Line to be 2 or 3?

lib/Format/Format.cpp

1886

nit: There shouldn't be a line break here.

1889

In LLVM style, there should be a line break here.

[krasimir]

• Changed IncompleteFormat to FormatComplete

[krasimir]

• Refactor tests

[krasimir]

include/clang/Format/Format.h

1524

I'd go with the original line, because I think it might be more useful and because it's easier to implement.

[krasimir]

include/clang/Format/Format.h

1524

Even better: the line after formatting is functionally dependent on the line before formatting and the replacements. I see two use-cases: clients that refuse to format in case of syntax error, then they can just use the original line, and clients that apply the partial replacements, which can compute the line after formatting from them and the original line.

[djasper]

Sounds good.

include/clang/Format/Format.h

 ///     - Special #include's among declarations (e.g. functions).
 /// - If a replacement has offset UINT_MAX, length 1, and a replacement text
 ///   that is the name of the header to be removed, the header will be removed
 ///   from \p Code if it exists.
 llvm::Expected<tooling::Replacements>
 cleanupAroundReplacements(StringRef Code, const tooling::Replacements &Replaces,
                           const FormatStyle &Style);
 
+/// \brief Represents the status of a formatting attempt.
+struct FormattingAttemptStatus {
+  /// \brief A value of ``false`` means that any of the affected ranges were not
+  /// formatted due to a non-recoverable syntax error.
+  bool FormatComplete = true;
+
+  /// \brief If ``FormatComplete`` is false, ``Line`` records a one-based
+  /// original line number at which a syntax error might have occurred. This is
+  /// based on a best-effort analysis and could be imprecise.
+  unsigned Line = 0;
+};
+
 /// \brief Reformats the given \p Ranges in \p Code.
 ///
 /// 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 ``Replacements`` necessary to make all \p Ranges comply with
 /// \p Style.
 ///
-/// If ``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.
+/// If ``Status`` is non-null, its value will be populated with the status of
+/// this formatting attempt. See \c FormattingAttemptStatus.
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
                                StringRef FileName = "<stdin>",
-                               bool *IncompleteFormat = nullptr);
+                               FormattingAttemptStatus *Status = nullptr);
+
+/// \brief Same as above, except if ``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, StringRef Code,
+                               ArrayRef<tooling::Range> Ranges,
+                               StringRef FileName,
+                               bool *IncompleteFormat);
 
 /// \brief Clean up any erroneous/redundant code in the given \p Ranges in \p
 /// Code.
 ///
 /// Returns the ``Replacements`` that clean up all \p Ranges in \p Code.
 tooling::Replacements cleanup(const FormatStyle &Style, StringRef Code,
                               ArrayRef<tooling::Range> Ranges,
                               StringRef FileName = "<stdin>");

lib/Format/Format.cpp

for (AnnotatedLine *Line : Lines) {
       }
     }
   }
 };
 
 class Formatter : public TokenAnalyzer {
 public:
   Formatter(const Environment &Env, const FormatStyle &Style,
-            bool *IncompleteFormat)
-      : TokenAnalyzer(Env, Style), IncompleteFormat(IncompleteFormat) {}
+            FormattingAttemptStatus *Status)
+      : TokenAnalyzer(Env, Style), Status(Status) {}
 
   tooling::Replacements
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override {
     tooling::Replacements Result;
     deriveLocalStyle(AnnotatedLines);
     AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
                                           AnnotatedLines.end());
     for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
       Annotator.calculateFormattingInformation(*AnnotatedLines[i]);
     }
     Annotator.setCommentLineLevels(AnnotatedLines);
 
     WhitespaceManager Whitespaces(
         Env.getSourceManager(), Style,
         inputUsesCRLF(Env.getSourceManager().getBufferData(Env.getFileID())));
     ContinuationIndenter Indenter(Style, Tokens.getKeywords(),
                                   Env.getSourceManager(), Whitespaces, Encoding,
                                   BinPackInconclusiveFunctions);
     UnwrappedLineFormatter(&Indenter, &Whitespaces, Style, Tokens.getKeywords(),
-                           IncompleteFormat)
+                           Env.getSourceManager(), Status)
         .format(AnnotatedLines);
     for (const auto &R : Whitespaces.generateReplacements())
       if (Result.add(R))
         return Result;
     return Result;
   }
 
 private:
if (Style.Standard == FormatStyle::LS_Auto)
       Style.Standard = hasCpp03IncompatibleFormat(AnnotatedLines)
                            ? FormatStyle::LS_Cpp11
                            : FormatStyle::LS_Cpp03;
     BinPackInconclusiveFunctions =
         HasBinPackedFunction || !HasOnePerLineFunction;
   }
 
   bool BinPackInconclusiveFunctions;
-  bool *IncompleteFormat;
+  FormattingAttemptStatus *Status;
 };
 
 // This class clean up the erroneous/redundant code around the given ranges in
 // file.
 class Cleaner : public TokenAnalyzer {
 public:
   Cleaner(const Environment &Env, const FormatStyle &Style)
       : TokenAnalyzer(Env, Style),
cleanupAroundReplacements(StringRef Code, const tooling::Replacements &Replaces,
   // Make header insertion replacements insert new headers into correct blocks.
   tooling::Replacements NewReplaces =
       fixCppIncludeInsertions(Code, Replaces, Style);
   return processReplacements(Cleanup, Code, NewReplaces, Style);
 }
 
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
-                               StringRef FileName, bool *IncompleteFormat) {
+                               StringRef FileName,
+                               FormattingAttemptStatus *Status) {
   FormatStyle Expanded = expandPresets(Style);
   if (Expanded.DisableFormat)
     return tooling::Replacements();
   if (Expanded.Language == FormatStyle::LK_JavaScript && isMpegTS(Code))
     return tooling::Replacements();
   auto Env = Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
 
   auto reformatAfterApplying = [&] (TokenAnalyzer& Fixer) {
     tooling::Replacements Fixes = Fixer.process();
     if (!Fixes.empty()) {
       auto NewCode = applyAllReplacements(Code, Fixes);
       if (NewCode) {
         auto NewEnv = Environment::CreateVirtualEnvironment(
             *NewCode, FileName,
             tooling::calculateRangesAfterReplacements(Fixes, Ranges));
-        Formatter Format(*NewEnv, Expanded, IncompleteFormat);
+        Formatter Format(*NewEnv, Expanded, Status);
         return Fixes.merge(Format.process());
       }
     }
-    Formatter Format(*Env, Expanded, IncompleteFormat);
+    Formatter Format(*Env, Expanded, Status);
     return Format.process();
   };
 
   if (Style.Language == FormatStyle::LK_Cpp &&
       Style.FixNamespaceComments) {
     NamespaceEndCommentsFixer CommentsFixer(*Env, Expanded);
     return reformatAfterApplying(CommentsFixer);
   }
 
   if (Style.Language == FormatStyle::LK_JavaScript &&
       Style.JavaScriptQuotes != FormatStyle::JSQS_Leave) {
     JavaScriptRequoter Requoter(*Env, Expanded);
     return reformatAfterApplying(Requoter);
   }
 
-  Formatter Format(*Env, Expanded, IncompleteFormat);
+  Formatter Format(*Env, Expanded, Status);
   return Format.process();
 }
 
 tooling::Replacements cleanup(const FormatStyle &Style, StringRef Code,
                               ArrayRef<tooling::Range> Ranges,
                               StringRef FileName) {
   std::unique_ptr<Environment> Env =
       Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
   Cleaner Clean(*Env, Style);
   return Clean.process();
 }
 
+tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
+                               ArrayRef<tooling::Range> Ranges,
+                               StringRef FileName, bool *IncompleteFormat) {
+  FormattingAttemptStatus Status;
+  auto Result = reformat(Style, Code, Ranges, FileName, &Status);
+  if (!Status.FormatComplete)
+    *IncompleteFormat = true;
+  return Result;
+}
+
 tooling::Replacements fixNamespaceEndComments(const FormatStyle &Style,
                                               StringRef Code,
                                               ArrayRef<tooling::Range> Ranges,
                                               StringRef FileName) {
   std::unique_ptr<Environment> Env =
       Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
   NamespaceEndCommentsFixer Fix(*Env, Style);
   return Fix.process();

lib/Format/UnwrappedLineFormatter.h

 class WhitespaceManager;
 
 class UnwrappedLineFormatter {
 public:
   UnwrappedLineFormatter(ContinuationIndenter *Indenter,
                          WhitespaceManager *Whitespaces,
                          const FormatStyle &Style,
                          const AdditionalKeywords &Keywords,
-                         bool *IncompleteFormat)
+                         const SourceManager &SourceMgr,
+                         FormattingAttemptStatus *Status)
       : Indenter(Indenter), Whitespaces(Whitespaces), Style(Style),
-        Keywords(Keywords), IncompleteFormat(IncompleteFormat) {}
+        Keywords(Keywords), SourceMgr(SourceMgr),
+        Status(Status) {}
 
   /// \brief Format the current block and return the penalty.
   unsigned format(const SmallVectorImpl<AnnotatedLine *> &Lines,
                   bool DryRun = false, int AdditionalIndent = 0,
                   bool FixBadIndentation = false);
 
 private:
   /// \brief Add a new line and the required indent before the first Token
private:
   std::map<std::pair<const SmallVectorImpl<AnnotatedLine *> *, unsigned>,
            unsigned>
       PenaltyCache;
 
   ContinuationIndenter *Indenter;
   WhitespaceManager *Whitespaces;
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
-  bool *IncompleteFormat;
+  const SourceManager &SourceMgr;
+  FormattingAttemptStatus *Status;
 };
 } // end namespace format
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEFORMATTER_H

lib/Format/UnwrappedLineFormatter.cpp

bool ContinueFormatting =
         TheLine.Level > RangeMinLevel ||
         (TheLine.Level == RangeMinLevel && !TheLine.startsWith(tok::r_brace));
 
     bool FixIndentation = (FixBadIndentation || ContinueFormatting) &&
                           Indent != TheLine.First->OriginalColumn;
     bool ShouldFormat = TheLine.Affected || FixIndentation;
     // We cannot format this line; if the reason is that the line had a
     // parsing error, remember that.
-    if (ShouldFormat && TheLine.Type == LT_Invalid && IncompleteFormat)
-      *IncompleteFormat = true;
+    if (ShouldFormat && TheLine.Type == LT_Invalid && Status) {
+      Status->FormatComplete = false;
+      Status->Line =
+          SourceMgr.getSpellingLineNumber(TheLine.First->Tok.getLocation());
+    }
 
     if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (!DryRun)
         formatFirstToken(TheLine, PreviousLine, Indent);
 
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       unsigned ColumnLimit = getColumnLimit(TheLine.InPPDirective, NextLine);
       bool FitsIntoOneLine =
           TheLine.Last->TotalLength + Indent <= ColumnLimit ||
           (TheLine.Type == LT_ImportStatement &&
            (Style.Language != FormatStyle::LK_JavaScript ||

test/Format/incomplete.cpp

 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=LLVM -cursor=0 \
 // RUN:   | FileCheck -strict-whitespace %s
-// CHECK: {{"IncompleteFormat": true}}
+// CHECK: {{"IncompleteFormat": true, "Line": 2}}
 // CHECK: {{^int\ \i;$}}
  int    i;
 // CHECK: {{^f  \( g  \(;$}}
 f  ( g  (;

tools/clang-format/ClangFormat.cpp

Replacements Replaces = sortIncludes(*FormatStyle, Code->getBuffer(), Ranges,
                                        AssumedFileName, &CursorPosition);
   auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), Replaces);
   if (!ChangedCode) {
     llvm::errs() << llvm::toString(ChangedCode.takeError()) << "\n";
     return true;
   }
   // Get new affected ranges after sorting `#includes`.
   Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
-  bool IncompleteFormat = false;
+  FormattingAttemptStatus Status;
   Replacements FormatChanges = reformat(*FormatStyle, *ChangedCode, Ranges,
-                                        AssumedFileName, &IncompleteFormat);
+                                        AssumedFileName, &Status);
   Replaces = Replaces.merge(FormatChanges);
   if (OutputXML) {
     outs() << "<?xml version='1.0'?>\n<replacements "
               "xml:space='preserve' incomplete_format='"
-           << (IncompleteFormat ? "true" : "false") << "'>\n";
+           << (Status.FormatComplete ? "false" : "true") << "'";
+    if (!Status.FormatComplete)
+      outs() << " line=" << Status.Line;
+    outs() << ">\n";
     if (Cursor.getNumOccurrences() != 0)
       outs() << "<cursor>"
              << FormatChanges.getShiftedCodePosition(CursorPosition)
              << "</cursor>\n";
 
     outputReplacementsXML(Replaces);
     outs() << "</replacements>\n";
   } else {
     IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
         new vfs::InMemoryFileSystem);
     FileManager Files(FileSystemOptions(), InMemoryFileSystem);
     DiagnosticsEngine Diagnostics(
         IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs),
         new DiagnosticOptions);
     SourceManager Sources(Diagnostics, Files);
     FileID ID = createInMemoryFile(AssumedFileName, Code.get(), Sources, Files,
                                    InMemoryFileSystem.get());
     Rewriter Rewrite(Sources, LangOptions());
     tooling::applyAllReplacements(Replaces, Rewrite);
     if (Inplace) {
       if (Rewrite.overwriteChangedFiles())
         return true;
     } else {
-      if (Cursor.getNumOccurrences() != 0)
+      if (Cursor.getNumOccurrences() != 0) {
         outs() << "{ \"Cursor\": "
                << FormatChanges.getShiftedCodePosition(CursorPosition)
                << ", \"IncompleteFormat\": "
-               << (IncompleteFormat ? "true" : "false") << " }\n";
+               << (Status.FormatComplete ? "false" : "true");
+        if (!Status.FormatComplete)
+          outs() << ", \"Line\": " << Status.Line;
+        outs() << " }\n";
+      }
       Rewrite.getEditBuffer(ID).write(outs());
     }
   }
   return false;
 }
 
 }  // namespace format
 }  // namespace clang

unittests/Format/FormatTest.cpp

protected:
   };
 
   std::string format(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle(),
                      IncompleteCheck CheckIncomplete = IC_ExpectComplete) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
-    bool IncompleteFormat = false;
+    FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
-        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
     if (CheckIncomplete != IC_DoNotCheck) {
       bool ExpectedIncompleteFormat = CheckIncomplete == IC_ExpectIncomplete;
-      EXPECT_EQ(ExpectedIncompleteFormat, IncompleteFormat) << Code << "\n\n";
+      EXPECT_EQ(ExpectedIncompleteFormat, !Status.FormatComplete)
+          << Code << "\n\n";
     }
     ReplacementCount = Replaces.size();
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 

unittests/Format/FormatTestComments.cpp

protected:
   };
 
   std::string format(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle(),
                      IncompleteCheck CheckIncomplete = IC_ExpectComplete) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
-    bool IncompleteFormat = false;
+    FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
-        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
     if (CheckIncomplete != IC_DoNotCheck) {
       bool ExpectedIncompleteFormat = CheckIncomplete == IC_ExpectIncomplete;
-      EXPECT_EQ(ExpectedIncompleteFormat, IncompleteFormat) << Code << "\n\n";
+      EXPECT_EQ(ExpectedIncompleteFormat, !Status.FormatComplete)
+          << Code << "\n\n";
     }
     ReplacementCount = Replaces.size();
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 

unittests/Format/FormatTestJS.cpp

 
 class FormatTestJS : public ::testing::Test {
 protected:
   static std::string format(llvm::StringRef Code, unsigned Offset,
                             unsigned Length, const FormatStyle &Style) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(Offset, Length));
-    bool IncompleteFormat = false;
+    FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
-        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
-    EXPECT_FALSE(IncompleteFormat);
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
+    EXPECT_TRUE(Status.FormatComplete);
     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,

unittests/Format/FormatTestObjC.cpp

enum IncompleteCheck {
     IC_DoNotCheck
   };
 
   std::string format(llvm::StringRef Code,
                      IncompleteCheck CheckIncomplete = IC_ExpectComplete) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(0, Code.size()));
-    bool IncompleteFormat = false;
+    FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
-        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
     if (CheckIncomplete != IC_DoNotCheck) {
       bool ExpectedIncompleteFormat = CheckIncomplete == IC_ExpectIncomplete;
-      EXPECT_EQ(ExpectedIncompleteFormat, IncompleteFormat) << Code << "\n\n";
+      EXPECT_EQ(ExpectedIncompleteFormat, !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;
   }
 
   void verifyFormat(StringRef Code) {

unittests/Format/FormatTestSelective.cpp

 namespace {
 
 class FormatTestSelective : public ::testing::Test {
 protected:
   std::string format(llvm::StringRef Code, unsigned Offset, unsigned Length) {
     DEBUG(llvm::errs() << "---\n");
     DEBUG(llvm::errs() << Code << "\n\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(Offset, Length));
-    bool IncompleteFormat = false;
+    FormattingAttemptStatus Status;
     tooling::Replacements Replaces =
-        reformat(Style, Code, Ranges, "<stdin>", &IncompleteFormat);
-    EXPECT_FALSE(IncompleteFormat) << Code << "\n\n";
+        reformat(Style, Code, Ranges, "<stdin>", &Status);
+    EXPECT_TRUE(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;
   }
 
   FormatStyle Style = getLLVMStyle();
 };