[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86
+  if (const auto *DR = dyn_cast<DeclRegion>(DerefRegion)) {
+    auto SmartPtrName = DR->getDecl()->getName();
+    OS << " '" << SmartPtrName << "'";
----------------
Please `getNameAsString()` because `getName()` crashes on anonymous declarations (eg., lambda captures, anonymous nested structs/unions, etc.).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+    OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";
----------------
Aha, this time you checked for anonymous declarations! Good.

That said, i'm not sure type is actually useful for this warning, because they're roughly all the same.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {
----------------
I feel this is a bit over-engineered. There's no need to create an enum and later convert it into a string when you can capture a string directly. Like, this entire "details" structure of your, it should be just captures instead, and your strings would make it immediately obvious what kind of note is emitted:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
    return "";
 
  return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + "' is null";
}));
```

Hmm, maybe we should also provide an overload with lambdas of signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to make the same one-liners possible with streams? Something like this:

```lang=c++
class CheckerContext {
// ...
  NoteTag *getNoteTag(std::function<void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)> f) {
    return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
      llvm::SmallString<128> Str;
      llvm::raw_svector_ostream OS(Str);
      f(BR, OS);
      return OS.str();
    });
  }
// ...
};
```

Then you could do:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
    return;

  OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is null";
}));
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+        TagDetails.trackValidExpr(BR);
+        TagDetails.explainSmartPtrAction(Out);
+        return std::string(Out.str());
----------------
Ok, note that note tags are attached to nodes independently of bug reports; when the report is thrown, only then we know what are the smart pointers that should be explained.

So there are two checks that you should do here:

1. Check that the bug report is emitted by your checker (eg., by comparing bug types). If not, don't add notes.

2. Check that the region about which the note speaks is related to your report (i.e., it's not a completely unrelated smart pointer). You can do that by marking the smart pointer as "interesting" (i.e., `PathSensitiveBugReport::markIntersting()`) when you emit the report, and then in the lambda you check whether the smart pointer is interesting before you emit a note. Additionally, you can carry over interestingness when smart pointers are copied.

This is what i was trying to accomplish with this code snippet that i included in the examples in the other comment:
```lang=c++
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
    return "";
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84600/new/

https://reviews.llvm.org/D84600