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

[sammccall]

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.

[sammccall]

@hokein @alexfh There is a test that asserts clang-tidy exits with 0 when processing a nonexistent file (test added in D17335 which you wrote/reviewed).
This seems like a bug to me, do I need to preserve this behavior? (See patch description for how this happens).

The command is clang-tidy --checks="-*,modernize-use-nullptr" -p /some/path /some/path/not-exist.

Behavior at HEAD is:

Error while processing /some/path/not-exist.
Suppressed 1 warnings (1 with check filters).
<exit with status 0>

More natural and possibly correct behavior (flushing all diagnostics):

Error while processing /some/path/not-exist.
error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).
<exit with status 1>

[alexfh]

LG in general.

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.

This looks like a bug indeed. Looking at https://reviews.llvm.org/D17335 where the test was added, I can't see any evidence of the opposite. Let's try fixing this (and the test) unless Haojian knows reasons to leave the current behavior.

unittests/clang-tidy/ClangTidyTest.h

128–129

Please specify the type explicitly, since it's not obvious from the context. http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

[alexfh]

error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

That's preferred to the current behavior, but from a user perspective a more descriptive message would be better (like "can't find file /some/file" or "can't open file /some/file"). I don't know whether it's better to do in clang proper or in clang-tidy, but it can be handled separately from this patch.

[sammccall]

Remove backwards-compat behavior around ClangdDiagnosticConsumer::finish(), it's presumably a bug
Update test to verify we fail but don't crash in that case.
auto -> real type

[sammccall]

Thanks Alex. @hokein, let me know if any concerns.

In D53953#1283044, @alexfh wrote:

error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

That's preferred to the current behavior, but from a user perspective a more descriptive message would be better (like "can't find file /some/file" or "can't open file /some/file"). I don't know whether it's better to do in clang proper or in clang-tidy, but it can be handled separately from this patch.

Agreed, this error message template is really weird by itself, and the empty string makes it worse.
Will dig into it, agree it doesn't seem directly related.

[alexfh]

LG with a nit.

clang-tidy/ClangTidyDiagnosticConsumer.h

252

Not used now.

[sammccall]

Removed unused var

[sammccall]

BTW, the missing "X doesn't exist" is because Tooling.cpp turns off driver diagnostics for nonexistent files, because clang/driver wasn't VFS-aware... in 2011.
I'll send patches to fix this if there isn't too much fallout (Driver still needs to be fixed to use its VFS in this case, and then Tooling needs to enable the check).

This means you'll get:

error: no such file or directory: '/path/to/file.cpp'
error: no input files

Clang-tidy will decorate these with [clang-diagnostic-error], but that's a separate bug I guess.

[alexfh]

LG

[hokein]

There is a test that asserts clang-tidy exits with 0 when processing a nonexistent file (test added in D17335 which you wrote/reviewed).

This seems like a bug to me, do I need to preserve this behavior? (See patch description for how this happens).

I think this is a bug, we should return non-zero code when processing a non-existing file. The new behavior seems better to me, though we need to polish the error message.

clang-tidy/ClangTidy.h

247–248

nit: maybe name it Errors, and we match the comment again (it is out-of-date at HEAD :().

[sammccall]

In D53953#1285244, @hokein wrote:

I think this is a bug, we should return non-zero code when processing a non-existing file. The new behavior seems better to me, though we need to polish the error message.

Thanks. See D53958 for (most of) the needed error message improvements.

[sammccall]

Address comment, rebase

[• galapoz1996]

This comment was removed by mehdi_amini.

[• MillyMe]

This comment was removed by mehdi_amini.

clang-tidy/ClangTidy.h

 
 /// \brief Run a set of clang-tidy checks on a set of files.
 ///
 /// \param EnableCheckProfile If provided, it enables check profile collection
 /// in MatchFinder, and will contain the result of the profile.
 /// \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,
 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
 /// \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);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
 void exportReplacements(StringRef MainFilePath,
                         const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H

clang-tidy/ClangTidy.cpp

getCheckOptions(const ClangTidyOptions &Options,
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options),
       AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   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,
   ClangTool Tool(Compilations, InputFiles,
                  std::make_shared<PCHContainerOperations>(), BaseFS);
 
   // Add extra arguments passed by the clang-tidy command-line.
   ArgumentsAdjuster PerFileExtraArgumentsInserter =
       [&Context](const CommandLineArguments &Args, StringRef Filename) {
         ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
         CommandLineArguments AdjustedArgs = Args;
private:
       ClangTidyASTConsumerFactory *Factory;
     };
 
     ClangTidyASTConsumerFactory ConsumerFactory;
   };
 
   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);
   llvm::vfs::FileSystem &FileSystem =
       *Reporter.getSourceManager().getFileManager().getVirtualFileSystem();
   auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
   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.
       //
       // Change the directory to the one used during the analysis.
       FileSystem.setCurrentWorkingDirectory(Error.BuildDirectory);
     }
     Reporter.reportDiagnostic(Error);

clang-tidy/ClangTidyDiagnosticConsumer.h

public:
   /// \brief Returns options for \c File. Does not change or depend on
   /// \c CurrentFile.
   ClangTidyOptions getOptionsForFile(StringRef File) const;
 
   /// \brief Returns \c ClangTidyStats containing issued and ignored diagnostic
   /// 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; }
 
   /// \brief Control storage of profile date.
   void setProfileStoragePrefix(StringRef ProfilePrefix);
   llvm::Optional<ClangTidyProfiling::StorageParams>
   getProfileStorageParams() const;
bool canEnableAnalyzerAlphaCheckers() const {
     return AllowEnablingAnalyzerAlphaCheckers;
   }
 
 private:
   // Sets DiagEngine and Errors.
   friend class ClangTidyDiagnosticConsumer;
   friend class ClangTidyPluginAction;
 
-  std::vector<ClangTidyError> Errors;
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   class CachedGlobList;
   std::unique_ptr<CachedGlobList> CheckFilter;
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
                               bool RemoveIncompatibleErrors = true);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
   // library.
   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.
   llvm::Regex *getHeaderFilter();
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   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;
   bool LastErrorWasIgnored;
 };
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H

clang-tidy/ClangTidyDiagnosticConsumer.cpp

 
 llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
   if (!HeaderFilter)
     HeaderFilter =
         llvm::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
   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.
   // An event here consists of the opening or closing of an interval. During the
   // process, we maintain a counter with the amount of open intervals. If we
   // find an endpoint of an interval and this counter is different from 0, it
   // means that this interval overlaps with another one, so we set it as
   // inapplicable.

 struct EqualClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
     LessClangTidyError Less;
     return !Less(LHS, RHS) && !Less(RHS, LHS);
   }
 };
 } // 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);
 }

clang-tidy/tool/ClangTidyMain.cpp

static int clangTidyMain(int argc, const char **argv) {
   }
 
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider),
                            AllowEnablingAnalyzerAlphaCheckers);
+  std::vector<ClangTidyError> Errors =
       runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
                    EnableCheckProfile, ProfilePrefix);
-  ArrayRef<ClangTidyError> Errors = Context.getErrors();
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
   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()) {
     std::error_code EC;
     llvm::raw_fd_ostream OS(ExportFixes, EC, llvm::sys::fs::F_None);
     if (EC) {
       llvm::errs() << "Error opening output file: " << EC.message() << '\n';
       return 1;

test/clang-tidy/clang-tidy-run-with-database.cpp

 // RUN: mkdir -p %T/compilation-database-test/include
 // RUN: mkdir -p %T/compilation-database-test/a
 // RUN: mkdir -p %T/compilation-database-test/b
 // RUN: echo 'int *AA = 0;' > %T/compilation-database-test/a/a.cpp
 // RUN: echo 'int *AB = 0;' > %T/compilation-database-test/a/b.cpp
 // RUN: echo 'int *BB = 0;' > %T/compilation-database-test/b/b.cpp
 // RUN: echo 'int *BC = 0;' > %T/compilation-database-test/b/c.cpp
 // 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
 // RUN: FileCheck -input-file=%T/compilation-database-test/b/b.cpp %s -check-prefix=CHECK-FIX3
 // RUN: FileCheck -input-file=%T/compilation-database-test/b/c.cpp %s -check-prefix=CHECK-FIX4
 // RUN: FileCheck -input-file=%T/compilation-database-test/include/header.h %s -check-prefix=CHECK-FIX5
 
 // CHECK-FIX1: int *AA = nullptr;
 // CHECK-FIX2: int *AB = nullptr;
 // CHECK-FIX3: int *BB = nullptr;
 // CHECK-FIX4: int *BC = nullptr;
 // CHECK-FIX5: int *HP = nullptr;

unittests/clang-tidy/ClangTidyTest.h

runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
   for (const auto &FileContent : PathsToContent) {
     InMemoryFileSystem->addFile(
         Twine("include/") + FileContent.first, 0,
         llvm::MemoryBuffer::getMemBuffer(FileContent.second));
   }
   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);
         // FIXME: better error handling. Keep the behavior for now.
         if (Err) {
           llvm::errs() << llvm::toString(std::move(Err)) << "\n";
           return "";
         }
       }
     }
   }
   if (Errors)
-    *Errors = Context.getErrors();
+    *Errors = std::move(Diags);
   auto Result = tooling::applyAllReplacements(Code, Fixes);
   if (!Result) {
     // FIXME: propogate the error.
     llvm::consumeError(Result.takeError());
     return "";
   }
   return *Result;
 }