[PATCH] D65484: [analyzer][NFC] Refactoring BugReporter.cpp P5.: Compact mile long function invocations into objects

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso, dcoughlin, rnkovacs.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity.

This patch is scary big, I'm aware, but I genuinely didn't find a satisfying splitting point. To my defense, its also brain dead refactoring for the most part!

---

In D65379 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

---

Please, if you find this explanation, the size of the patch or anything too vague/too large, let me know and I'll make adjustments. Overall, I feel this is a giant leap towards the right direction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65484

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D65484.212445.patch
Type: text/x-patch
Size: 56286 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190730/19e20628/attachment-0001.bin>