[analyzer][NFC] Refactoring BugReporter.cpp P5.: Compact mile long fu…

…nction invocations into objects

In D65379, I briefly described the construction of bug paths from an
ExplodedGraph. This patch is about refactoring the code processing the bug path
into a bug report.

A part of finding a valid bug report was running all visitors on the bug path,
so we already have a (possibly empty) set of diagnostics for each ExplodedNode
in it.
Then, for each diagnostic consumer, we construct non-visitor diagnostic pieces.

* We first construct the final diagnostic piece (the warning), then
* We start ascending the bug path from the error node's predecessor (since the
error node itself was used to construct the warning event). For each node
  * We check the location (whether its a CallEnter, CallExit) etc. We simultaneously
  keep track of where we are with the execution by pushing CallStack when we see a
  CallExit (keep in mind that everything is happening in reverse!), popping it
  when we find a CallEnter, compacting them into a single PathDiagnosticCallEvent.

void f() {
  bar();
}

void g() {
  f();
  error(); // warning
}

=== The bug path ===

(root) -> f's CallEnter -> bar() -> f's CallExit -> (error node)

=== Constructed report ===

  f's CallEnter -> bar() -> f's CallExit
           ^               /
            \             V
(root) --->  f's CallEvent --> (error node)

  * We also keep track of different PathPieces different location contexts
  * (CallEvent::path in the above example has f's LocationContext, while the
  CallEvent itself is in g's context) in a LocationContextMap object. Construct
  whatever piece, if any, is needed for the note.
  * If we need to generate edges (or arrows) do so. Make sure to also connect
  these pieces with the ones that visitors emitted.
  * Clean up the constructed PathDiagnostic by making arrows nicer, pruning
  function calls, etc.

So I complained about mile long function invocations with seemingly the same
parameters being passed around. This problem, as I see it, a natural candidate
for creating classes and tying them all together.

I tried very hard to make the implementation feel natural, like, rolling off the
tongue. I introduced 2 new classes: PathDiagnosticBuilder (I mean, I kept the
name but changed almost everything in it) contains every contextual information
(owns the bug path, the diagnostics constructed but the visitors, the BugReport
itself, etc) needed for constructing a PathDiagnostic object, and is pretty much
completely immutable. BugReportContruct is the object containing every
non-contextual information (the PathDiagnostic object we're constructing, the
current location in the bug path, the location context map and the call stack I
meantioned earlier), and is passed around all over the place as a single entity
instead of who knows how many parameters.

I tried to used constness, asserts, limiting visibility of fields to my
advantage to clean up the code big time and dramatically improve safety. Also,
whenever I found the code difficult to understand, I added comments and/or
examples.

Here's a complete list of changes and my design philosophy behind it:

* Instead of construcing a ReportInfo object (added by D65379) after finding a
valid bug report, simply return an optional PathDiagnosticBuilder object straight
away. Move findValidReport into the class as a static method. I find
GRBugReporter::generatePathDiagnostics a joy to look at now.
* Rename generatePathDiagnosticForConsumer to generate (maybe not needed, but
felt that way in the moment) and moved it to PathDiagnosticBuilder. If we don't
need to generate diagnostics, bail out straight away, like we always should have.
After that, construct a BugReportConstruct object, leaving the rest of the logic
untouched.
* Move all static methods that would use contextual information into
PathDiagnosticBuilder, reduce their parameter count drastically by simply
passing around a BugReportConstruct object.
* Glance at the code I removed: Could you tell what the original
PathDiagnosticBuilder::LC object was for? It took a gooood long while for me to
realize that nothing really. It is always equal with the LocationContext
associated with our current position in the bug path. Remove it completely.
* The original code contains the following expression quite a bit:
LCM[&PD.getActivePath()], so what does it mean? I said that we collect the
contexts associated with different PathPieces, but why would we ever modify that,
shouldn't it be set? Well, theoretically yes, but in the implementation, the
address of PathDiagnostic::getActivePath doesn't change if we move to an outer,
previously unexplored function. Add both descriptive method names and
explanations to BugReportConstruct to help on this.
* Add plenty of asserts, both for safety and as a poor man's documentation.

Differential Revision: https://reviews.llvm.org/D65484

llvm-svn: 368737

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

@@ -80,7 +80,7 @@ class BugReporterVisitor : public llvm::FoldingSetNode {
   virtual void Profile(llvm::FoldingSetNodeID &ID) const = 0;
 
   /// Generates the default final diagnostic piece.
-  static PathDiagnosticPieceRef getDefaultEndPath(BugReporterContext &BRC,
+  static PathDiagnosticPieceRef getDefaultEndPath(const BugReporterContext &BRC,
                                                   const ExplodedNode *N,
                                                   BugReport &BR);
 };

clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

@@ -125,6 +125,13 @@ class PathDiagnosticConsumer {
   };
 
   virtual PathGenerationScheme getGenerationScheme() const { return Minimal; }
+
+  bool shouldGenerateDiagnostics() const {
+    return getGenerationScheme() != None;
+  }
+
+  bool shouldAddPathEdges() const { return getGenerationScheme() == Extensive; }
+
   virtual bool supportsLogicalOpControlFlow() const { return false; }
 
   /// Return true if the PathDiagnosticConsumer supports individual