[clang][deps] Make the C++ API more type-safe

[jansvoboda11]

Summary

Scanner's C++ API accepts a set of modular dependencies the client has already seen and for which it doesn't need the full details. This is currently a set of strings, which somewhat implies that it should contain the set of module names. However, scanner internally expects the values to be in the format "{hash}{name}". Besides not being documented, this is very unintuitive. This patch makes this expectation explicit by changing the type to set of ModuleID.

[benlangmuir]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h

174–175

You can drop the third template argument; it now matches the default.

clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h

 //===- DependencyScanningTool.h - clang-scan-deps service -----------------===//
 //
 // 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_DEPENDENCYSCANNING_DEPENDENCYSCANNINGTOOL_H
 #define LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGTOOL_H
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/MapVector.h"
-#include "llvm/ADT/StringSet.h"
-#include "llvm/ADT/StringMap.h"
 #include <optional>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace tooling {
 namespace dependencies {
 
public:
   ///                           "-fmodule-file=", "-o" and other output
   ///                           arguments for dependencies.
   ///
   /// \returns a \c StringError with the diagnostic output if clang errors
   /// occurred, \c TranslationUnitDeps otherwise.
   llvm::Expected<TranslationUnitDeps>
   getTranslationUnitDependencies(const std::vector<std::string> &CommandLine,
                                  StringRef CWD,
-                                 const llvm::StringSet<> &AlreadySeen,
+                                 const llvm::DenseSet<ModuleID> &AlreadySeen,
                                  LookupModuleOutputCallback LookupModuleOutput);
 
   /// Given a compilation context specified via the Clang driver command-line,
   /// gather modular dependencies of module with the given name, and return the
   /// information needed for explicit build.
-  llvm::Expected<ModuleDepsGraph>
-  getModuleDependencies(StringRef ModuleName,
-                        const std::vector<std::string> &CommandLine,
-                        StringRef CWD, const llvm::StringSet<> &AlreadySeen,
-                        LookupModuleOutputCallback LookupModuleOutput);
+  llvm::Expected<ModuleDepsGraph> getModuleDependencies(
+      StringRef ModuleName, const std::vector<std::string> &CommandLine,
+      StringRef CWD, const llvm::DenseSet<ModuleID> &AlreadySeen,
 
 private:
   DependencyScanningWorker Worker;
 };
 
 class FullDependencyConsumer : public DependencyConsumer {
 public:
-  FullDependencyConsumer(const llvm::StringSet<> &AlreadySeen)
+  FullDependencyConsumer(const llvm::DenseSet<ModuleID> &AlreadySeen)
       : AlreadySeen(AlreadySeen) {}
 
   void handleBuildCommand(Command Cmd) override {
     Commands.push_back(std::move(Cmd));
   }
 
   void handleDependencyOutputOpts(const DependencyOutputOptions &) override {}
 
   void handleFileDependency(StringRef File) override {
     Dependencies.push_back(std::string(File));
   }
 
   void handlePrebuiltModuleDependency(PrebuiltModuleDep PMD) override {
     PrebuiltModuleDeps.emplace_back(std::move(PMD));
   }
 
   void handleModuleDependency(ModuleDeps MD) override {
-    ClangModuleDeps[MD.ID.ContextHash + MD.ID.ModuleName] = std::move(MD);
+    ClangModuleDeps[MD.ID] = std::move(MD);
   }
 
   void handleContextHash(std::string Hash) override {
     ContextHash = std::move(Hash);
   }
 
   TranslationUnitDeps takeTranslationUnitDeps();
   ModuleDepsGraph takeModuleGraphDeps();
 
 private:
   std::vector<std::string> Dependencies;
   std::vector<PrebuiltModuleDep> PrebuiltModuleDeps;
-  llvm::MapVector<std::string, ModuleDeps, llvm::StringMap<unsigned>>
-      ClangModuleDeps;
+  llvm::MapVector<ModuleID, ModuleDeps> ClangModuleDeps;
   std::vector<Command> Commands;
   std::string ContextHash;
   std::vector<std::string> OutputPaths;
-  const llvm::StringSet<> &AlreadySeen;
+  const llvm::DenseSet<ModuleID> &AlreadySeen;
 };
 
 /// A simple dependency action controller that uses a callback. If no callback
 /// is provided, it is assumed that looking up module outputs is unreachable.
 class CallbackActionController : public DependencyActionController {
 public:
   virtual ~CallbackActionController();
 

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 #include <string>
 #include <unordered_map>
 
 namespace clang {
 namespace tooling {
private:
   void associateWithContextHash(const CompilerInvocation &CI, ModuleDeps &Deps);
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 namespace llvm {
+inline hash_code hash_value(const clang::tooling::dependencies::ModuleID &ID) {
+  return hash_combine(ID.ModuleName, ID.ContextHash);
+}
+
 template <> struct DenseMapInfo<clang::tooling::dependencies::ModuleID> {
   using ModuleID = clang::tooling::dependencies::ModuleID;
   static inline ModuleID getEmptyKey() { return ModuleID{"", ""}; }
   static inline ModuleID getTombstoneKey() {
     return ModuleID{"~", "~"}; // ~ is not a valid module name or context hash
   }
-  static unsigned getHashValue(const ModuleID &ID) {
-    return hash_combine(ID.ModuleName, ID.ContextHash);
-  }
+  static unsigned getHashValue(const ModuleID &ID) { return hash_value(ID); }
   static bool isEqual(const ModuleID &LHS, const ModuleID &RHS) {
     return LHS == RHS;
   }
 };
 } // namespace llvm
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_MODULEDEPCOLLECTOR_H

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile(
   if (!MakeformatOutputPath.empty())
     Consumer.printDependencies(MakeformatOutput);
   return Rule;
 }
 
 llvm::Expected<TranslationUnitDeps>
 DependencyScanningTool::getTranslationUnitDependencies(
     const std::vector<std::string> &CommandLine, StringRef CWD,
-    const llvm::StringSet<> &AlreadySeen,
+    const llvm::DenseSet<ModuleID> &AlreadySeen,
     LookupModuleOutputCallback LookupModuleOutput) {
   FullDependencyConsumer Consumer(AlreadySeen);
   CallbackActionController Controller(LookupModuleOutput);
   llvm::Error Result =
       Worker.computeDependencies(CWD, CommandLine, Consumer, Controller);
   if (Result)
     return std::move(Result);
   return Consumer.takeTranslationUnitDeps();
 }
 
 llvm::Expected<ModuleDepsGraph> DependencyScanningTool::getModuleDependencies(
     StringRef ModuleName, const std::vector<std::string> &CommandLine,
-    StringRef CWD, const llvm::StringSet<> &AlreadySeen,
+    StringRef CWD, const llvm::DenseSet<ModuleID> &AlreadySeen,
     LookupModuleOutputCallback LookupModuleOutput) {
   FullDependencyConsumer Consumer(AlreadySeen);
   CallbackActionController Controller(LookupModuleOutput);
   llvm::Error Result = Worker.computeDependencies(CWD, CommandLine, Consumer,
                                                   Controller, ModuleName);
   if (Result)
     return std::move(Result);
   return Consumer.takeModuleGraphDeps();

clang/tools/clang-scan-deps/ClangScanDeps.cpp

struct IndexedModuleID {
     ModuleID ID;
 
     // FIXME: This is mutable so that it can still be updated after insertion
     //  into an unordered associative container. This is "fine", since this
     //  field doesn't contribute to the hash, but it's a brittle hack.
     mutable size_t InputIndex;
 
     bool operator==(const IndexedModuleID &Other) const {
-      return std::tie(ID.ModuleName, ID.ContextHash) ==
-             std::tie(Other.ID.ModuleName, Other.ID.ContextHash);
+      return ID == Other.ID;
     }
 
     bool operator<(const IndexedModuleID &Other) const {
       /// We need the output of clang-scan-deps to be deterministic. However,
       /// the dependency graph may contain two modules with the same name. How
       /// do we decide which one to print first? If we made that decision based
       /// on the context hash, the ordering would be deterministic, but
       /// different across machines. This can happen for example when the inputs
       /// or the SDKs (which both contribute to the "context" hash) live in
       /// different absolute locations. We solve that by tracking the index of
       /// the first input TU that (transitively) imports the dependency, which
       /// is always the same for the same input, resulting in deterministic
       /// sorting that's also reproducible across machines.
       return std::tie(ID.ModuleName, InputIndex) <
              std::tie(Other.ID.ModuleName, Other.InputIndex);
     }
 
     struct Hasher {
       std::size_t operator()(const IndexedModuleID &IMID) const {
-        return llvm::hash_combine(IMID.ID.ModuleName, IMID.ID.ContextHash);
+        return llvm::hash_value(IMID.ID);
       }
     };
   };
 
   struct InputDeps {
     std::string FileName;
     std::string ContextHash;
     std::vector<std::string> FileDeps;
llvm::outs() << "Running clang-scan-deps on " << Inputs.size()
                  << " files using " << Pool.getThreadCount() << " workers\n";
   }
 
   llvm::Timer T;
   T.startTimer();
 
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I) {
     Pool.async([&, I]() {
-      llvm::StringSet<> AlreadySeenModules;
+      llvm::DenseSet<ModuleID> AlreadySeenModules;
       while (auto MaybeInputIndex = GetNextInputIndex()) {
         size_t LocalIndex = *MaybeInputIndex;
         const tooling::CompileCommand *Input = &Inputs[LocalIndex];
         std::string Filename = std::move(Input->Filename);
         std::string CWD = std::move(Input->Directory);
 
         std::optional<StringRef> MaybeModuleName;
         if (!ModuleName.empty())