[analyzer] Display the checker name in the text output

[Szelethus]

Summary

Exactly what it says on the tin! There is no reason I think not to have this.

Also, I added test files for checkers that emit warning under the wrong name.

[martong]

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

98

StringRef? getCheckerName() returns with StringRef...

112

Why the .str() ?

clang/test/Analysis/incorrect-checker-names.mm

1

This seems like a copy of an existing test file, though I don't know which. Wouldn't it be better to adjust the original test file?

[Szelethus]

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

98

Mind that we're constructing a string here from a bunch of smaller ones, we need an owning container. With that said, I could use a string stream instead.

112

StringRef no longer converts to std::string implicitly.

clang/test/Analysis/incorrect-checker-names.mm

1

Good observation, these test cases are indeed copied from other files. The justification behind it however is to highlight that these are the checkers that need fixing. Adding FIXMEs to other thousand-line test files don't grab the attention enough :)

[NoQ]

Sounds great!

Does the checker name get printed on the final note as well in text output? We should probably test it.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

313–316

Why do we need an option? Is it just for tests? Is it for clang-tidy to avoid printing the checker name twice?

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

112

But it seems to have been fine before(?)

clang/test/Analysis/incorrect-checker-names.cpp

6

Do you have a plan to address this FIXME? Does clang-tidy have the same problem?

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

313–316

Why do we need an option?

Well, we don't, but there is just no reason not to make this configurable, as seen in the debug checker test file. It would be unnecessary noise in their case.

Is it just for tests?

Tests are a big motivating factor, but it just doesn't hurt to know (just like in clang-tidy!). I suspect this change affects developers or powerusers more than any others, and could accelerate debugging and/or configuring just a tiny bit more.

Is it for clang-tidy to avoid printing the checker name twice?

Clang-tidy isnt affected, as they use the PD_NONE output type, not PD_TEXT.

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

112

Mind that I changed the parameter of reportPiece to std::string from StringRef.

clang/test/Analysis/incorrect-checker-names.cpp

6

It does not. This is the issue of emitting reports under the wrong name, and I do have plans to address it, though I'm very anxious about RetainCountChecker. Its huge, the modeling/bug emission parts of it a very much intertwined, and me dum dum about ObjC.

[NoQ]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

313–316

Clang-tidy isnt affected, as they use the PD_NONE output type, not PD_TEXT.

But they do have path notes printed out. It would have been terrible if they didn't have path notes.

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

313–316

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidy.cpp#L68

Clang-tidy implements its own PathDiagnosticConsumer.

[Szelethus]

Fixing according to the reviews!

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

112

Couldn't see the forest behind the trees :^)

[martong]

I was just wondering if it would make sense to have a test with display-checker-name=true. Other than that, LGTM!

[Szelethus]

In D76605#1969535, @martong wrote:

I was just wondering if it would make sense to have a test with display-checker-name=true. Other than that, LGTM!

Could do! Thanks!

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

 ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
                 "Whether to place an event at each tracked condition.",
                 false)
 
 ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
                 "Emit fix-it hints as remarks for testing purposes",
                 false)
 
+ANALYZER_OPTION(bool, ShouldDisplayCheckerNameForText, "display-checker-name",
+                "Display the checker name for textual outputs",
+                true)
+
 //===----------------------------------------------------------------------===//
 // Unsigned analyzer options.
 //===----------------------------------------------------------------------===//
 
 ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
                 "The maximal amount of translation units that is considered "
                 "for import when inlining functions during CTU analysis. "
                 "Lowering this threshold can alleviate the memory burder of "

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

 namespace {
 
 /// Emitsd minimal diagnostics (report message + notes) for the 'none' output
 /// type to the standard error, or to to compliment many others. Emits detailed
 /// diagnostics in textual format for the 'text' output type.
 class TextDiagnostics : public PathDiagnosticConsumer {
   DiagnosticsEngine &DiagEng;
   const bool IncludePath = false, ShouldEmitAsError = false,
-             FixitsAsRemarks = false;
+             FixitsAsRemarks = false, ShouldDisplayCheckerName = false;
 
 public:
   TextDiagnostics(DiagnosticsEngine &Diag, bool ShouldIncludePath,
                   const AnalyzerOptions &AnOpts)
       : DiagEng(Diag), IncludePath(ShouldIncludePath),
         ShouldEmitAsError(AnOpts.AnalyzerWerror),
-        FixitsAsRemarks(AnOpts.ShouldEmitFixItHintsAsRemarks) {}
+        FixitsAsRemarks(AnOpts.ShouldEmitFixItHintsAsRemarks),
+        ShouldDisplayCheckerName(AnOpts.ShouldDisplayCheckerNameForText) {}
   ~TextDiagnostics() override {}
 
   StringRef getName() const override { return "TextDiagnostics"; }
 
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
     return IncludePath ? Minimal : None;
   }
 
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
     unsigned WarnID =
         ShouldEmitAsError
             ? DiagEng.getCustomDiagID(DiagnosticsEngine::Error, "%0")
             : DiagEng.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
     unsigned NoteID = DiagEng.getCustomDiagID(DiagnosticsEngine::Note, "%0");
     unsigned RemarkID =
         DiagEng.getCustomDiagID(DiagnosticsEngine::Remark, "%0");
 
-    auto reportPiece = [&](unsigned ID, SourceLocation Loc, StringRef String,
+    auto reportPiece = [&](unsigned ID, SourceLocation Loc, std::string String,
                            ArrayRef<SourceRange> Ranges,
                            ArrayRef<FixItHint> Fixits) {
       if (!FixitsAsRemarks) {
         DiagEng.Report(Loc, ID) << String << Ranges << Fixits;
       } else {
         DiagEng.Report(Loc, ID) << String << Ranges;
         for (const FixItHint &Hint : Fixits) {
           SourceManager &SM = DiagEng.getSourceManager();
auto reportPiece = [&](unsigned ID, SourceLocation Loc, std::string String,
         }
       }
     };
 
     for (std::vector<const PathDiagnostic *>::iterator I = Diags.begin(),
                                                        E = Diags.end();
          I != E; ++I) {
       const PathDiagnostic *PD = *I;
+      std::string WarningMsg =
+          (ShouldDisplayCheckerName ? " [" + PD->getCheckerName() + "]" : "")
+              .str();
+
       reportPiece(WarnID, PD->getLocation().asLocation(),
-                  PD->getShortDescription(), PD->path.back()->getRanges(),
-                  PD->path.back()->getFixits());
+                  (PD->getShortDescription() + WarningMsg).str(),
+                  PD->path.back()->getRanges(), PD->path.back()->getFixits());
 
       // First, add extra notes, even if paths should not be included.
       for (const auto &Piece : PD->path) {
         if (!isa<PathDiagnosticNotePiece>(Piece.get()))
           continue;
 
         reportPiece(NoteID, Piece->getLocation().asLocation(),
-                    Piece->getString(), Piece->getRanges(), Piece->getFixits());
+                    Piece->getString().str(), Piece->getRanges(),
+                    Piece->getFixits());
       }
 
       if (!IncludePath)
         continue;
 
       // Then, add the path notes if necessary.
       PathPieces FlatPath = PD->path.flatten(/*ShouldFlattenMacros=*/true);
       for (const auto &Piece : FlatPath) {
         if (isa<PathDiagnosticNotePiece>(Piece.get()))
           continue;
 
         reportPiece(NoteID, Piece->getLocation().asLocation(),
-                    Piece->getString(), Piece->getRanges(), Piece->getFixits());
+                    Piece->getString().str(), Piece->getRanges(),
+                    Piece->getFixits());
       }
     }
   }
 };
 
 } // namespace
 
 void ento::createTextPathDiagnosticConsumer(

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCastExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtOffsetOfExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtCXXNewExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtCastExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PreStmtOffsetOfExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:RegionChanges = false
+// CHECK-NEXT: display-checker-name = true
 // CHECK-NEXT: display-ctu-progress = false
 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: expand-macros = false
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: fixits-as-remarks = false

 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 97
+// CHECK-NEXT: num-entries = 98

clang/test/Analysis/dispatch-once.m

-// RUN: %clang_analyze_cc1 -w -fblocks -analyzer-checker=core,osx.API,unix.Malloc -verify %s
-// RUN: %clang_analyze_cc1 -w -fblocks -fobjc-arc -analyzer-checker=core,osx.API,unix.Malloc -verify %s
+// RUN: %clang_analyze_cc1 -w -fblocks -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.API \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config display-checker-name=false
+
+// RUN: %clang_analyze_cc1 -w -fblocks -fobjc-arc -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.API \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config display-checker-name=false
 
 #include "Inputs/system-header-simulator-objc.h"
 
 typedef unsigned long size_t;
 void *calloc(size_t nmemb, size_t size);
 
 typedef void (^dispatch_block_t)(void);
 typedef long dispatch_once_t;

clang/test/Analysis/explain-svals.c

-// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-checker=core.builtin,debug.ExprInspection,unix.cstring -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -verify %s \
+// RUN:   -analyzer-checker=core.builtin \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-checker=unix.cstring \
+// RUN:   -analyzer-config display-checker-name=false
 
 struct S {
   int z;
 };
 
 void clang_analyzer_explain_int(int);
 void clang_analyzer_explain_voidp(void *);
 void clang_analyzer_explain_S(struct S);

clang/test/Analysis/explain-svals.cpp

-// RUN: %clang_analyze_cc1 -std=c++14 -triple i386-apple-darwin10 -analyzer-checker=core.builtin,debug.ExprInspection,unix.cstring -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -verify %s \
+// RUN:   -analyzer-checker=core.builtin \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-checker=unix.cstring \
+// RUN:   -analyzer-config display-checker-name=false
 
 typedef unsigned long size_t;
 
 struct S {
   struct S3 {
     int y[10];
   };
   struct S2 : S3 {

clang/test/Analysis/explain-svals.m

-// RUN: %clang_analyze_cc1 -w -triple i386-apple-darwin10 -fblocks -analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -w -triple i386-apple-darwin10 -fblocks -verify %s \
+// RUN:   -analyzer-checker=core.builtin \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config display-checker-name=false
 
 #include "Inputs/system-header-simulator-objc.h"
 
 void clang_analyzer_explain(void *);
 
 @interface Object : NSObject {
 @public
   Object *x;

clang/test/Analysis/incorrect-checker-names.cpp

+// RUN: %clang_analyze_cc1 -verify %s -fblocks \
+// RUN:   -analyzer-checker=core 
+
+int* stack_addr_escape_base() {
+  int x = 0;
+  // FIXME: This shouldn't be tied to a modeling checker.
+  return &x; // expected-warning{{Address of stack memory associated with local variable 'x' returned to caller [core.StackAddrEscapeBase]}}
+  // Just a regular compiler warning.
+  // expected-warning@-2{{address of stack memory associated with local variable 'x' returned}}
+}
+

clang/test/Analysis/incorrect-checker-names.mm

+// RUN: %clang_analyze_cc1 -fblocks -verify %s -Wno-objc-root-class \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=nullability \
+// RUN:   -analyzer-checker=osx
+
+#include "Inputs/system-header-simulator-for-nullability.h"
+#include "os_object_base.h"
+
+struct OSIterator : public OSObject {
+  static const OSMetaClass * const metaClass;
+};
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template <typename T> T *eraseNullab(T *p) { return p; }
+
+void takesAttrNonnull(Dummy *p) __attribute((nonnull(1)));
+
+void testBasicRules() {
+  // FIXME: None of these should be tied to a modeling checker.
+  Dummy *p = returnsNullable();
+  int *ptr = returnsNullableInt();
+  // Make every dereference a different path to avoid sinks after errors.
+  switch (getRandom()) {
+  case 0: {
+    Dummy &r = *p; // expected-warning {{Nullable pointer is dereferenced [nullability.NullabilityBase]}}
+  } break;
+  case 1: {
+    int b = p->val; // expected-warning {{Nullable pointer is dereferenced [nullability.NullabilityBase]}}
+  } break;
+  case 2: {
+    int stuff = *ptr; // expected-warning {{Nullable pointer is dereferenced [nullability.NullabilityBase]}}
+  } break;
+  case 3:
+    takesNonnull(p); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter [nullability.NullabilityBase]}}
+    break;
+  case 4: {
+    Dummy d;
+    takesNullable(&d);
+    Dummy dd(d);
+    break;
+  }
+  case 5: takesAttrNonnull(p); break; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null [nullability.NullabilityBase]}}
+  default: { Dummy d = *p; } break; // expected-warning {{Nullable pointer is dereferenced [nullability.NullabilityBase]}}
+  }
+  if (p) {
+    takesNonnull(p);
+    if (getRandom()) {
+      Dummy &r = *p;
+    } else {
+      int b = p->val;
+    }
+  }
+  Dummy *q = 0;
+  if (getRandom()) {
+    takesNullable(q);
+  // FIXME: This shouldn't be tied to a modeling checker.
+    takesNonnull(q); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter [nullability.NullabilityBase]}}
+  }
+  Dummy a;
+  Dummy *_Nonnull nonnull = &a;
+  // FIXME: This shouldn't be tied to a modeling checker.
+  nonnull = q; // expected-warning {{Null assigned to a pointer which is expected to have non-null value [nullability.NullabilityBase]}}
+  q = &a;
+  takesNullable(q);
+  takesNonnull(q);
+}
+
+typedef int NSInteger;
+typedef struct _NSZone NSZone;
+@class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
+@class NSDictionary;
+@interface NSError : NSObject <NSCopying, NSCoding> {}
++ (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
+@end
+
+struct __CFError {};
+typedef struct __CFError* CFErrorRef;
+
+void foo(CFErrorRef* error) { // expected-warning{{Function accepting CFErrorRef* should have a non-void return value to indicate whether or not an error occurred [osx.coreFoundation.CFError]}}
+  // FIXME: This shouldn't be tied to a modeling checker.
+  *error = 0;  // expected-warning {{Potential null dereference.  According to coding standards documented in CoreFoundation/CFError.h the parameter may be null [osx.NSOrCFErrorDerefChecker]}}
+}
+
+bool write_into_out_param_on_success(OS_RETURNS_RETAINED OSObject **obj);
+
+void use_out_param_leak() {
+  OSObject *obj;
+  // FIXME: This shouldn't be tied to a modeling checker.
+  write_into_out_param_on_success(&obj); // expected-warning{{Potential leak of an object stored into 'obj' [osx.cocoa.RetainCountBase]}}
+}