[StaticAnalyzer] Added notes to the plist output

[Szelethus]

Summary

Added notes to -analyzer-output=plist.

[NoQ]

The output looks reasonable to me, but we'll need to see if other consumers of the plist output (IDEs that supports the analyzer, such as Xcode) will be able to accept the modified output (at least, will be able to ignore it). I'll have a look.

[Szelethus]

Thanks!

[Szelethus]

Did you have some time to check on those programs? :)

[NoQ]

Sorry, overwhelmed a bit, i'll try to get to this as soon as possible.

Btw, what sort of UI are you trying to make these extra note pieces of mine work with? Was -analyzer-config notes-as-events=true of any help?

[Szelethus]

Btw, what sort of UI are you trying to make these extra note pieces of mine work with?

I'm also developing a checker: https://reviews.llvm.org/D45532. This checker emits very important information in notes. When I tried testing it with Ericsson's CodeChecker, I realized that notes aren't displayed. I'm planning to extend it so it will be able to.

Was -analyzer-config notes-as-events=true of any help?

Never knew about the existence of this flag. Woops. I'll only be able to check it out tomorrow, but I'll post my findings here as soon as I have them :)

[Szelethus]

I just had a look with -analyzer-config notes-as-events=true, and it works (with CodeChecker, at least). I'd still implement a better support for notes, but this is a great workaround for now. Thanks!

[whisperity]

@NoQ The problem with emitting notes as events is that we lose the information that the node was a note. How does Xcode behave with these notes? Does it ignore them, or can read them from the command-line output of the analyser?

[NoQ]

Checked this out, seems to be safely ignored for now. The .plist format is pretty resistant to this sort of stuff because most APIs to handle it are almost treating it as native arrays/dictionaries, so i guess it should be fine.

In D45407#1068724, @whisperity wrote:

@NoQ The problem with emitting notes as events is that we lose the information that the node was a note. How does Xcode behave with these notes? Does it ignore them, or can read them from the command-line output of the analyser?

Yep, precisely: notes are completely different from path pieces - they shouldn't be connected to the rest of the path via arrows or necessarily have a certain ordering with respect to the path.

For now notes are ignored by Xcode; there's no good UI for them implemented. They are also not used by many on-by-default checkers, so they're mostly an experiment for now.

[Szelethus]

Can someone commit this for me? I don't have a write access.

[NoQ]

Yup thanks!~

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

 void ento::createPlistMultiFileDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
                                                   PathDiagnosticConsumers &C,
                                                   const std::string &s,
                                                   const Preprocessor &PP) {
   C.push_back(new PlistDiagnostics(AnalyzerOpts, s,
                                    PP.getLangOpts(), true));
 }
 
+static void EmitRanges(raw_ostream &o,
+                       const ArrayRef<SourceRange> Ranges,
+                       const FIDMap& FM,
+                       const SourceManager &SM,
+                       const LangOptions &LangOpts,
+                       unsigned indent) {
+
+  if (Ranges.empty())
+    return;
+
+  Indent(o, indent) << "<key>ranges</key>\n";
+  Indent(o, indent) << "<array>\n";
+  ++indent;
+  for (auto &R : Ranges)
+    EmitRange(o, SM,
+              Lexer::getAsCharRange(SM.getExpansionRange(R), SM, LangOpts),
+              FM, indent + 1);
+  --indent;
+  Indent(o, indent) << "</array>\n";
+}
+
+static void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent) {
+  // Output the text.
+  assert(!Message.empty());
+  Indent(o, indent) << "<key>extended_message</key>\n";
+  Indent(o, indent);
+  EmitString(o, Message) << '\n';
+
+  // Output the short text.
+  // FIXME: Really use a short string.
+  Indent(o, indent) << "<key>message</key>\n";
+  Indent(o, indent);
+  EmitString(o, Message) << '\n';
+}
+
 static void ReportControlFlow(raw_ostream &o,
                               const PathDiagnosticControlFlowPiece& P,
                               const FIDMap& FM,
                               const SourceManager &SM,
                               const LangOptions &LangOpts,
                               unsigned indent) {
 
   Indent(o, indent) << "<dict>\n";
if (!s.empty()) {
     Indent(o, indent) << "<key>alternate</key>";
     EmitString(o, s) << '\n';
   }
 
   --indent;
   Indent(o, indent) << "</dict>\n";
 }
 
-static void ReportEvent(raw_ostream &o, const PathDiagnosticPiece& P,
+static void ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
                         const FIDMap& FM,
                         const SourceManager &SM,
                         const LangOptions &LangOpts,
                         unsigned indent,
                         unsigned depth,
                         bool isKeyEvent = false) {
 
   Indent(o, indent) << "<dict>\n";
   ++indent;
 
   Indent(o, indent) << "<key>kind</key><string>event</string>\n";
 
   if (isKeyEvent) {
     Indent(o, indent) << "<key>key_event</key><true/>\n";
   }
 
   // Output the location.
   FullSourceLoc L = P.getLocation().asLocation();
 
   Indent(o, indent) << "<key>location</key>\n";
   EmitLocation(o, SM, L, FM, indent);
 
   // Output the ranges (if any).
   ArrayRef<SourceRange> Ranges = P.getRanges();
-
-  if (!Ranges.empty()) {
-    Indent(o, indent) << "<key>ranges</key>\n";
-    Indent(o, indent) << "<array>\n";
-    ++indent;
-    for (auto &R : Ranges)
-      EmitRange(o, SM,
-                Lexer::getAsCharRange(SM.getExpansionRange(R), SM, LangOpts),
-                FM, indent + 1);
-    --indent;
-    Indent(o, indent) << "</array>\n";
-  }
+  EmitRanges(o, Ranges, FM, SM, LangOpts, indent);
 
   // Output the call depth.
   Indent(o, indent) << "<key>depth</key>";
   EmitInteger(o, depth) << '\n';
 
   // Output the text.
-  assert(!P.getString().empty());
-  Indent(o, indent) << "<key>extended_message</key>\n";
-  Indent(o, indent);
-  EmitString(o, P.getString()) << '\n';
-
-  // Output the short text.
-  // FIXME: Really use a short string.
-  Indent(o, indent) << "<key>message</key>\n";
-  Indent(o, indent);
-  EmitString(o, P.getString()) << '\n';
+  EmitMessage(o, P.getString(), indent);
 
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
 static void ReportPiece(raw_ostream &o,
                         const PathDiagnosticPiece &P,
static void ReportMacro(raw_ostream &o,
                         unsigned depth) {
 
   for (PathPieces::const_iterator I = P.subPieces.begin(), E=P.subPieces.end();
        I!=E; ++I) {
     ReportPiece(o, **I, FM, SM, LangOpts, indent, depth, false);
   }
 }
 
+static void ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
+                        const FIDMap& FM,
+                        const SourceManager &SM,
+                        const LangOptions &LangOpts,
+                        unsigned indent,
+                        unsigned depth) {
+
+  Indent(o, indent) << "<dict>\n";
+  ++indent;
+
+  // Output the location.
+  FullSourceLoc L = P.getLocation().asLocation();
+
+  Indent(o, indent) << "<key>location</key>\n";
+  EmitLocation(o, SM, L, FM, indent);
+
+  // Output the ranges (if any).
+  ArrayRef<SourceRange> Ranges = P.getRanges();
+  EmitRanges(o, Ranges, FM, SM, LangOpts, indent);
+
+  // Output the text.
+  EmitMessage(o, P.getString(), indent);
+
+  // Finish up.
+  --indent;
+  Indent(o, indent); o << "</dict>\n";
+}
+
 static void ReportDiag(raw_ostream &o, const PathDiagnosticPiece& P,
                        const FIDMap& FM, const SourceManager &SM,
                        const LangOptions &LangOpts) {
   ReportPiece(o, P, FM, SM, LangOpts, 4, 0, true);
 }
 
 static void ReportPiece(raw_ostream &o,
                         const PathDiagnosticPiece &P,
case PathDiagnosticPiece::ControlFlow:
         ReportControlFlow(o, cast<PathDiagnosticControlFlowPiece>(P), FM, SM,
                           LangOpts, indent);
       break;
     case PathDiagnosticPiece::Call:
       ReportCall(o, cast<PathDiagnosticCallPiece>(P), FM, SM, LangOpts,
                  indent, depth);
       break;
     case PathDiagnosticPiece::Event:
-      ReportEvent(o, cast<PathDiagnosticSpotPiece>(P), FM, SM, LangOpts,
+      ReportEvent(o, cast<PathDiagnosticEventPiece>(P), FM, SM, LangOpts,
                   indent, depth, isKeyEvent);
       break;
     case PathDiagnosticPiece::Macro:
       ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, SM, LangOpts,
                   indent, depth);
       break;
     case PathDiagnosticPiece::Note:
-      // FIXME: Extend the plist format to support those.
+      ReportNote(o, cast<PathDiagnosticNotePiece>(P), FM, SM, LangOpts,
+                  indent, depth);
       break;
   }
 }
 
 void PlistDiagnostics::FlushDiagnosticsImpl(
                                     std::vector<const PathDiagnostic *> &Diags,
                                     FilesMade *filesMade) {
   // Build up a set of FIDs that we use by scanning the locations and
void PlistDiagnostics::FlushDiagnosticsImpl(
 
   o << " </array>\n"
        " <key>diagnostics</key>\n"
        " <array>\n";
 
   for (std::vector<const PathDiagnostic*>::iterator DI=Diags.begin(),
        DE = Diags.end(); DI!=DE; ++DI) {
 
-    o << "  <dict>\n"
-         "   <key>path</key>\n";
+    o << "  <dict>\n";
 
     const PathDiagnostic *D = *DI;
+    const PathPieces &PP = D->path;
+
+    assert(std::is_partitioned(
+             PP.begin(), PP.end(),
+             [](const std::shared_ptr<PathDiagnosticPiece> &E)
+               { return E->getKind() == PathDiagnosticPiece::Note; }) &&
+           "PathDiagnostic is not partitioned so that notes precede the rest");
+
+    PathPieces::const_iterator FirstNonNote = std::partition_point(
+        PP.begin(), PP.end(),
+        [](const std::shared_ptr<PathDiagnosticPiece> &E)
+          { return E->getKind() == PathDiagnosticPiece::Note; });
+
+    PathPieces::const_iterator I = PP.begin();
+
+    if (FirstNonNote != PP.begin()) {
+      o << "   <key>notes</key>\n"
+           "   <array>\n";
+
+      for (; I != FirstNonNote; ++I)
+        ReportDiag(o, **I, FM, *SM, LangOpts);
+
+      o << "   </array>\n";
+    }
+
+    o << "   <key>path</key>\n";
 
     o << "   <array>\n";
 
-    for (PathPieces::const_iterator I = D->path.begin(), E = D->path.end();
-         I != E; ++I)
+    for (PathPieces::const_iterator E = PP.end(); I != E; ++I)
       ReportDiag(o, **I, FM, *SM, LangOpts);
 
     o << "   </array>\n";
 
     // Output the bug type and bug category.
     o << "   <key>description</key>";
     EmitString(o, D->getShortDescription()) << '\n';
     o << "   <key>category</key>";

cfe/trunk/test/Analysis/copypaste/plist-diagnostics.cpp

 
 int maxClone(int a, int b) { // expected-note{{Similar code here}}
   log();
   if (a > b)
     return a;
   return b;
 }
 
-// FIXME: This plist output doesn't include the extra note on line 13.
-// It should be updated once the format for extra notes in plists is defined.
-
 // CHECK:  <key>diagnostics</key>
 // CHECK-NEXT:  <array>
 // CHECK-NEXT:   <dict>
+// CHECK-NEXT:    <key>notes</key>
+// CHECK-NEXT:    <array>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>13</integer>
+// CHECK-NEXT:       <key>col</key><integer>28</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>ranges</key>
+// CHECK-NEXT:      <array>
+// CHECK-NEXT:        <array>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>13</integer>
+// CHECK-NEXT:          <key>col</key><integer>28</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>18</integer>
+// CHECK-NEXT:          <key>col</key><integer>1</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:        </array>
+// CHECK-NEXT:      </array>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>Similar code here</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>Similar code here</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:    </array>
 // CHECK-NEXT:    <key>path</key>
 // CHECK-NEXT:    <array>
 // CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>event</string>
 // CHECK-NEXT:      <key>location</key>
 // CHECK-NEXT:      <dict>
 // CHECK-NEXT:       <key>line</key><integer>6</integer>
 // CHECK-NEXT:       <key>col</key><integer>23</integer>