[analyzer] Implement conversion from Clang diagnostics to PathDiagnostics.

[NoQ]

Summary

This clang::DiagnosticConsumer can take normal clang diagnostics (eg., Sema errors or Clang-Tidy warnings) and turn them into static analyzer's PathDiagnostics which are then fed into PathDiagnosticConsumers. It's an essential component for my clang-tidy/static analyzer cross-integration efforts.

The consumer isn't magically aware of information that isn't present in Clang diagnostics but expected in path diagnostics. Such info includes checker name / bug type / category; the consumer expects an external provider to provide such information for every incoming diagnostic.

Additionally, PathDiagnostic's declaration-with-issue and uniqueing-declaration are currently set to nullptr. I'm slowly thinking about auto-detecting those based on the source location but technically all path diagnostic consumer facilities can function just fine without them (declaration-with-issue is there purely for displaying user-visible names and uniqueing declaration only makes sense for path-sensitive warnings that require exotic uniqueing which will probably never be fed into this consumer).

There are also a few features missing here and there, such as fix-it hint support, which i plan to add later.

Though technically possible, this consumer is never supposed to consume the output of TextPathDiagnosticConsumer; a lot of information is lost that way that can't be recovered. In particular, the proper way to teach clang-tidy to emit path diagnostics while keeping in mind that it already ships with static analyzer integration is to allow the integrated static analyzer to talk to path diagnostic consumers directly; static analyzer warnings should never go through this new consumer.

[NoQ]

Add a few comments here and there.

[vsavchenko]

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h

24

I think that this class (or file?) needs to have some comments on how it is supposed to be used.

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

39

I think this type of stuff should be covered by a comment or a descriptive function name.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

2

Looks like a copy-paste error :)

64

nit

64

nit: llvm::enumerate or llvm::zip?

84

TBH it is pretty hard to follow the test and from it's structure see what is the input and what is expected output. Maybe it can be reorganized a bit?

140

WE NEED MORE NOUNS!

150–151

Also doesn't seem relevant

155

It is also about the structure, I guess it would've been nice to have all the inputs and expected outputs to be specified here in the actual test, so the reader can figure out what is tested without diving deep into the classes. And it also seems that with the current structure you'll need a couple more classes for every new test.

[steakhal]

Seems pretty straightforward and clean.

The cleanup of the report's message should be reworked.
Besides, that looks good to me.

I think these cases should be tested as well:

• [Warning, Warning, Warning]
• [Warning, Note, Note]
• [Warning, Note, Note, Warning, Note]
• [Warning, Note, Remark, Warning, Remark]

PS: clang-format; sorry for the nit-spam :D

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h

54–57

Overriding the virtual void clang::DiagnosticConsumer::finish() can't accomplish the same thing?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

2

I've seen this a few times, and I still don't know why we use it.
The file extension should be enough to recognize that this is a C++ file.
Can someone clarify this?

15

I frequently see #include "clang/..."-ish includes but never #include <clang/...>.

24

38

39

Eh, I don't think it's gonna work this way.
We can not assume that the [ won't appear in the payload of the message.
Eg.:
NewDelete-checker-test.cpp:193

// newdelete-warning{{Argument to 'delete[]' is the address of the local variable 'i', which is not memory allocated by 'new[]'}}

The best you could do is to do a reverse search.
Do we emit the [mypackage.mychecker] suffix for all the reports? If not, then we have a problem.

46–53

So, Error and Fatalis treated in the same way as Warning.
What about the Ignored?

55–61

The ternary operator could simplify this inside the push_back function - emplace_back?

72

Should Desc and ShortDesc be the same?

75–78

Why not construct in place?

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

45

I would suggest removing the redundant virtual. The same applies to the other decls.

64

clang-format should format these files as well, am I right?

84

I can confirm.
What about passing the ExpectedDiags as a parameter?
Probably some shorter type alias could come handy for TestPathDiagnosticConsumer::ExpectedDiagTy and TestPathDiagnosticConsumer::ExpectedPieceTy.

140

What about calling this DiagnosticMock?

[xazax.hun]

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h

39

A pointer pair might be small enough for a DenseMap?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

21

Do we need this early return? We might get the same behavior by simply omitting this check. I have no strong preference about keeping or removing it.

[NoQ]

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

2

https://llvm.org/docs/CodingStandards.html#file-headers

(with our file name it doesn't look like there's much space even for a short description)
(i should probably convert the long description into a doxygen comment as well)

15

Whoops thanks.

21

We need it. @steakhal figured this out correctly in the next comment.

39

Uh-oh, mmm, indeed. I should definitely make this optional as well.

55–61

Can't easily use ?: here because LHS and RHS are of different types and operator ?: doesn't do implicit casts.

72

They don't need to be the same but typically clang diagnostics don't provide two different wordings for the same warning.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

45

Yup, should've said override instead.

155

We have to follow the libTooling architecture where we have to have a FrontendAction class and an ASTConsumer class with specific callback signatures. I'll try to think of something.

[NoQ]

Address review comments!

[NoQ]

[Warning, Note, Remark, Warning, Remark]

I excluded remarks for now with an assertion. Other tests added :)

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h

54–57

*mind blown*

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

39

Do we emit the [mypackage.mychecker] suffix for all the reports? If not, then we have a problem.

This was supposed to be a workaround for clang-tidy doing this to us. I think it's actually better to teach clang-tidy not to do this when it's pumping output to us. PathDiagnosticConsumers will re-add the checker name when appropriate (eg., in text output but not in plist output).

75–78

With these classes typically there's a bunch of stuff in between, such as Piece->addRange(). Wait a sec, I can actually handle ranges pretty quickly. Let me add them so that this place wasn't empty :D

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

140

We still need to discriminate between FrontendAction mock, ASTConsumer mock, and PathDiagnosticConsumer mock. Dropped some of the nouns though.

[Szelethus]

LGTM! As a side note, are we aware of anyone that uses short messages instead of the full length one?

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

70–71

For later, consider testing and handling -werror and -analyzer-werror.

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

140

Do we need really need the postfix DiagnosticConsumer everywhere? Can't we convert it into a namespace? Maybe at least for PathDiagnosticConsumers? Its not a big deal, and it is certainly descriptive, but looks a bit funny.

Also, RIP ClangDiagPathDiagConsumer.

clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h

+//===--- PathDiagnosticConverterDiagnosticConsumer.h ------------*- 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines a Clang DiagnosticConsumer that converts Clang diagnostics
+//  into PathDiagnostics.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYSIS_PATHDIAGNOSTICCONVERTERDIAGNOSTICCONSUMER_H
+#define LLVM_CLANG_ANALYSIS_PATHDIAGNOSTICCONVERTERDIAGNOSTICCONSUMER_H
+
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
+
+namespace clang {
+namespace ento {
+
+class PathDiagnosticConverterDiagnosticConsumer : public DiagnosticConsumer {
+public:
+  struct BugTypeInfo {
+    StringRef CheckName;
+    StringRef BugType;
+    StringRef BugCategory;
+  };
+
+  using BugTypeInfoProvider = std::function<BugTypeInfo(const Diagnostic &)>;
+
+private:
+  PathDiagnosticConsumers &PathConsumers;
+  BugTypeInfoProvider BTIProvider;
+  bool ShouldDisplayNotesAsEvents;
+
+  std::map<PathDiagnosticConsumer *, std::unique_ptr<PathDiagnostic>>
+      PartialPDs;
+
+  void flushPartialDiagnostic();
+
+public:
+  PathDiagnosticConverterDiagnosticConsumer(
+      PathDiagnosticConsumers &PathConsumers, BugTypeInfoProvider &&BTIProvider,
+      bool ShouldDisplayNotesAsEvents)
+      : PathConsumers(PathConsumers), BTIProvider(std::move(BTIProvider)),
+        ShouldDisplayNotesAsEvents(ShouldDisplayNotesAsEvents) {}
+
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                        const Diagnostic &Info) override;
+
+  /// Inform the consumer that the last diagnostic has been sent. This is
+  /// necessary because the consumer does not know whether more notes are
+  /// going to be attached to the last warning.
+  void finalize() { flushPartialDiagnostic(); }
+};
+
+} // end 'ento' namespace
+} // end 'clang' namespace
+
+#endif

clang/lib/Analysis/CMakeLists.txt

add_clang_library(clangAnalysis
   CodeInjector.cpp
   Dominators.cpp
   ExprMutationAnalyzer.cpp
   HTMLPathDiagnosticConsumer.cpp
   IssueHash.cpp
   LiveVariables.cpp
   ObjCNoReturn.cpp
   PathDiagnostic.cpp
+  PathDiagnosticConverterDiagnosticConsumer.cpp
   PlistPathDiagnosticConsumer.cpp
   PlistHTMLPathDiagnosticConsumer.cpp
   PostOrderCFGView.cpp
   ProgramPoint.cpp
   ReachableCode.cpp
   RetainSummaryManager.cpp
   SarifPathDiagnosticConsumer.cpp
   TextPathDiagnosticConsumer.cpp

clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

+//===--- PathDiagnosticConverterDiagnosticConsumer.cpp ----------*- 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the PathDiagnosticConverterDiagnosticConsumer object
+//  which converts regular Clang diagnostics into PathDiagnostics.
+//
+//===----------------------------------------------------------------------===//
+
+#include <clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h>
+
+using namespace clang;
+using namespace ento;
+
+void PathDiagnosticConverterDiagnosticConsumer::flushPartialDiagnostic() {
+  if (PartialPDs.empty())
+    return;
+
+  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+    Consumer->HandlePathDiagnostic(std::move(PartialPDs[Consumer]));
+  }
+
+  PartialPDs.clear();
+}
+
+void PathDiagnosticConverterDiagnosticConsumer::HandleDiagnostic(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
+  // Count warnings/errors.
+  DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+
+  llvm::SmallString<128> Msg;
+  Info.FormatDiagnostic(Msg);
+  Msg[0] = toupper(Msg[0]);
+  std::string CleanMsg = Msg.str().substr(0, Msg.str().find('[') - 1).str();
+
+  SourceLocation Loc = Info.getLocation();
+  const SourceManager &SM = Info.getSourceManager();
+
+  PathDiagnosticLocation PDLoc(Loc, SM);
+
+  // Notes do not come in attached to their respective warnings but are fed
+  // to us independently. The good news is they always follow their respective
+  // warnings and come in in the right order so we can automatically re-attach
+  // them.
+  if (DiagLevel == DiagnosticsEngine::Note) {
+    for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+      PathDiagnosticPieceRef Piece;
+      if (ShouldDisplayNotesAsEvents) {
+        Piece = std::make_shared<PathDiagnosticEventPiece>(PDLoc, Msg);
+      } else {
+        Piece = std::make_shared<PathDiagnosticNotePiece>(PDLoc, Msg);
+      }
+      PartialPDs[Consumer]->getMutablePieces().push_back(Piece);
+    }
+  } else {
+    // No more incoming notes. The current path diagnostic is complete.
+    flushPartialDiagnostic();
+
+    BugTypeInfo BTI = BTIProvider(Info);
+
+    for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+      auto PD = std::make_unique<PathDiagnostic>(
+          BTI.CheckName, /*DeclWithIssue=*/nullptr, BTI.BugType,
+          CleanMsg, CleanMsg, BTI.BugCategory, PDLoc, /*DeclToUnique=*/nullptr,
+          std::make_unique<FilesToLineNumsMap>());
+
+      PathDiagnosticPieceRef Piece =
+          std::make_shared<PathDiagnosticEventPiece>(PDLoc, CleanMsg);
+
+      PD->setEndOfPath(std::move(Piece));
+
+      PartialPDs[Consumer] = std::move(PD);
+    }
+  }
+}

clang/unittests/Analysis/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_unittest(ClangAnalysisTests
   CFGDominatorTree.cpp
   CFGTest.cpp
   CloneDetectionTest.cpp
   ExprMutationAnalyzerTest.cpp
+  PathDiagnosticConverterDiagnosticConsumer.cpp
   )
 
 clang_target_link_libraries(ClangAnalysisTests
   PRIVATE
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangSerialization
   clangTooling
   )

clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp

+//===- unittests/Analysis/CFGTest.cpp - CFG tests -------------------------===//
+//
+// 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 "clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+using namespace llvm;
+
+namespace {
+class TestPathDiagnosticConsumer : public PathDiagnosticConsumer {
+public:
+  struct ExpectedPieceTy {
+    SourceLocation Loc;
+    std::string Text;
+  };
+  struct ExpectedDiagTy {
+    SourceLocation Loc;
+    std::string VerboseDescription;
+    std::string ShortDescription;
+    std::string CheckerName;
+    std::string BugType;
+    std::string Category;
+    std::vector<ExpectedPieceTy> Pieces;
+  };
+  typedef std::vector<ExpectedDiagTy> ExpectedDiagsTy;
+
+private:
+  ExpectedDiagsTy ExpectedDiags;
+
+public:
+  TestPathDiagnosticConsumer(ExpectedDiagsTy ExpectedDiags)
+    : ExpectedDiags(ExpectedDiags)
+  {}
+
+  virtual StringRef getName() const override { return "test"; }
+
+  virtual void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
+                                    FilesMade *filesMade) override {
+    EXPECT_EQ(Diags.size(), ExpectedDiags.size());
+    for (size_t I = 0, E = Diags.size(); I < E; ++I) {
+      const PathDiagnostic &Diag = *Diags[I];
+      const ExpectedDiagTy &ExpectedDiag = ExpectedDiags[I];
+
+      EXPECT_EQ(Diag.getLocation().asLocation(), ExpectedDiag.Loc);
+      EXPECT_EQ(Diag.getVerboseDescription(), ExpectedDiag.VerboseDescription);
+      EXPECT_EQ(Diag.getShortDescription(), ExpectedDiag.ShortDescription);
+      EXPECT_EQ(Diag.getCheckerName(), ExpectedDiag.CheckerName);
+      EXPECT_EQ(Diag.getBugType(), ExpectedDiag.BugType);
+      EXPECT_EQ(Diag.getCategory(), ExpectedDiag.Category);
+
+      const PathPieces &Pieces = Diag.path;
+      EXPECT_EQ(Pieces.size(), ExpectedDiag.Pieces.size());
+      size_t PieceI = 0;
+      for (const auto &Piece: Pieces) {
+        const ExpectedPieceTy &ExpectedPiece = ExpectedDiag.Pieces[PieceI];
+        EXPECT_EQ(Piece->getLocation().asLocation(), ExpectedPiece.Loc);
+        EXPECT_EQ(Piece->getString(), ExpectedPiece.Text);
+        ++PieceI;
+      }
+    }
+  }
+};
+
+PathDiagnosticConverterDiagnosticConsumer::BugTypeInfo bugTypeInfoProvider(
+    const Diagnostic &) {
+  return PathDiagnosticConverterDiagnosticConsumer::BugTypeInfo{
+      "test check name", "test bug type", "test bug category"};
+}
+
+class PathDiagnosticConverterDiagnosticConsumerTestConsumer
+    : public ASTConsumer {
+  SourceManager &SM;
+
+  void performTest(const Decl *D) {
+    TestPathDiagnosticConsumer::ExpectedDiagsTy ExpectedDiags{
+        TestPathDiagnosticConsumer::ExpectedDiagTy{
+            D->getBeginLoc(),
+            "Test warning", // Auto-capitalization!
+            "Test warning", // Auto-capitalization!
+            "test check name",
+            "test bug type",
+            "test bug category",
+            {TestPathDiagnosticConsumer::ExpectedPieceTy{
+                 D->getBeginLoc(),
+                 "Test warning", // Auto-capitalization!
+             },
+             TestPathDiagnosticConsumer::ExpectedPieceTy{
+                 D->getEndLoc(),
+                 "Test note" // Auto-capitalization!
+             }}}};
+
+    TestPathDiagnosticConsumer TestPDC {ExpectedDiags};
+    PathDiagnosticConsumers PathConsumers = {&TestPDC};
+
+    llvm::IntrusiveRefCntPtr<DiagnosticsEngine> DiagEngine =
+        new DiagnosticsEngine(new DiagnosticIDs, new DiagnosticOptions);
+    PathDiagnosticConverterDiagnosticConsumer DiagConsumer(
+        PathConsumers, bugTypeInfoProvider,
+        /*ShouldDisplayNotesAsEvents=*/true);
+    DiagEngine->setClient(&DiagConsumer, /*ShouldOwnClient=*/false);
+    DiagEngine->setSourceManager(&SM);
+
+    unsigned WarningDiagID =
+        DiagEngine->getCustomDiagID(DiagnosticsEngine::Warning, "test warning");
+    DiagEngine->Report(D->getBeginLoc(), WarningDiagID);
+
+    unsigned NoteDiagID =
+        DiagEngine->getCustomDiagID(DiagnosticsEngine::Note, "test note");
+    DiagEngine->Report(D->getEndLoc(), NoteDiagID);
+
+    // The last diagnostic must be flushed manually!
+    DiagConsumer.finalize();
+
+    PathDiagnosticConsumer::FilesMade FM;
+    TestPDC.FlushDiagnostics(&FM);
+  }
+
+public:
+  PathDiagnosticConverterDiagnosticConsumerTestConsumer(CompilerInstance &CI)
+    : SM(CI.getSourceManager()) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+    for (auto *D : DG)
+      performTest(D);
+
+    return true;
+  }
+};
+
+class PathDiagnosticConverterDiagnosticConsumerTestAction
+    : public ASTFrontendAction {
+public:
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef File) override {
+    return std::make_unique<
+        PathDiagnosticConverterDiagnosticConsumerTestConsumer>(CI);
+  }
+};
+
+// Constructing a CFG for a range-based for over a dependent type fails (but
+// should not crash).
+TEST(PathDiagnosticConverter, ConsumeWarning) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::make_unique<PathDiagnosticConverterDiagnosticConsumerTestAction>(),
+      "void foo() {}", "input.c"));
+}
+
+} // end of anonymous namespace