[clang-tidy] Expand modular headers for PPCallbacks

[alexfh]

Summary

Add a way to expand modular headers for PPCallbacks. Checks can opt-in for this
expansion by overriding the new registerPPCallbacks virtual method and
registering their PPCallbacks in the preprocessor created for this specific
purpose.

Use module expansion in the readability-identifier-naming check

[alexfh]

• Fix a typo in the comment. Rearrange paragraphs to make the comment more readable.

[alexfh]

• Fix naming.

[gribozavr]

clang-tools-extra/clang-tidy/ClangTidy.h

156 ↗

(On Diff #191230)

Please document that ModuleExpanderPP also provides events from the main file (until I read the implementation in the check, I thought the check should subscribe to both preprocessors).

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp

19 ↗

(On Diff #191230)

Three slashes in doc comments (everywhere in the patch).

20 ↗

(On Diff #191230)

"Records that a given file entry is needed for replaying callbacks."

"addNecessaryFile"?

23 ↗

(On Diff #191230)

"recordFileContent"?

24 ↗

(On Diff #191230)

const & for File and ContentCache ?

38 ↗

(On Diff #191230)

"remove the file from the set of necessary files..."

71 ↗

(On Diff #191230)

of => off

However I don't see value in a comment that repeats the source code.

115 ↗

(On Diff #191230)

Recorder->addFileToRecord(IF.getFile())

The intermediate variable doesn't add anything...

171 ↗

(On Diff #191230)

What's target_callbacks_? It is only mentioned in comments.

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h

9 ↗

(On Diff #191230)

This looks like a documentation comment for the class itself, I think it should be moved there.

43 ↗

(On Diff #191230)

"Returns the preprocessor that provides callbacks for contents of modular headers.

This preprocessor is separate from the one used by the rest of the compiler."

clang-tools-extra/test/clang-tidy/expand-modular-headers-ppcallbacks.cpp

31 ↗

(On Diff #191230)

Please also add a check for a diagnostic coming from the main file.

[gribozavr]

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h

32 ↗

(On Diff #191230)

replays => re-runs

[alexfh]

• Addressed review comments. Parse code to the end (seems right to do this, but no specific test that would fail otherwise).

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp

24 ↗

(On Diff #191230)

Done for ContentCache. The File is stored into a DenseSet, thus a pointer makes sense here.

171 ↗

(On Diff #191230)

It's an artifact of an older version. Removed.

[alexfh]

Thank you for the review! I hope I covered all the points.

[gribozavr]

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp

166 ↗

(On Diff #191703)

Haha, so the test that I asked to add did catch a bug -- just not the one I expected :)

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h

44 ↗

(On Diff #191703)

"... callbacks for the whole translation unit, including the main file, textual headers, and modular headers."

Sorry, I wrote the comment before I fully understood what this preprocessor does.

[alexfh]

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp

166 ↗

(On Diff #191703)

Nope, the test passes without this. I'm not even sure this override is changing any observable behavior of PPCallbacks. I hope to find this out by running other checks on real code with modules enabled.

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h

44 ↗

(On Diff #191703)

Indeed. Thanks for the text.

[alexfh]

• - Update a comment according to the review comments.

[uabelho]

Hi,

I noticed that with this commit I get a whole bunch (~40) of warnings like the below when compiling with gcc 7.4:

[10/16] Building CXX object tools/clang/tools/extra/clang-tidy/utils/CMakeFiles/clangTidyUtils.dir/HeaderGuard.cpp.o
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:12:0,
                 from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:
../tools/clang/tools/extra/clang-tidy/utils/../ClangTidy.h:161:16: warning: 'virtual void clang::tidy::ClangTidyCheck::registerPPCallbacks(const clang::SourceManager&, clang::Preprocessor*, clang::Preprocessor*)' was hidden [-Woverloaded-virtual]
   virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                ^~~~~~~~~~~~~~~~~~~
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:0:
../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:35:8: warning:   by 'virtual void clang::tidy::utils::HeaderGuardCheck::registerPPCallbacks(clang::CompilerInstance&)' [-Woverloaded-virtual]
   void registerPPCallbacks(CompilerInstance &Compiler) override;
        ^~~~~~~~~~~~~~~~~~~

One of the registerPPCallbacks methods has the following comment

/// DEPRECATED: Use the other overload.
virtual void registerPPCallbacks(CompilerInstance &Compiler) {}

so I suppose it will be removed at some point?

I'm not super familiar with the warning, but there is at least one discussion about it here:
https://stackoverflow.com/questions/9995421/gcc-woverloaded-virtual-warnings

[PiotrZSL]

Looks like this change is responsible for https://github.com/llvm/llvm-project/issues/59807 and https://github.com/llvm/llvm-project/issues/62337 issues.

clang-tools-extra/trunk/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangTidy
   ClangTidy.cpp
   ClangTidyModule.cpp
   ClangTidyDiagnosticConsumer.cpp
   ClangTidyOptions.cpp
   ClangTidyProfiling.cpp
+  ExpandModularHeadersPPCallbacks.cpp
 
   DEPENDS
   ClangSACheckers
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic

clang-tools-extra/trunk/clang-tidy/ClangTidy.h

public:
   }
 
   /// \brief Override this to register ``PPCallbacks`` with ``Compiler``.
   ///
   /// This should be used for clang-tidy checks that analyze preprocessor-
   /// dependent properties, e.g. the order of include directives.
   virtual void registerPPCallbacks(CompilerInstance &Compiler) {}
 
+  /// \brief Override this to register ``PPCallbacks`` in the preprocessor.
+  ///
+  /// This should be used for clang-tidy checks that analyze preprocessor-
+  /// dependent properties, e.g. include directives and macro definitions.
+  ///
+  /// There are two Preprocessors to choose from that differ in how they handle
+  /// modular #includes:
+  ///  - PP is the real Preprocessor. It doesn't walk into modular #includes and
+  ///    thus doesn't generate PPCallbacks for their contents.
+  ///  - ModuleExpanderPP preprocesses the whole translation unit in the
+  ///    non-modular mode, which allows it to generate PPCallbacks not only for
+  ///    the main file and textual headers, but also for all transitively
+  ///    included modular headers when the analysis runs with modules enabled.
+  ///    When modules are not enabled ModuleExpanderPP just points to the real
+  ///    preprocessor.
+  virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                                   Preprocessor *ModuleExpanderPP) {}
+
   /// \brief Override this to register AST matchers with \p Finder.
   ///
   /// This should be used by clang-tidy checks that analyze code properties that
   /// dependent on AST knowledge.
   ///
   /// You can register as many matchers as necessary with \p Finder. Usually,
   /// "this" will be used as callback, but you can also specify other callback
   /// classes. Thereby, different matchers can trigger different callbacks.
protected:
   /// \brief Returns the language options from the context.
   LangOptions getLangOpts() const { return Context->getLangOpts(); }
 };
 
 class ClangTidyCheckFactories;
 
 class ClangTidyASTConsumerFactory {
 public:
-  ClangTidyASTConsumerFactory(ClangTidyContext &Context);
+  ClangTidyASTConsumerFactory(
+      ClangTidyContext &Context,
+      IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS = nullptr);
 
   /// \brief Returns an ASTConsumer that runs the specified clang-tidy checks.
   std::unique_ptr<clang::ASTConsumer>
   CreateASTConsumer(clang::CompilerInstance &Compiler, StringRef File);
 
   /// \brief Get the list of enabled checks.
   std::vector<std::string> getCheckNames();
 
   /// \brief Get the union of options from all checks.
   ClangTidyOptions::OptionMap getCheckOptions();
 
 private:
   ClangTidyContext &Context;
+  IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS;
   std::unique_ptr<ClangTidyCheckFactories> CheckFactories;
 };
 
 /// \brief Fills the list of check names that are enabled when the provided
 /// filters are applied.
 std::vector<std::string> getCheckNames(const ClangTidyOptions &Options,
                                        bool AllowEnablingAnalyzerAlphaCheckers);
 

 /// 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.
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
              const tooling::CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
-             llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+             llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> 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.
 //
 /// \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-tools-extra/trunk/clang-tidy/ClangTidy.cpp

 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidy.h"
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyModuleRegistry.h"
 #include "ClangTidyProfiling.h"
+#include "ExpandModularHeadersPPCallbacks.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Config/config.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/ASTConsumers.h"
 #include "clang/Frontend/CompilerInstance.h"
private:
   std::unique_ptr<ClangTidyProfiling> Profiling;
   std::unique_ptr<ast_matchers::MatchFinder> Finder;
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
 };
 
 } // namespace
 
 ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
-    ClangTidyContext &Context)
-    : Context(Context), CheckFactories(new ClangTidyCheckFactories) {
+    ClangTidyContext &Context,
+    IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS)
+    : Context(Context), OverlayFS(OverlayFS),
+      CheckFactories(new ClangTidyCheckFactories) {
   for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
                                          E = ClangTidyModuleRegistry::end();
        I != E; ++I) {
     std::unique_ptr<ClangTidyModule> Module(I->instantiate());
     Module->addCheckFactories(*CheckFactories);
   }
 }
 

 }
 #endif // CLANG_ENABLE_STATIC_ANALYZER
 
 std::unique_ptr<clang::ASTConsumer>
 ClangTidyASTConsumerFactory::CreateASTConsumer(
     clang::CompilerInstance &Compiler, StringRef File) {
   // FIXME: Move this to a separate method, so that CreateASTConsumer doesn't
   // modify Compiler.
-  Context.setSourceManager(&Compiler.getSourceManager());
+  SourceManager *SM = &Compiler.getSourceManager();
+  Context.setSourceManager(SM);
   Context.setCurrentFile(File);
   Context.setASTContext(&Compiler.getASTContext());
 
   auto WorkingDir = Compiler.getSourceManager()
                         .getFileManager()
                         .getVirtualFileSystem()
                         ->getCurrentWorkingDirectory();
   if (WorkingDir)
if (Context.getEnableProfiling()) {
     Profiling = llvm::make_unique<ClangTidyProfiling>(
         Context.getProfileStorageParams());
     FinderOptions.CheckProfiling.emplace(Profiling->Records);
   }
 
   std::unique_ptr<ast_matchers::MatchFinder> Finder(
       new ast_matchers::MatchFinder(std::move(FinderOptions)));
 
+  Preprocessor *PP = &Compiler.getPreprocessor();
+  Preprocessor *ModuleExpanderPP = PP;
+
+  if (Context.getLangOpts().Modules && OverlayFS != nullptr) {
+    auto ModuleExpander = llvm::make_unique<ExpandModularHeadersPPCallbacks>(
+        &Compiler, OverlayFS);
+    ModuleExpanderPP = ModuleExpander->getPreprocessor();
+    PP->addPPCallbacks(std::move(ModuleExpander));
+  }
+
   for (auto &Check : Checks) {
     Check->registerMatchers(&*Finder);
     Check->registerPPCallbacks(Compiler);
+    Check->registerPPCallbacks(*SM, PP, ModuleExpanderPP);
   }
 
   std::vector<std::unique_ptr<ASTConsumer>> Consumers;
   if (!Checks.empty())
     Consumers.push_back(Finder->newASTConsumer());
 
 #if CLANG_ENABLE_STATIC_ANALYZER
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
getCheckOptions(const ClangTidyOptions &Options,
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
              const CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
-             llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+             llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
              bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
   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);
runClangTidy(clang::tidy::ClangTidyContext &Context,
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
   DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
                        &DiagConsumer, /*ShouldOwnClient=*/false);
   Context.setDiagnosticsEngine(&DE);
   Tool.setDiagnosticConsumer(&DiagConsumer);
 
   class ActionFactory : public FrontendActionFactory {
   public:
-    ActionFactory(ClangTidyContext &Context) : ConsumerFactory(Context) {}
+    ActionFactory(ClangTidyContext &Context,
+                  IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS)
+        : ConsumerFactory(Context, BaseFS) {}
     FrontendAction *create() override { return new Action(&ConsumerFactory); }
 
     bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
                        FileManager *Files,
                        std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                        DiagnosticConsumer *DiagConsumer) override {
       // Explicitly set ProgramAction to RunAnalysis to make the preprocessor
       // define __clang_analyzer__ macro. The frontend analyzer action will not
private:
 
     private:
       ClangTidyASTConsumerFactory *Factory;
     };
 
     ClangTidyASTConsumerFactory ConsumerFactory;
   };
 
-  ActionFactory Factory(Context);
+  ActionFactory Factory(Context, BaseFS);
   Tool.run(&Factory);
   return DiagConsumer.take();
 }
 
 void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
                   ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {

clang-tools-extra/trunk/clang-tidy/ExpandModularHeadersPPCallbacks.h

+//===- ExpandModularHeadersPPCallbacks.h - clang-tidy -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_EXPANDMODULARHEADERSPPCALLBACKS_H_
+#define LLVM_CLANG_TOOLING_EXPANDMODULARHEADERSPPCALLBACKS_H_
+
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/DenseSet.h"
+
+namespace clang {
+class CompilerInstance;
+
+namespace serialization {
+class ModuleFile;
+} // namespace serialization
+
+namespace tooling {
+
+/// \brief Handles PPCallbacks and re-runs preprocessing of the whole
+/// translation unit with modules disabled.
+///
+/// This way it's possible to get PPCallbacks for the whole translation unit
+/// including the contents of the modular headers and all their transitive
+/// includes.
+///
+/// This allows existing tools based on PPCallbacks to retain their functionality
+/// when running with C++ modules enabled. This only works in the backwards
+/// compatible modules mode, i.e. when code can still be parsed in non-modular
+/// way.
+class ExpandModularHeadersPPCallbacks : public PPCallbacks {
+public:
+  ExpandModularHeadersPPCallbacks(
+      CompilerInstance *Compiler,
+      IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS);
+  ~ExpandModularHeadersPPCallbacks();
+
+  /// \brief Returns the preprocessor that provides callbacks for the whole
+  /// translation unit, including the main file, textual headers, and modular
+  /// headers.
+  ///
+  /// This preprocessor is separate from the one used by the rest of the
+  /// compiler.
+  Preprocessor *getPreprocessor() const;
+
+private:
+  class FileRecorder;
+
+  void handleModuleFile(serialization::ModuleFile *MF);
+  void parseToLocation(SourceLocation Loc);
+
+  // Handle PPCallbacks.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind FileType,
+                   FileID PrevFID) override;
+
+  void InclusionDirective(SourceLocation DirectiveLoc,
+                          const Token &IncludeToken, StringRef IncludedFilename,
+                          bool IsAngled, CharSourceRange FilenameRange,
+                          const FileEntry *IncludedFile, StringRef SearchPath,
+                          StringRef RelativePath, const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override;
+
+  void EndOfMainFile() override;
+
+  // Handle all other callbacks.
+  // Just parse to the corresponding location to generate PPCallbacks for the
+  // corresponding range
+  void Ident(SourceLocation Loc, StringRef) override;
+  void PragmaDirective(SourceLocation Loc, PragmaIntroducerKind) override;
+  void PragmaComment(SourceLocation Loc, const IdentifierInfo *,
+                     StringRef) override;
+  void PragmaDetectMismatch(SourceLocation Loc, StringRef, StringRef) override;
+  void PragmaDebug(SourceLocation Loc, StringRef) override;
+  void PragmaMessage(SourceLocation Loc, StringRef, PragmaMessageKind,
+                     StringRef) override;
+  void PragmaDiagnosticPush(SourceLocation Loc, StringRef) override;
+  void PragmaDiagnosticPop(SourceLocation Loc, StringRef) override;
+  void PragmaDiagnostic(SourceLocation Loc, StringRef, diag::Severity,
+                        StringRef) override;
+  void HasInclude(SourceLocation Loc, StringRef, bool, const FileEntry *,
+                  SrcMgr::CharacteristicKind) override;
+  void PragmaOpenCLExtension(SourceLocation NameLoc, const IdentifierInfo *,
+                             SourceLocation StateLoc, unsigned) override;
+  void PragmaWarning(SourceLocation Loc, StringRef, ArrayRef<int>) override;
+  void PragmaWarningPush(SourceLocation Loc, int) override;
+  void PragmaWarningPop(SourceLocation Loc) override;
+  void PragmaAssumeNonNullBegin(SourceLocation Loc) override;
+  void PragmaAssumeNonNullEnd(SourceLocation Loc) override;
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &,
+                    SourceRange Range, const MacroArgs *) override;
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override;
+  void MacroUndefined(const Token &, const MacroDefinition &,
+                      const MacroDirective *Undef) override;
+  void Defined(const Token &MacroNameTok, const MacroDefinition &,
+               SourceRange Range) override;
+  void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+  void If(SourceLocation Loc, SourceRange, ConditionValueKind) override;
+  void Elif(SourceLocation Loc, SourceRange, ConditionValueKind,
+            SourceLocation) override;
+  void Ifdef(SourceLocation Loc, const Token &,
+             const MacroDefinition &) override;
+  void Ifndef(SourceLocation Loc, const Token &,
+              const MacroDefinition &) override;
+  void Else(SourceLocation Loc, SourceLocation) override;
+  void Endif(SourceLocation Loc, SourceLocation) override;
+
+  std::unique_ptr<FileRecorder> Recorder;
+  // Set of all the modules visited. Avoids processing a module more than once.
+  llvm::DenseSet<serialization::ModuleFile *> VisitedModules;
+
+  CompilerInstance &Compiler;
+  // Additional filesystem for replay. Provides all input files from modules.
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFs;
+
+  SourceManager &Sources;
+  DiagnosticsEngine Diags;
+  LangOptions LangOpts;
+  TrivialModuleLoader ModuleLoader;
+
+  std::unique_ptr<HeaderSearch> HeaderInfo;
+  std::unique_ptr<Preprocessor> Preprocessor;
+  bool EnteredMainFile = false;
+  bool StartedLexing = false;
+  Token CurrentToken;
+};
+
+} // namespace tooling
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLING_EXPANDMODULARHEADERSPPCALLBACKS_H_

clang-tools-extra/trunk/clang-tidy/ExpandModularHeadersPPCallbacks.cpp

+//===- ExpandModularHeadersPPCallbacks.h - clang-tidy -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExpandModularHeadersPPCallbacks.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTReader.h"
+
+namespace clang {
+namespace tooling {
+
+class ExpandModularHeadersPPCallbacks::FileRecorder {
+public:
+  /// Records that a given file entry is needed for replaying callbacks.
+  void addNecessaryFile(const FileEntry *File) { FilesToRecord.insert(File); }
+
+  /// Records content for a file and adds it to the FileSystem.
+  void recordFileContent(const FileEntry *File,
+                         const SrcMgr::ContentCache &ContentCache,
+                         llvm::vfs::InMemoryFileSystem &InMemoryFs) {
+    // Return if we are not interested in the contents of this file.
+    if (!FilesToRecord.count(File))
+      return;
+
+    // FIXME: Why is this happening? We might be losing contents here.
+    if (!ContentCache.getRawBuffer())
+      return;
+
+    InMemoryFs.addFile(File->getName(), /*ModificationTime=*/0,
+                       llvm::MemoryBuffer::getMemBufferCopy(
+                           ContentCache.getRawBuffer()->getBuffer()));
+    // Remove the file from the set of necessary files.
+    FilesToRecord.erase(File);
+  }
+
+  /// Makes sure we have contents for all the files we were interested in. Ideally
+  /// `FilesToRecord` should be empty.
+  void checkAllFilesRecorded() {
+    for (auto FileEntry : FilesToRecord)
+      llvm::errs() << "Did not record contents for input file: "
+                   << FileEntry->getName() << "\n";
+  }
+
+private:
+  /// A set of files whose contents are to be recorded.
+  llvm::DenseSet<const FileEntry *> FilesToRecord;
+};
+
+ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
+    CompilerInstance *CI,
+    IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS)
+    : Recorder(llvm::make_unique<FileRecorder>()), Compiler(*CI),
+      InMemoryFs(new llvm::vfs::InMemoryFileSystem),
+      Sources(Compiler.getSourceManager()),
+      // Forward the new diagnostics to the original DiagnosticConsumer.
+      Diags(new DiagnosticIDs, new DiagnosticOptions,
+            new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
+      LangOpts(Compiler.getLangOpts()) {
+  // Add a FileSystem containing the extra files needed in place of modular
+  // headers.
+  OverlayFS->pushOverlay(InMemoryFs);
+
+  Diags.setSourceManager(&Sources);
+
+  LangOpts.Modules = false;
+
+  auto HSO = std::make_shared<HeaderSearchOptions>();
+  *HSO = Compiler.getHeaderSearchOpts();
+
+  HeaderInfo = llvm::make_unique<HeaderSearch>(HSO, Sources, Diags, LangOpts,
+                                               &Compiler.getTarget());
+
+  auto PO = std::make_shared<PreprocessorOptions>();
+  *PO = Compiler.getPreprocessorOpts();
+
+  Preprocessor = llvm::make_unique<clang::Preprocessor>(
+      PO, Diags, LangOpts, Sources, *HeaderInfo, ModuleLoader,
+      /*IILookup=*/nullptr,
+      /*OwnsHeaderSearch=*/false);
+  Preprocessor->Initialize(Compiler.getTarget(), Compiler.getAuxTarget());
+  InitializePreprocessor(*Preprocessor, *PO, Compiler.getPCHContainerReader(),
+                         Compiler.getFrontendOpts());
+  ApplyHeaderSearchOptions(*HeaderInfo, *HSO, LangOpts,
+                           Compiler.getTarget().getTriple());
+}
+
+ExpandModularHeadersPPCallbacks::~ExpandModularHeadersPPCallbacks() = default;
+
+Preprocessor *ExpandModularHeadersPPCallbacks::getPreprocessor() const {
+  return Preprocessor.get();
+}
+
+void ExpandModularHeadersPPCallbacks::handleModuleFile(
+    serialization::ModuleFile *MF) {
+  if (!MF)
+    return;
+  // Avoid processing a ModuleFile more than once.
+  if (VisitedModules.count(MF))
+    return;
+  VisitedModules.insert(MF);
+
+  // Visit all the input files of this module and mark them to record their
+  // contents later.
+  Compiler.getModuleManager()->visitInputFiles(
+      *MF, true, false,
+      [this](const serialization::InputFile &IF, bool /*IsSystem*/) {
+        Recorder->addNecessaryFile(IF.getFile());
+      });
+  // Recursively handle all transitively imported modules.
+  for (auto Import : MF->Imports)
+    handleModuleFile(Import);
+}
+
+void ExpandModularHeadersPPCallbacks::parseToLocation(SourceLocation Loc) {
+  // Load all source locations present in the external sources.
+  for (unsigned I = 0, N = Sources.loaded_sloc_entry_size(); I != N; ++I) {
+    Sources.getLoadedSLocEntry(I, nullptr);
+  }
+  // Record contents of files we are interested in and add to the FileSystem.
+  for (auto It = Sources.fileinfo_begin(); It != Sources.fileinfo_end(); ++It) {
+    Recorder->recordFileContent(It->getFirst(), *It->getSecond(), *InMemoryFs);
+  }
+  Recorder->checkAllFilesRecorded();
+
+  if (!StartedLexing) {
+    StartedLexing = true;
+    Preprocessor->Lex(CurrentToken);
+  }
+  while (!CurrentToken.is(tok::eof) &&
+         Sources.isBeforeInTranslationUnit(CurrentToken.getLocation(), Loc)) {
+    Preprocessor->Lex(CurrentToken);
+  }
+}
+
+void ExpandModularHeadersPPCallbacks::FileChanged(
+    SourceLocation Loc, FileChangeReason Reason,
+    SrcMgr::CharacteristicKind FileType, FileID PrevFID = FileID()) {
+  if (!EnteredMainFile) {
+    EnteredMainFile = true;
+    Preprocessor->EnterMainSourceFile();
+  }
+}
+
+void ExpandModularHeadersPPCallbacks::InclusionDirective(
+    SourceLocation DirectiveLoc, const Token &IncludeToken,
+    StringRef IncludedFilename, bool IsAngled, CharSourceRange FilenameRange,
+    const FileEntry *IncludedFile, StringRef SearchPath, StringRef RelativePath,
+    const Module *Imported, SrcMgr::CharacteristicKind FileType) {
+  if (Imported) {
+    serialization::ModuleFile *MF =
+        Compiler.getModuleManager()->getModuleManager().lookup(
+            Imported->getASTFile());
+    handleModuleFile(MF);
+  }
+  parseToLocation(DirectiveLoc);
+}
+
+void ExpandModularHeadersPPCallbacks::EndOfMainFile() {
+  while (!CurrentToken.is(tok::eof))
+    Preprocessor->Lex(CurrentToken);
+}
+
+// Handle all other callbacks.
+// Just parse to the corresponding location to generate the same callback for
+// the PPCallbacks registered in our custom preprocessor.
+void ExpandModularHeadersPPCallbacks::Ident(SourceLocation Loc, StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDirective(SourceLocation Loc,
+                                                      PragmaIntroducerKind) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaComment(SourceLocation Loc,
+                                                    const IdentifierInfo *,
+                                                    StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDetectMismatch(SourceLocation Loc,
+                                                           StringRef,
+                                                           StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDebug(SourceLocation Loc,
+                                                  StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaMessage(SourceLocation Loc,
+                                                    StringRef,
+                                                    PragmaMessageKind,
+                                                    StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDiagnosticPush(SourceLocation Loc,
+                                                           StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDiagnosticPop(SourceLocation Loc,
+                                                          StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaDiagnostic(SourceLocation Loc,
+                                                       StringRef,
+                                                       diag::Severity,
+                                                       StringRef) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::HasInclude(SourceLocation Loc, StringRef,
+                                                 bool, const FileEntry *,
+                                                 SrcMgr::CharacteristicKind) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaOpenCLExtension(
+    SourceLocation NameLoc, const IdentifierInfo *, SourceLocation StateLoc,
+    unsigned) {
+  // FIME: Figure out whether it's the right location to parse to.
+  parseToLocation(NameLoc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaWarning(SourceLocation Loc,
+                                                    StringRef, ArrayRef<int>) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaWarningPush(SourceLocation Loc,
+                                                        int) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaWarningPop(SourceLocation Loc) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaAssumeNonNullBegin(
+    SourceLocation Loc) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::PragmaAssumeNonNullEnd(
+    SourceLocation Loc) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::MacroExpands(const Token &MacroNameTok,
+                                                   const MacroDefinition &,
+                                                   SourceRange Range,
+                                                   const MacroArgs *) {
+  // FIME: Figure out whether it's the right location to parse to.
+  parseToLocation(Range.getBegin());
+}
+void ExpandModularHeadersPPCallbacks::MacroDefined(const Token &MacroNameTok,
+                                                   const MacroDirective *MD) {
+  parseToLocation(MD->getLocation());
+}
+void ExpandModularHeadersPPCallbacks::MacroUndefined(
+    const Token &, const MacroDefinition &, const MacroDirective *Undef) {
+  if (Undef)
+    parseToLocation(Undef->getLocation());
+}
+void ExpandModularHeadersPPCallbacks::Defined(const Token &MacroNameTok,
+                                              const MacroDefinition &,
+                                              SourceRange Range) {
+  // FIME: Figure out whether it's the right location to parse to.
+  parseToLocation(Range.getBegin());
+}
+void ExpandModularHeadersPPCallbacks::SourceRangeSkipped(
+    SourceRange Range, SourceLocation EndifLoc) {
+  // FIME: Figure out whether it's the right location to parse to.
+  parseToLocation(EndifLoc);
+}
+void ExpandModularHeadersPPCallbacks::If(SourceLocation Loc, SourceRange,
+                                         ConditionValueKind) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::Elif(SourceLocation Loc, SourceRange,
+                                           ConditionValueKind, SourceLocation) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::Ifdef(SourceLocation Loc, const Token &,
+                                            const MacroDefinition &) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::Ifndef(SourceLocation Loc, const Token &,
+                                             const MacroDefinition &) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::Else(SourceLocation Loc, SourceLocation) {
+  parseToLocation(Loc);
+}
+void ExpandModularHeadersPPCallbacks::Endif(SourceLocation Loc,
+                                            SourceLocation) {
+  parseToLocation(Loc);
+}
+
+} // namespace tooling
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.h

 /// configured.
 class IdentifierNamingCheck : public ClangTidyCheck {
 public:
   IdentifierNamingCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
   void onEndOfTranslationUnit() override;
 
   enum CaseType {
     CT_AnyCase = 0,
     CT_LowerCase,
     CT_CamelBack,
     CT_UpperCase,
     CT_CamelCase,

clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp

void IdentifierNamingCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(usingDecl().bind("using"), this);
   Finder->addMatcher(declRefExpr().bind("declRef"), this);
   Finder->addMatcher(cxxConstructorDecl().bind("classRef"), this);
   Finder->addMatcher(cxxDestructorDecl().bind("classRef"), this);
   Finder->addMatcher(typeLoc().bind("typeLoc"), this);
   Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
 }
 
-void IdentifierNamingCheck::registerPPCallbacks(CompilerInstance &Compiler) {
-  Compiler.getPreprocessor().addPPCallbacks(
-      llvm::make_unique<IdentifierNamingCheckPPCallbacks>(
-          &Compiler.getPreprocessor(), this));
+void IdentifierNamingCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  ModuleExpanderPP->addPPCallbacks(
+      llvm::make_unique<IdentifierNamingCheckPPCallbacks>(ModuleExpanderPP,
+                                                          this));
 }
 
 static bool matchesStyle(StringRef Name,
                          IdentifierNamingCheck::NamingStyle Style) {
   static llvm::Regex Matchers[] = {
       llvm::Regex("^.*$"),
       llvm::Regex("^[a-z][a-z0-9_]*$"),
       llvm::Regex("^[a-z][a-zA-Z0-9]*$"),

clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp

if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
       return nullptr;
     }
   }
   return llvm::make_unique<FileOptionsProvider>(GlobalOptions, DefaultOptions,
                                                 OverrideOptions, std::move(FS));
 }
 
 llvm::IntrusiveRefCntPtr<vfs::FileSystem>
-getVfsOverlayFromFile(const std::string &OverlayFile) {
-  llvm::IntrusiveRefCntPtr<vfs::OverlayFileSystem> OverlayFS(
-      new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+getVfsFromFile(const std::string &OverlayFile,
+               llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS) {
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
-      OverlayFS->getBufferForFile(OverlayFile);
+      BaseFS->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) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
                                     cl::ZeroOrMore);
-  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS(
-      VfsOverlay.empty() ? vfs::getRealFileSystem()
-                         : getVfsOverlayFromFile(VfsOverlay));
-  if (!BaseFS)
+  llvm::IntrusiveRefCntPtr<vfs::OverlayFileSystem> BaseFS(
+      new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+
+  if (!VfsOverlay.empty()) {
+    IntrusiveRefCntPtr<vfs::FileSystem> VfsFromFile =
+        getVfsFromFile(VfsOverlay, BaseFS);
+    if (!VfsFromFile)
       return 1;
+    BaseFS->pushOverlay(VfsFromFile);
+  }
 
   auto OwningOptionsProvider = createOptionsProvider(BaseFS);
   auto *OptionsProvider = OwningOptionsProvider.get();
   if (!OptionsProvider)
     return 1;
 
   auto MakeAbsolute = [](const std::string &Input) -> SmallString<256> {
     if (Input.empty())

clang-tools-extra/trunk/test/CMakeLists.txt

set(CLANG_TOOLS_TEST_DEPS
   ExtraToolsUnitTests
 
   # For the clang-tidy libclang integration test.
   c-index-test
   # clang-tidy tests require it.
   clang-resource-headers
 
   clang-tidy
+  # Clang-tidy tests need clang for building modules.
+  clang
 )
 
 if(CLANGD_BUILD_XPC_SUPPORT)
   list(APPEND CLANG_TOOLS_TEST_DEPS clangd-xpc-test-client)
 endif()
 
 set(CLANGD_TEST_DEPS
   clangd

clang-tools-extra/trunk/test/clang-tidy/Inputs/expand-modular-headers-ppcallbacks/a.h

+#define a

clang-tools-extra/trunk/test/clang-tidy/Inputs/expand-modular-headers-ppcallbacks/b.h

+#include "a.h"
+#define b

clang-tools-extra/trunk/test/clang-tidy/Inputs/expand-modular-headers-ppcallbacks/c.h

+#include "b.h"
+#define c

clang-tools-extra/trunk/test/clang-tidy/Inputs/expand-modular-headers-ppcallbacks/module.modulemap

+module a { header "a.h" export * }
+module b { header "b.h" export * use a }
+module c { header "c.h" export * use b }

clang-tools-extra/trunk/test/clang-tidy/expand-modular-headers-ppcallbacks.cpp

+// Sanity-check. Run without modules:
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cp %S/Inputs/expand-modular-headers-ppcallbacks/* %t/
+// RUN: %check_clang_tidy %s readability-identifier-naming %t/without-modules -- \
+// RUN:   -config="CheckOptions: [{ \
+// RUN:      key: readability-identifier-naming.MacroDefinitionCase, value: UPPER_CASE }]" \
+// RUN:   -header-filter=.* \
+// RUN:   -- -x c++ -std=c++11 -I%t/
+//
+// Run clang-tidy on a file with modular includes:
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cp %S/Inputs/expand-modular-headers-ppcallbacks/* %t/
+// RUN: %check_clang_tidy %s readability-identifier-naming %t/with-modules -- \
+// RUN:   -config="CheckOptions: [{ \
+// RUN:      key: readability-identifier-naming.MacroDefinitionCase, value: UPPER_CASE }]" \
+// RUN:   -header-filter=.* \
+// RUN:   -- -x c++ -std=c++11 -I%t/ \
+// RUN:   -fmodules -fimplicit-modules -fno-implicit-module-maps \
+// RUN:   -fmodule-map-file=%t/module.modulemap \
+// RUN:   -fmodules-cache-path=%t/module-cache/
+#include "c.h"
+
+// CHECK-MESSAGES: a.h:1:9: warning: invalid case style for macro definition 'a' [readability-identifier-naming]
+// CHECK-MESSAGES: a.h:1:9: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: b.h:2:9: warning: invalid case style for macro definition 'b'
+// CHECK-MESSAGES: b.h:2:9: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: c.h:2:9: warning: invalid case style for macro definition 'c'
+// CHECK-MESSAGES: c.h:2:9: note: FIX-IT applied suggested code changes
+
+#define m
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for macro definition 'm'
+// CHECK-MESSAGES: :[[@LINE-2]]:9: note: FIX-IT applied suggested code changes