[analyzer][NFC] Make sure that the BugReport is not modified during the construction of non-visitor pieces

[Szelethus]

Summary

I feel this is kinda important, because in a followup patch I'm adding different kinds of interestingness, and propagating the correct kind in BugReporter.cpp is just one less thing to worry about.

[NoQ]

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

108 ↗

(On Diff #212811)

I guess you should add a comment for anybody who would want to remove this const on why it's a bad idea(?)

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

protected:
   std::string ShortDescription;
   std::string Description;
   PathDiagnosticLocation Location;
   PathDiagnosticLocation UniqueingLocation;
   const Decl *UniqueingDecl;
 
   const ExplodedNode *ErrorNode = nullptr;
   SmallVector<SourceRange, 4> Ranges;
+  const SourceRange ErrorNodeRange;
   ExtraTextList ExtraText;
   NoteList Notes;
 
   /// A (stack of) a set of symbols that are registered with this
   /// report as being "interesting", and thus used to help decide which
   /// diagnostics to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
protected:
   /// \sa markInvalid
   /// \sa removeInvalidation
   llvm::SmallSet<InvalidationRecord, 4> Invalidations;
 
   /// Conditions we're already tracking.
   llvm::SmallSet<const ExplodedNode *, 4> TrackedConditions;
 
 public:
-  BugReport(const BugType& bt, StringRef desc, const ExplodedNode *errornode)
-      : BT(bt), Description(desc), ErrorNode(errornode) {}
+  BugReport(const BugType &bt, StringRef desc, const ExplodedNode *errornode)
+      : BT(bt), Description(desc), ErrorNode(errornode),
+        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
+                                 : SourceRange()) {}
 
-  BugReport(const BugType& bt, StringRef shortDesc, StringRef desc,
+  BugReport(const BugType &bt, StringRef shortDesc, StringRef desc,
             const ExplodedNode *errornode)
       : BT(bt), ShortDescription(shortDesc), Description(desc),
-        ErrorNode(errornode) {}
+        ErrorNode(errornode),
+        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
+                                 : SourceRange()) {}
 
   BugReport(const BugType &bt, StringRef desc, PathDiagnosticLocation l)
-      : BT(bt), Description(desc), Location(l) {}
+      : BT(bt), Description(desc), Location(l),
+        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
+                                 : SourceRange()) {}
 
   /// Create a BugReport with a custom uniqueing location.
   ///
   /// The reports that have the same report location, description, bug type, and
   /// ranges are uniqued - only one of the equivalent reports will be presented
   /// to the user. This method allows to rest the location which should be used
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
public:
   /// ranges.
   void addRange(SourceRange R) {
     assert((R.isValid() || Ranges.empty()) && "Invalid range can only be used "
                            "to specify that the report does not have a range.");
     Ranges.push_back(R);
   }
 
   /// Get the SourceRanges associated with the report.
-  virtual llvm::iterator_range<ranges_iterator> getRanges();
+  virtual llvm::iterator_range<ranges_iterator> getRanges() const;
 
   /// Add custom or predefined bug report visitors to this report.
   ///
   /// The visitors should be used when the default trace is not sufficient.
   /// For example, they allow constructing a more elaborate trace.
   /// \sa registerConditionVisitor(), registerTrackNullOrUndefValue(),
   /// registerFindLastStore(), registerNilReceiverVisitor(), and
   /// registerVarDeclsLastStore().

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

public:
   getEndPath(BugReporterContext &BRC, const ExplodedNode *N,
              BugReport &BR);
 
   virtual void Profile(llvm::FoldingSetNodeID &ID) const = 0;
 
   /// Generates the default final diagnostic piece.
   static PathDiagnosticPieceRef getDefaultEndPath(const BugReporterContext &BRC,
                                                   const ExplodedNode *N,
-                                                  BugReport &BR);
+                                                  const BugReport &BR);
 };
 
 /// Finds last store into the given region,
 /// which is different from a given symbolic value.
 class FindLastStoreBRVisitor final : public BugReporterVisitor {
   const MemRegion *R;
   SVal V;
   bool Satisfied = false;

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h

public:
   RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
               ExplodedNode *n, SymbolRef sym,
               bool isLeak=false);
 
   RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
               ExplodedNode *n, SymbolRef sym,
               StringRef endText);
 
-  llvm::iterator_range<ranges_iterator> getRanges() override {
+  llvm::iterator_range<ranges_iterator> getRanges() const override {
     if (!isLeak)
       return BugReport::getRanges();
     return llvm::make_range(ranges_iterator(), ranges_iterator());
   }
 };
 
 class RefLeakReport : public RefCountReport {
   const MemRegion* AllocBinding;

cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp

bool shouldGenerateDiagnostics() const {
     return Consumer->shouldGenerateDiagnostics();
   }
   bool supportsLogicalOpControlFlow() const {
     return Consumer->supportsLogicalOpControlFlow();
   }
 };
 
 /// Contains every contextual information needed for constructing a
-/// PathDiagnostic object for a given bug report. This class (and aside from
-/// some caching BugReport does in the background) and its fields are immutable,
-/// and passes a BugReportConstruct object around during the construction.
+/// PathDiagnostic object for a given bug report. This class and its fields are
+/// immutable, and passes a BugReportConstruct object around during the
+/// construction.
 class PathDiagnosticBuilder : public BugReporterContext {
   /// A linear path from the error node to the root.
   std::unique_ptr<const ExplodedGraph> BugPath;
-  BugReport *R;
+  /// The bug report we're describing. Visitors create their diagnostics with
+  /// them being the last entities being able to modify it (for example,
+  /// changing interestingness here would cause inconsistencies as to how this
+  /// file and visitors construct diagnostics), hence its const.
+  const BugReport *R;
   /// The leaf of the bug path. This isn't the same as the bug reports error
   /// node, which refers to the *original* graph, not the bug path.
   const ExplodedNode *const ErrorNode;
   /// The diagnostic pieces visitors emitted, which is expected to be collected
   /// by the time this builder is constructed.
   std::unique_ptr<const VisitorsDiagnosticsTy> VisitorsDiagnostics;
 
 public:
private:
 
   PathDiagnosticLocation
   ExecutionContinues(const PathDiagnosticConstruct &C) const;
 
   PathDiagnosticLocation
   ExecutionContinues(llvm::raw_string_ostream &os,
                      const PathDiagnosticConstruct &C) const;
 
-  BugReport *getBugReport() const { return R; }
+  const BugReport *getBugReport() const { return R; }
 };
 
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // Helper routines for walking the ExplodedGraph and fetching statements.
 //===----------------------------------------------------------------------===//
 
if (BE->getBlock() == &Exit)
       S = GetPreviousStmt(ErrorNode);
   }
   if (!S)
     S = PathDiagnosticLocation::getStmt(ErrorNode);
 
   return S;
 }
 
-llvm::iterator_range<BugReport::ranges_iterator> BugReport::getRanges() {
+llvm::iterator_range<BugReport::ranges_iterator> BugReport::getRanges() const {
   // If no custom ranges, add the range of the statement corresponding to
   // the error node.
   if (Ranges.empty()) {
     if (const auto *E = dyn_cast_or_null<Expr>(getStmt()))
-      addRange(E->getSourceRange());
-    else
-      return llvm::make_range(ranges_iterator(), ranges_iterator());
+      return llvm::make_range(&ErrorNodeRange, &ErrorNodeRange + 1);
   }
 
   // User-specified absence of range info.
   if (Ranges.size() == 1 && !Ranges.begin()->isValid())
     return llvm::make_range(ranges_iterator(), ranges_iterator());
 
   return llvm::make_range(Ranges.begin(), Ranges.end());
 }

cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 }
 
 void BugReporterVisitor::finalizeVisitor(BugReporterContext &,
                                          const ExplodedNode *, BugReport &) {}
 
 PathDiagnosticPieceRef
 BugReporterVisitor::getDefaultEndPath(const BugReporterContext &BRC,
                                       const ExplodedNode *EndPathNode,
-                                      BugReport &BR) {
+                                      const BugReport &BR) {
   PathDiagnosticLocation L = PathDiagnosticLocation::createEndOfPath(
       EndPathNode, BRC.getSourceManager());
 
   const auto &Ranges = BR.getRanges();
 
   // Only add the statement itself as a range if we didn't specify any
   // special ranges for this report.
   auto P = std::make_shared<PathDiagnosticEventPiece>(