[clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC

Summary:
Currently ClangTidyContext::diag() sends the diagnostics to a
DiagnosticsEngine, which probably delegates to a ClangTidyDiagnosticsConsumer,
which is supposed to go back and populate ClangTidyContext::Errors.

After this patch, the diagnostics are stored in the ClangTidyDiagnosticsConsumer
itself and can be retrieved from there.

Why?
 - the round-trip from context -> engine -> consumer -> context is confusing
   and makes it harder to establish layering between these things.
 - context does too many things, and makes it hard to use clang-tidy as a library
 - everyone who actually wants the diagnostics has access to the ClangTidyDiagnosticsConsumer

The most natural implementation (ClangTidyDiagnosticsConsumer::take()
finalizes diagnostics) causes a test failure: clang-tidy-run-with-database.cpp
asserts that clang-tidy exits successfully when trying to process a file
that doesn't exist.
In clang-tidy today, this happens because finish() is never called, so the
diagnostic is never flushed. This looks like a bug to me.
For now, this patch carefully preserves that behavior, but I'll ping the
authors to see whether it's deliberate and worth preserving.

Reviewers: hokein

Subscribers: xazax.hun, cfe-commits, alexfh

Differential Revision: https://reviews.llvm.org/D53953

llvm-svn: 345961

7 files changed, 42 insertions, 44 deletions

diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 9ab85b61389..f0126e8668b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -500,11 +500,12 @@ getCheckOptions(const ClangTidyOptions &Options,
   return Factory.getCheckOptions();
 }
 
-void runClangTidy(clang::tidy::ClangTidyContext &Context,
-                  const CompilationDatabase &Compilations,
-                  ArrayRef<std::string> InputFiles,
-                  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-                  bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
+std::vector<ClangTidyError>
+runClangTidy(clang::tidy::ClangTidyContext &Context,
+             const CompilationDatabase &Compilations,
+             ArrayRef<std::string> InputFiles,
+             llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+             bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
   ClangTool Tool(Compilations, InputFiles,
                  std::make_shared<PCHContainerOperations>(), BaseFS);
 
@@ -586,9 +587,11 @@ void runClangTidy(clang::tidy::ClangTidyContext &Context,
 
   ActionFactory Factory(Context);
   Tool.run(&Factory);
+  return DiagConsumer.take();
 }
 
-void handleErrors(ClangTidyContext &Context, bool Fix,
+void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
+                  ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
   ErrorReporter Reporter(Context, Fix, BaseFS);
@@ -598,7 +601,7 @@ void handleErrors(ClangTidyContext &Context, bool Fix,
   if (!InitialWorkingDir)
     llvm::report_fatal_error("Cannot get current working path.");
 
-  for (const ClangTidyError &Error : Context.getErrors()) {
+  for (const ClangTidyError &Error : Errors) {
     if (!Error.BuildDirectory.empty()) {
       // By default, the working directory of file system is the current
       // clang-tidy running directory.
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.h b/clang-tools-extra/clang-tidy/ClangTidy.h
index 031a7a2b3b7..dc11200dfac 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.h
+++ b/clang-tools-extra/clang-tidy/ClangTidy.h
@@ -230,12 +230,13 @@ getCheckOptions(const ClangTidyOptions &Options,
 /// \param StoreCheckProfile If provided, and EnableCheckProfile is true,
 /// the profile will not be output to stderr, but will instead be stored
 /// as a JSON file in the specified directory.
-void runClangTidy(clang::tidy::ClangTidyContext &Context,
-                  const tooling::CompilationDatabase &Compilations,
-                  ArrayRef<std::string> InputFiles,
-                  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-                  bool EnableCheckProfile = false,
-                  llvm::StringRef StoreCheckProfile = StringRef());
+std::vector<ClangTidyError>
+runClangTidy(clang::tidy::ClangTidyContext &Context,
+             const tooling::CompilationDatabase &Compilations,
+             ArrayRef<std::string> InputFiles,
+             llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+             bool EnableCheckProfile = false,
+             llvm::StringRef StoreCheckProfile = StringRef());
 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
@@ -243,7 +244,8 @@ void runClangTidy(clang::tidy::ClangTidyContext &Context,
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
-void handleErrors(ClangTidyContext &Context, bool Fix,
+void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
+                  ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 55ba044c257..ae3b3301411 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -525,8 +525,7 @@ llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
   return HeaderFilter.get();
 }
 
-void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
-    SmallVectorImpl<ClangTidyError> &Errors) const {
+void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
   // Each error is modelled as the set of intervals in which it applies
   // replacements. To detect overlapping replacements, we use a sweep line
   // algorithm over these sets of intervals.
@@ -664,17 +663,13 @@ struct EqualClangTidyError {
 };
 } // end anonymous namespace
 
-// Flushes the internal diagnostics buffer to the ClangTidyContext.
-void ClangTidyDiagnosticConsumer::finish() {
+std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() {
   finalizeLastError();
 
   std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
   Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()),
                Errors.end());
-
   if (RemoveIncompatibleErrors)
-    removeIncompatibleErrors(Errors);
-
-  std::move(Errors.begin(), Errors.end(), std::back_inserter(Context.Errors));
-  Errors.clear();
+    removeIncompatibleErrors();
+  return std::move(Errors);
 }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index c138b36d846..3266f37260f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -160,12 +160,6 @@ public:
   /// counters.
   const ClangTidyStats &getStats() const { return Stats; }
 
-  /// \brief Returns all collected errors.
-  ArrayRef<ClangTidyError> getErrors() const { return Errors; }
-
-  /// \brief Clears collected errors.
-  void clearErrors() { Errors.clear(); }
-
   /// \brief Control profile collection in clang-tidy.
   void setEnableProfiling(bool Profile);
   bool getEnableProfiling() const { return Profile; }
@@ -196,7 +190,6 @@ private:
   friend class ClangTidyDiagnosticConsumer;
   friend class ClangTidyPluginAction;
 
-  std::vector<ClangTidyError> Errors;
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
@@ -236,13 +229,12 @@ public:
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
-  /// \brief Flushes the internal diagnostics buffer to the ClangTidyContext.
-  void finish() override;
+  // Retrieve the diagnostics that were captured.
+  std::vector<ClangTidyError> take();
 
 private:
   void finalizeLastError();
-
-  void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors) const;
+  void removeIncompatibleErrors();
 
   /// \brief Returns the \c HeaderFilter constructed for the options set in the
   /// context.
@@ -256,7 +248,7 @@ private:
   ClangTidyContext &Context;
   bool RemoveIncompatibleErrors;
   std::unique_ptr<DiagnosticsEngine> Diags;
-  SmallVector<ClangTidyError, 8> Errors;
+  std::vector<ClangTidyError> Errors;
   std::unique_ptr<llvm::Regex> HeaderFilter;
   bool LastErrorRelatesToUserCode;
   bool LastErrorPassesLineFilter;
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index d272a482ad7..8a16dbd3403 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -422,9 +422,9 @@ static int clangTidyMain(int argc, const char **argv) {
 
   ClangTidyContext Context(std::move(OwningOptionsProvider),
                            AllowEnablingAnalyzerAlphaCheckers);
-  runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-               EnableCheckProfile, ProfilePrefix);
-  ArrayRef<ClangTidyError> Errors = Context.getErrors();
+  std::vector<ClangTidyError> Errors =
+      runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
+                   EnableCheckProfile, ProfilePrefix);
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
@@ -434,7 +434,7 @@ static int clangTidyMain(int argc, const char **argv) {
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
+  handleErrors(Errors, Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
                BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
diff --git a/clang-tools-extra/test/clang-tidy/clang-tidy-run-with-database.cpp b/clang-tools-extra/test/clang-tidy/clang-tidy-run-with-database.cpp
index 05bc14c4fa0..f9b2955c6fd 100644
--- a/clang-tools-extra/test/clang-tidy/clang-tidy-run-with-database.cpp
+++ b/clang-tools-extra/test/clang-tidy/clang-tidy-run-with-database.cpp
@@ -8,7 +8,13 @@
 // RUN: echo 'int *HP = 0;' > %T/compilation-database-test/include/header.h
 // RUN: echo '#include "header.h"' > %T/compilation-database-test/b/d.cpp
 // RUN: sed 's|test_dir|%/T/compilation-database-test|g' %S/Inputs/compilation-database/template.json > %T/compile_commands.json
-// RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/b/not-exist -header-filter=.*
+
+// Regression test: shouldn't crash.
+// RUN: not clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/b/not-exist -header-filter=.* 2>&1 | FileCheck %s -check-prefix=CHECK-NOT-EXIST
+// CHECK-NOT-EXIST: Error while processing {{.*}}/not-exist.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK-NOT-EXIST: Found compiler error
+
 // RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/a/a.cpp %T/compilation-database-test/a/b.cpp %T/compilation-database-test/b/b.cpp %T/compilation-database-test/b/c.cpp %T/compilation-database-test/b/d.cpp -header-filter=.* -fix
 // RUN: FileCheck -input-file=%T/compilation-database-test/a/a.cpp %s -check-prefix=CHECK-FIX1
 // RUN: FileCheck -input-file=%T/compilation-database-test/a/b.cpp %s -check-prefix=CHECK-FIX2
diff --git a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
index 2d2675489aa..f6e8e8efdcf 100644
--- a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
+++ b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
@@ -118,15 +118,15 @@ runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
   Invocation.setDiagnosticConsumer(&DiagConsumer);
   if (!Invocation.run()) {
     std::string ErrorText;
-    for (const auto &Error : Context.getErrors()) {
+    for (const auto &Error : DiagConsumer.take()) {
       ErrorText += Error.Message.Message + "\n";
     }
     llvm::report_fatal_error(ErrorText);
   }
 
-  DiagConsumer.finish();
   tooling::Replacements Fixes;
-  for (const ClangTidyError &Error : Context.getErrors()) {
+  std::vector<ClangTidyError> Diags = DiagConsumer.take();
+  for (const ClangTidyError &Error : Diags) {
     for (const auto &FileAndFixes : Error.Fix) {
       for (const auto &Fix : FileAndFixes.second) {
         auto Err = Fixes.add(Fix);
@@ -139,7 +139,7 @@ runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
     }
   }
   if (Errors)
-    *Errors = Context.getErrors();
+    *Errors = std::move(Diags);
   auto Result = tooling::applyAllReplacements(Code, Fixes);
   if (!Result) {
     // FIXME: propogate the error.