[clang-tidy] Add project-level analysis support to misc-discarded-return-value

[bahramib]

Summary

This enables calculating the consumed/discarded distribution of function
calls for the entire project, resulting in more meaningful diagnostics.

[martong]

clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp

132

Please spell out the type instead of auto.

132–142

This could be split out nicely in a member function, e.g. findFunctionID.

204

How is it emitting dianoses then in the singleTU mode?

365

?

clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.h

70

Some docs could be useful here.

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

 
 clang_target_link_libraries(clangTidyMiscModule
   PRIVATE
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
+  clangIndex
   clangSerialization
   clangTooling
   )

clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.h

 //===--- DiscardedReturnValueCheck.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_TOOLS_EXTRA_CLANG_TIDY_MISC_DISCARDEDRETURNVALUECHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DISCARDEDRETURNVALUECHECK_H
 
 #include "../ClangTidyCheck.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/StringMap.h"
 #include <cinttypes>
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 /// Flags function calls which return value is discarded if most of the
 /// other calls of the function consume the return value.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/misc-discarded-return-value.html
 class DiscardedReturnValueCheck : public ClangTidyCheck {
 public:
   struct Function {
     std::size_t ConsumedCalls;
     std::size_t TotalCalls;
+    const FunctionDecl *FD;
     llvm::SmallPtrSet<const CallExpr *, 32> DiscardedCEs;
 
     /// Returns ConsumedCalls / TotalCalls expressed as a whole percentage.
     std::uint8_t ratio() const;
   };
-  using FunctionMapTy = llvm::DenseMap<const FunctionDecl *, Function>;
+  using FunctionMapTy = llvm::StringMap<Function>;
 
   DiscardedReturnValueCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void onStartOfTranslationUnit() override;
   void onEndOfTranslationUnit() override;
+  void collect(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void postCollect(StringRef OutputFile) override;
+  void compact(const std::vector<std::string> &PerTuCollectedData,
+               StringRef OutputFile) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   /// The threshold (expressed as a whole percentage) the calls to a function
   /// must be consumed for discarded calls to be warned about. A threshold of
   /// 80 means that if 8 calls out of 10 calls (80%) to a function is consumed,
   /// the remaining 2 will be warned.
   const std::uint8_t ConsumeThreshold;
 
+  /// Contains whether during diagnostics the data was loaded from a serialized
+  /// project-level file created by compact().
+  /// (This is only used as a result cache so no several rounds of lookup is
+  /// made.)
+  Optional<bool> CacheProjectDataLoadedSuccessfully;
+
   /// Stores AST nodes which we have observed to be consuming calls.
   /// (This is a helper data structure to prevent matchers matching consuming
   /// contexts firing multiple times and messing up the statistics created.)
   llvm::DenseMap<const CallExpr *, SmallVector<const void *, 2>> ConsumedCalls;
 
   FunctionMapTy CallMap;
+  llvm::DenseMap<const FunctionDecl *, std::string> FunctionIDs;
 
+  void matchResult(const ast_matchers::MatchFinder::MatchResult &Result,
+                   bool ShouldCount);
   void registerCall(const CallExpr *CE, const FunctionDecl *FD,
-                    const void *ConsumingContext);
-  void diagnose(const FunctionDecl *FD, const Function &F);
+                    bool IncrementCounters, const void *ConsumingContext);
+  void diagnose(const Function &F);
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DISCARDEDRETURNVALUECHECK_H

clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp

 //===--- DiscardedReturnValueCheck.cpp - clang-tidy -----------------------===//
 //
 // 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 "DiscardedReturnValueCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Index/USRGeneration.h"
+#include "llvm/Support/YAMLParser.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 using namespace clang::tidy::misc;
 
+namespace {
+
+/// The format of the YAML document the checker uses in multi-pass project-level
+/// mode. This is the same for the per-TU and the per-project data.
+struct SerializedFunction {
+  std::string ID;
+  std::size_t ConsumedCalls, TotalCalls;
+};
+using FunctionVec = std::vector<SerializedFunction>;
+
+} // namespace
+
+namespace llvm {
+namespace yaml {
+
+template <> struct MappingTraits<SerializedFunction> {
+  static void mapping(IO &IO, SerializedFunction &F) {
+    IO.mapRequired("ID", F.ID);
+    IO.mapRequired("Consumed", F.ConsumedCalls);
+    IO.mapRequired("Total", F.TotalCalls);
+  }
+};
+
+} // namespace yaml
+} // namespace llvm
+
+LLVM_YAML_IS_SEQUENCE_VECTOR(SerializedFunction)
+
+static Optional<FunctionVec> loadYAML(StringRef File) {
+  using namespace llvm;
+
+  ErrorOr<std::unique_ptr<MemoryBuffer>> IStream =
+      MemoryBuffer::getFileAsStream(File);
+  if (!IStream)
+    return None;
+
+  FunctionVec R;
+  yaml::Input YIn{**IStream};
+  YIn >> R;
+
+  return R;
+}
+
+static bool loadYAML(StringRef FromFile,
+                     DiscardedReturnValueCheck::FunctionMapTy &ToMap) {
+  Optional<FunctionVec> OV = loadYAML(FromFile);
+  if (!OV)
+    return false;
+
+  for (const SerializedFunction &SF : *OV) {
+    DiscardedReturnValueCheck::Function &F =
+        ToMap
+            .try_emplace(SF.ID,
+                         DiscardedReturnValueCheck::Function{0, 0, nullptr, {}})
+            .first->second;
+
+    F.ConsumedCalls += SF.ConsumedCalls;
+    F.TotalCalls += SF.TotalCalls;
+  }
+
+  return true;
+}
+
+static FunctionVec
+yamlize(const DiscardedReturnValueCheck::FunctionMapTy &Map) {
+  FunctionVec SFs;
+  llvm::transform(Map, std::back_inserter(SFs), [](auto &&E) {
+    return SerializedFunction{E.first().str(), E.second.ConsumedCalls,
+                              E.second.TotalCalls};
+  });
+  return SFs;
+}
+
+static void writeYAML(StringRef Whence, FunctionVec Elements,
+                      StringRef ToFile) {
+  std::error_code EC;
+  llvm::raw_fd_ostream FS(ToFile, EC, llvm::sys::fs::OF_Text);
+  if (EC) {
+    llvm::errs() << "DiscardedReturnValueCheck: Failed to write " << Whence
+                 << " output file: " << EC.message();
+    llvm::report_fatal_error("", false);
+    return;
+  }
+
+  llvm::yaml::Output YAMLOut(FS);
+  YAMLOut << Elements;
+}
+
 static constexpr llvm::StringLiteral Call = "call";
 static constexpr llvm::StringLiteral Consume = "consume";
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 std::uint8_t DiscardedReturnValueCheck::Function::ratio() const {
   if (TotalCalls == 0)
     return 0;
 
   std::uint8_t R = static_cast<float>(ConsumedCalls * 100) / TotalCalls;
   assert(R <= 100 && "Invalid percentage, maybe bogus compacted data?");
   return R;
 }
 
 void DiscardedReturnValueCheck::registerCall(const CallExpr *CE,
                                              const FunctionDecl *FD,
+                                             bool IncrementCounters,
                                              const void *ConsumingContext) {
   assert(CE && FD);
   FD = FD->getCanonicalDecl();
-  Function &Data = CallMap.try_emplace(FD, Function{0, 0, {}}).first->second;
+
+  auto IDIt = FunctionIDs.find(FD);
+  if (IDIt == FunctionIDs.end()) {
+    SmallString<128> ID;
+    bool USRFailure = clang::index::generateUSRForDecl(FD, ID);
+    if (USRFailure)
+      ID = FD->getDeclName().getAsString();
+    assert(!ID.empty() && "Generating name for function failed");
+
+    IDIt = FunctionIDs.try_emplace(FD, ID.str().str()).first;
+  }
+  assert(IDIt != FunctionIDs.end());
+
+  Function &Data =
+      CallMap.try_emplace(IDIt->second, Function{0, 0, FD, {}}).first->second;
+  if (!Data.FD)
+    // If the map already contains data which was loaded from an earlier project
+    // pass, store FD (in the current TU) so diagose() can point to it properly.
+    Data.FD = FD;
 
   if (ConsumingContext) {
     using ConsumeVecTy = decltype(ConsumedCalls)::mapped_type;
     ConsumeVecTy &ConsumeVec =
         ConsumedCalls.try_emplace(CE, ConsumeVecTy{}).first->second;
     if (llvm::find(ConsumeVec, ConsumingContext) != ConsumeVec.end())
       return;
     ConsumeVec.emplace_back(ConsumingContext);
   } else
     Data.DiscardedCEs.insert(CE);
 
+  if (IncrementCounters) {
     if (ConsumingContext)
       ++Data.ConsumedCalls;
     ++Data.TotalCalls;
   }
+}
 
-void DiscardedReturnValueCheck::diagnose(const FunctionDecl *FD,
-                                         const Function &F) {
+void DiscardedReturnValueCheck::diagnose(const Function &F) {
   if (F.ratio() < ConsumeThreshold)
     return;
 
   for (const CallExpr *Call : F.DiscardedCEs) {
     diag(Call->getExprLoc(),
          "return value of %0 is used in most calls, but not in this one",
          DiagnosticIDs::Warning)
-        << *FD << F.ratio() << F.ConsumedCalls << F.TotalCalls;
+        << F.FD << F.ratio() << F.ConsumedCalls << F.TotalCalls;
 
     diag(Call->getExprLoc(),
          "value consumed or checked in %0%% (%1 out of %2) of cases",
          DiagnosticIDs::Note)
         << F.ratio() << F.ConsumedCalls << F.TotalCalls;
   }
 }
 
 DiscardedReturnValueCheck::DiscardedReturnValueCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ConsumeThreshold(Options.get("ConsumeThreshold", 80)) {}
 
 void DiscardedReturnValueCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ConsumeThreshold", ConsumeThreshold);
 }
 
 void DiscardedReturnValueCheck::onStartOfTranslationUnit() {
+  CacheProjectDataLoadedSuccessfully.reset();
   ConsumedCalls.clear();
   CallMap.clear();
+  FunctionIDs.clear();
 }
 
 void DiscardedReturnValueCheck::onEndOfTranslationUnit() {
   // Once everything had been counted, emit the results.
+  if (getPhase() == MultipassProjectPhase::Diagnose)
     for (const auto &Call : CallMap)
-    diagnose(Call.first, Call.second);
+      diagnose(Call.second);
 }
 
 namespace {
 
 AST_MATCHER_P(StaticAssertDecl, hasAssertExpr,
               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
   return InnerMatcher.matches(*Node.getAssertExpr(), Finder, Builder);
 }
Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
                               type(eachOf(Decltype, TemplateArg, VLA)))
                          .bind(Consume),
                      this);
 
   // Matches discarded function calls.
   Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource, Call), this);
 }
 
-void DiscardedReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
+void DiscardedReturnValueCheck::matchResult(
+    const MatchFinder::MatchResult &Result, bool ShouldCount) {
   const auto *CE = Result.Nodes.getNodeAs<CallExpr>(Call);
   assert(CE && "Bad matcher");
 
   // There are two matchers registered, one that matches the larger tree that
   // contains the consume of the call's result, and the other that matches every
   // call.
   const void *ConsumeNode = nullptr;
   if (const auto *D = Result.Nodes.getNodeAs<Decl>(Consume))
     ConsumeNode = D;
   if (const auto *S = Result.Nodes.getNodeAs<Stmt>(Consume))
     ConsumeNode = S;
   if (const auto *T = Result.Nodes.getNodeAs<Type>(Consume))
     ConsumeNode = T;
   if (ConsumeNode)
-    return registerCall(CE, CE->getDirectCallee(), ConsumeNode);
+    return registerCall(CE, CE->getDirectCallee(), ShouldCount, ConsumeNode);
 
   // If ConsumeNode is left to be nullptr, the current match is not through the
   // "consuming" matcher. It might be the only match of this function (and then
   // it is discarded), or it might have been matched earlier and consumed.
   if (ConsumedCalls.find(CE) == ConsumedCalls.end())
-    return registerCall(CE, CE->getDirectCallee(), nullptr);
+    return registerCall(CE, CE->getDirectCallee(), ShouldCount, nullptr);
+}
+
+void DiscardedReturnValueCheck::collect(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  matchResult(Result, true);
+}
+
+void DiscardedReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
+  // check() might be called without previous data existing if Tidy is executed
+  // in single-phase mode.
+  if (!CacheProjectDataLoadedSuccessfully.hasValue()) {
+    StringRef ProjectLevelData = getCompactedDataPath();
+    bool Success = false;
+    if (!ProjectLevelData.empty() && CallMap.empty())
+      Success = loadYAML(ProjectLevelData, CallMap);
+
+    CacheProjectDataLoadedSuccessfully.emplace(Success);
+  }
+  // If there was previously loaded data, the current match callback should
+  // not invalidate the counters we just loaded.
+  bool ShouldCountMatches = !CacheProjectDataLoadedSuccessfully.getValue();
+
+  matchResult(Result, ShouldCountMatches);
+}
+
+void DiscardedReturnValueCheck::postCollect(StringRef OutputFilename) {
+  writeYAML("postCollect()", yamlize(CallMap), OutputFilename);
+}
+
+void DiscardedReturnValueCheck::compact(
+    const std::vector<std::string> &PerTUCollectedData, StringRef OutputFile) {
+  for (const std::string &PerTUFilename : PerTUCollectedData)
+    if (!loadYAML(PerTUFilename, CallMap))
+      llvm::errs()
+          << "DiscardedReturnValueCheck: Failed to load compact() input file "
+          << PerTUFilename << ".\n";
+
+  writeYAML("compact()", yamlize(CallMap), OutputFile);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang