[clang-tidy] Align usage of ClangTool interface with new VFS injection

[whisperity]

Summary

Aligns the interface landed in D41535 to the patch which makes VFS injection more user-friendly. The interface how ClangTidy and the error reporter gets the filesystem to use has also been made more explicit: now ErrorReporter gets the exact Clang Tool('s filesystem layout) that Clang-Tidy used to run checks.

[whisperity]

Update to be in line with contents in dependency patch.

[ilya-biryukov]

clang-tidy/ClangTidy.cpp

91–93

Why do we need to change the signature of ErrorReporter constructor?
Broadening the accepted interface does not seem right. It only needs the vfs and the clients could get vfs from ClangTool themselves.

clang-tidy/ClangTidy.h

229

I'm not sure if passing ClangTool here is an improvement.
Let's ask someone more familiar with clang-tidy. @hokein, @alexfh, WDYT?

clang-tidy/tool/CMakeLists.txt

19

Why do we need an extra dependency?

clang-tidy/tool/ClangTidyMain.cpp

432–433

Maybe use a shorter name, e.g. FileSystem?

[whisperity]

clang-tidy/ClangTidy.cpp

91–93

Is it okay interface-wise if the FS used by the ErrorReporter is not the same as the one used by the module that produced the errors? It seems like an undocumented consensus/convention that the same VFS should have been passed here.

clang-tidy/tool/CMakeLists.txt

19

In the current state of the patch, clang-tidy/tool/ClangTidyMain.cpp constructs the ClangTool which constructor requires a std::shared_ptr<PCHContainerOperations>. PCHContainerOperations's definition and implementation is part of the clangFrontend library, and without this dependency, there would be a symbol resolution error.

[ilya-biryukov]

clang-tidy/ClangTidy.cpp

91–93

I find actually documenting that the vfs should be the same to be a better option. Or even sharing the FileManager, but that might be more involved.

The problem of passing ClangTool is that it's a much more powerful than what we need (e.g. it allows to rerun clang-tidy, which is certainly not something that ErrorReporter should do).

clang-tidy/tool/CMakeLists.txt

19

Thanks for clarification.
Does it mean that to use ClangTool one needs both a dependency on clangTooling and clangFrontend?
That's weird, given that clangTooling itself depends on clangFrontend and it would be nice if the buildsystem actually propagated those.

[whisperity]

clang-tidy/tool/CMakeLists.txt

19

I'm not sure if you need to have both as a dependency just to use ClangTool. If you want to construct an empty PCHContainerOperations to pass as an argument, though, it seems you do.

One can wonder where the default argument's (which is a shared pointer to a default constructed object) expression's evaluation is in the object code or if passing nullptr breaks something.

You need the dependency because it's your code who wants to run the constructor of the PCH class.

[whisperity]

Akin to D45094, pinging this too. 🙂

clang-tidy/ClangTidy.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H
 
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>
 #include <type_traits>
 #include <vector>
 
 namespace clang {
 

 /// Options.
 ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options);
 
 /// \brief Run a set of clang-tidy checks on a set of files.
 ///
 /// \param Profile if provided, it enables check profile collection in
 /// MatchFinder, and will contain the result of the profile.
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
-                  const tooling::CompilationDatabase &Compilations,
-                  ArrayRef<std::string> InputFiles,
-                  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS,
+                  clang::tooling::ClangTool &Tool,
                   ProfileData *Profile = nullptr);
 
 // 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,
                   unsigned &WarningsAsErrorsCount,
-                  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS);
+                  clang::tooling::ClangTool &Tool);
 
 /// \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

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
-#include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <algorithm>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;
public:
   bool supportsCrossFileDiagnostics() const override { return true; }
 
 private:
   ClangTidyContext &Context;
 };
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
-                llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS)
-      : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, ClangTool &Tool)
+      : Files(FileSystemOptions(), Tool.getFiles().getVirtualFileSystem()),
+        DiagOpts(new DiagnosticOptions()),
         DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
         SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes),
         TotalFixes(0), AppliedFixes(0), WarningsAsErrors(0) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }
ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options) {
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options));
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
-                  const CompilationDatabase &Compilations,
-                  ArrayRef<std::string> InputFiles,
-                  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS,
+                  ClangTool &Tool,
                   ProfileData *Profile) {
-  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;
         if (Opts.ExtraArgsBefore) {
           auto I = AdjustedArgs.begin();
           if (I != AdjustedArgs.end() && !StringRef(*I).startswith("-"))
void runClangTidy(clang::tidy::ClangTidyContext &Context,
   };
 
   ActionFactory Factory(Context);
   Tool.run(&Factory);
 }
 
 void handleErrors(ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
-                  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS) {
-  ErrorReporter Reporter(Context, Fix, BaseFS);
+                  ClangTool &Tool) {
+  ErrorReporter Reporter(Context, Fix, Tool);
   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()) {
     if (!Error.BuildDirectory.empty()) {

clang-tidy/tool/CMakeLists.txt

 add_dependencies(clang-tidy
   clang-headers
   )
 target_link_libraries(clang-tidy
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
+  clangFrontend
   clangTidy
   clangTidyAndroidModule
   clangTidyAbseilModule
   clangTidyBoostModule
   clangTidyBugproneModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyFuchsiaModule

clang-tidy/tool/ClangTidyMain.cpp

if (!Config.empty()) {
     }
   }
   return llvm::make_unique<FileOptionsProvider>(GlobalOptions, DefaultOptions,
                                                 OverrideOptions);
 }
 
 llvm::IntrusiveRefCntPtr<vfs::FileSystem>
 getVfsOverlayFromFile(const std::string &OverlayFile) {
-  llvm::IntrusiveRefCntPtr<vfs::OverlayFileSystem> OverlayFS(
-      new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
-      OverlayFS->getBufferForFile(OverlayFile);
+      vfs::getRealFileSystem()->getBufferForFile(OverlayFile);
   if (!Buffer) {
     llvm::errs() << "Can't load virtual filesystem overlay file '"
                  << OverlayFile << "': " << Buffer.getError().message()
                  << ".\n";
     return nullptr;
   }
 
   IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getVFSFromYAML(
       std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
   if (!FS) {
     llvm::errs() << "Error: invalid virtual filesystem overlay file '"
                  << OverlayFile << "'.\n";
     return nullptr;
   }
-  OverlayFS->pushOverlay(FS);
-  return OverlayFS;
+  return FS;
 }
 
 static int clangTidyMain(int argc, const char **argv) {
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
                                     cl::ZeroOrMore);
 
   auto OwningOptionsProvider = createOptionsProvider();
   auto *OptionsProvider = OwningOptionsProvider.get();
if (EnabledChecks.empty()) {
     return 1;
   }
 
   if (PathList.empty()) {
     llvm::errs() << "Error: no input files specified.\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return 1;
   }
-  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS(
-      VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr<vfs::FileSystem> VirtualFileSystem(
+      VfsOverlay.empty() ? nullptr
                          : getVfsOverlayFromFile(VfsOverlay));
-  if (!BaseFS)
+  if (!VfsOverlay.empty() && !VirtualFileSystem)
     return 1;
 
   ProfileData Profile;
 
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
-  runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-               EnableCheckProfile ? &Profile : nullptr);
+  ClangTool Tool(OptionsParser.getCompilations(), PathList,
+                 std::make_shared<PCHContainerOperations>(),
+                 VirtualFileSystem, true);
+  runClangTidy(Context, Tool, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef<ClangTidyError> Errors = Context.getErrors();
   bool FoundErrors =
       std::find_if(Errors.begin(), Errors.end(), [](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,
-               BaseFS);
+               Tool);
 
   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;
     }