[analyzer] Fix a couple of bugs in HTML report generation.

[NoQ]

Summary

This patch fixes an accidental redundant </td> and disables generation of variable popups within macro popups due to https://bugs.llvm.org/show_bug.cgi?id=44782

I also added some tests for our HTML files because those were pretty much non-existent so far. Given that we can now also test scan-build (i actually landed D69781 yesterday), there are no more excuses for not writing tests. Test variable-popups.c was passing previously, so it's here just for the sake of finally having a test, while the other two tests were failing on their respective CHECK-NOT directives.

Another interesting thing i did was apply tidy-html5 to our HTML files. They seem to satisfy the linter now, and btw that's how i found the first issue (while trying to reduce the second issue by running tidy-html5 under creduce).

@Charusso: I didn't manage to understand how PopUpRanges get passed around and why, so i removed them. If you have an example of where it matters, please send tests :)

[Charusso]

Thanks for the fix! The PopUpRanges is very important, please revert it back in its original shape. Sorry for the inconvenience.

I have ran a quick scan-build with this patch on LLVM because I wanted to give you a real world example (which you cannot visibly see at the test file). Here it is:

The idea of ranges was that to add a new entry into a pop-up note as a table so you could inject any kind of pop-up information in order on the same range. Also if we highlighted the range as a pop-up do not highlight it in gray as well (which could break the HTML). Back in the days I did not realize the importance of the Doxygen and test files, I did not really know how to do so. Here I have adjusted the comments a little-bit:

+ /// Create the pop-up notes' table's start tag.
static void
HandlePopUpPieceStartTag(Rewriter &R,
                         const std::vector<SourceRange> &PopUpRanges) {
  for (const auto &Range : PopUpRanges) {
    html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "",
                         "<table class='variable_popup'><tbody>",
                         /*IsTokenRange=*/true);
  }
}

+ /// Inject a new entry into the pop-up notes' table or add the table's end tag.
static void HandlePopUpPieceEndTag(Rewriter &R,
                                   const PathDiagnosticPopUpPiece &Piece,
                                   std::vector<SourceRange> &PopUpRanges,
                                   unsigned int LastReportedPieceIndex,
                                   unsigned int PopUpPieceIndex) {
  SmallString<256> Buf;
  llvm::raw_svector_ostream Out(Buf);

  SourceRange Range(Piece.getLocation().asRange());

  // Write out the path indices with a right arrow and the message as a row.
  Out << "<tr><td valign='top'><div class='PathIndex PathIndexPopUp'>"
      << LastReportedPieceIndex;

  // Also annotate the state transition with extra indices.
  Out << '.' << PopUpPieceIndex;

  Out << "</div></td><td>" << Piece.getString() << "</td></tr>";

- // If no report made at this range mark the variable and add the end tags.
+ // If no report made at the current range \c Range we could inject the table's end tag as this is the last report on that range. Also this is the first encounter of the range, after that it is safe to insert new entries to the table.
  if (std::find(PopUpRanges.begin(), PopUpRanges.end(), Range) ==
      PopUpRanges.end()) {
    // Store that we create a report at this range.
    PopUpRanges.push_back(Range);

    Out << "</tbody></table></span>";
    html::HighlightRange(R, Range.getBegin(), Range.getEnd(),
                         "<span class='variable'>", Buf.c_str(),
                         /*IsTokenRange=*/true);
  } else {
-    // Otherwise inject just the new row at the end of the range.
+    // Otherwise we are working with multiple notes at the same range, so inject a new row to the table at the end of the range which is the beginning of the table. With that we fill the table "upwards" which is the same order as we emit reports.
    html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "", Buf.c_str(),
                         /*IsTokenRange=*/true);
  }
}

---

Please rename the variable-popups.c to variable-popups-simple.c, variable-popups-2.c to variable-popups-macro.c, and here variable-popups-multiple.c comes:

// RUN: rm -fR %t
// RUN: mkdir %t
// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN:                    -analyzer-output=html -o %t -verify %s
// RUN: cat %t/report-*.html | FileCheck %s

void bar(int);

void foo() {
  int a;
  for (unsigned i = 0; i < 3; ++i)
    if (i)
      bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
}

// CHECK:      <span class='variable'>i
// CHECK-SAME:   <table class='variable_popup'><tbody><tr>
// CHECK-SAME:     <td valign='top'>
// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>2.1</div>
// CHECK-SAME:     </td>
// CHECK-SAME:     <td>'i' is 0</td>
// CHECK-SAME:   </tr>
// CHECK-SAME:   <tr>
// CHECK-SAME:     <td valign='top'>
// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>4.1</div>
// CHECK-SAME:     </td>
// CHECK-SAME:     <td>'i' is 1</td>
// CHECK-SAME:   </tr></tbody></table>
// CHECK-SAME: </span>

[Charusso]

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

967

Here the gray highlighting goes, so the PopUpRanges store whether we have already highlighted the range and we early-continue. The cool thing is, we do not check for intersections because the pop-up range has a smaller scope than the entire expression's gray range so the HTML handles the colors/mouse-hover for us. In case of macro pop-ups the coloring and-or pop-upping was already buggy, they are handled differently, where I have not found such a cool workaround.

[NoQ]

Aha, that's what those are! Great. I thought they're only for resolving conflicts with range highlights, which seemed kinda redundant.

Addressed all comments :)

[Charusso]

Cool, thanks you!

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

612

Side note: I like the other form of De Morgan's laws because here I have to apply it in my head every time I see such a construct. Also we are using this function in negation, so I would write:

static bool isMacro(const SourceRange &Range) {
  return Range.getBegin().isMacroID() || Range.getEnd().isMacroID();
}

The idiom is to write code for readability so that understandability over everything else.

[NoQ]

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

612

Objection :) I want this function to figure out if a pop-up range should be displayed, not whether the range is in a macro. Like, if we come up with other excuses for not displaying a range, we'll update this function, not make a new one. And if we need to check whether something is a macro in another place, we'll make a new function, not re-use this one.

I could do something like shouldSkipPopUpRange() but that'd move the confusing negation into the function name, making call sites harder to read.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

if (event.key == "?") {
     return;
   }
   event.preventDefault();
 });
 </script>
 )<<<";
 }
 
+static bool shouldDisplayPopUpRange(const SourceRange &Range) {
+  return !(Range.getBegin().isMacroID() || Range.getEnd().isMacroID());
+}
+
 static void
 HandlePopUpPieceStartTag(Rewriter &R,
                          const std::vector<SourceRange> &PopUpRanges) {
   for (const auto &Range : PopUpRanges) {
+    if (!shouldDisplayPopUpRange(Range))
+      continue;
+
     html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "",
                          "<table class='variable_popup'><tbody>",
                          /*IsTokenRange=*/true);
   }
 }
 
 static void HandlePopUpPieceEndTag(Rewriter &R,
                                    const PathDiagnosticPopUpPiece &Piece,
                                    std::vector<SourceRange> &PopUpRanges,
                                    unsigned int LastReportedPieceIndex,
                                    unsigned int PopUpPieceIndex) {
   SmallString<256> Buf;
   llvm::raw_svector_ostream Out(Buf);
 
   SourceRange Range(Piece.getLocation().asRange());
+  if (!shouldDisplayPopUpRange(Range))
+    return;
 
   // Write out the path indices with a right arrow and the message as a row.
   Out << "<tr><td valign='top'><div class='PathIndex PathIndexPopUp'>"
       << LastReportedPieceIndex;
 
   // Also annotate the state transition with extra indices.
   Out << '.' << PopUpPieceIndex;
 
if (!SuppressIndex) {
     if (Kind) os << " PathIndex" << Kind;
     os << "\">" << num << "</div>";
 
     if (num > 1) {
       os << "</td><td><div class=\"PathNav\"><a href=\"#Path"
          << (num - 1)
          << "\" title=\"Previous event ("
          << (num - 1)
-         << ")\">&#x2190;</a></div></td>";
+         << ")\">&#x2190;</a></div>";
     }
 
     os << "</td><td>";
   }
 
   if (const auto *MP = dyn_cast<PathDiagnosticMacroPiece>(&P)) {
     os << "Within the expansion of the macro '";
 
void HTMLDiagnostics::HandlePiece(Rewriter &R, FileID BugFileID,
   for (const auto &Range : Ranges) {
     // If we have already highlighted the range as a pop-up there is no work.
     if (std::find(PopUpRanges.begin(), PopUpRanges.end(), Range) !=
         PopUpRanges.end())
       continue;
 
     HighlightRange(R, LPosInfo.first, Range);
   }
 }
 
 static void EmitAlphaCounter(raw_ostream &os, unsigned n) {
   unsigned x = n % ('z' - 'a');
   n /= 'z' - 'a';
 
   if (n > 0)
     EmitAlphaCounter(os, n);
 

clang/test/Analysis/html_diagnostics/td-hotfix.c

+// RUN: rm -fR %t
+// RUN: mkdir %t
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:                    -analyzer-output=html -o %t -verify %s
+// RUN: cat %t/report-*.html | FileCheck %s
+
+void bar(int);
+
+void foo() {
+  int a;
+  bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
+}
+
+// CHECK-LABEL:    <div id="EndPath" class="msg msgEvent" style="margin-left:3ex">
+// CHECK-SAME:       <table class="msgT">
+// CHECK-SAME:         <tr>
+// CHECK-SAME:           <td valign="top">
+// CHECK-SAME:             <div class="PathIndex PathIndexEvent">2</div>
+// CHECK-SAME:           </td>
+// CHECK-SAME:           <td>
+// CHECK-SAME:             <div class="PathNav">
+// CHECK-SAME:               <a href="#Path1" title="Previous event (1)">&#x2190;</a>
+// CHECK-SAME:             </div>
+// CHECK-SAME:           </td>
+// CHECK-NOT:            </td>
+// CHECK-SAME:           <td>
+// CHECK-SAME:             1st function call argument is an uninitialized value
+// CHECK-SAME:           </td>
+// CHECK-SAME:         </tr>
+// CHECK-SAME:       </table>
+// CHECK-SAME:     </div>

clang/test/Analysis/html_diagnostics/variable-popups-macro.c

+// RUN: rm -fR %t
+// RUN: mkdir %t
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:                    -analyzer-output=html -o %t -verify %s
+// RUN: cat %t/report-*.html | FileCheck %s
+
+void bar(int);
+
+#define MACRO if (b)
+
+void foo2() {
+  int a;
+  int b = 1;
+  MACRO
+    bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
+}
+
+// For now we don't emit popups inside macros due to UI limitations.
+// Once we do, we should test it thoroughly.
+
+// CHECK-LABEL: <tr class="codeline" data-linenumber="14">
+// CHECK-NOT:   <span class='variable'>
+// CHECK-SAME:  <span class='macro'>
+// CHECK-SAME:    MACRO
+// CHECK-SAME:    <span class='macro_popup'>
+// CHECK-SAME:      if (b)
+// CHECK-SAME:    </span>
+// CHECK-SAME:  </span>

clang/test/Analysis/html_diagnostics/variable-popups-multiple.c

+// RUN: rm -fR %t
+// RUN: mkdir %t
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:                    -analyzer-output=html -o %t -verify %s
+// RUN: cat %t/report-*.html | FileCheck %s
+
+void bar(int);
+
+void foo() {
+  int a;
+  for (unsigned i = 0; i < 3; ++i)
+    if (i)
+      bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
+}
+
+// CHECK:      <span class='variable'>i
+// CHECK-SAME:   <table class='variable_popup'><tbody><tr>
+// CHECK-SAME:     <td valign='top'>
+// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>2.1</div>
+// CHECK-SAME:     </td>
+// CHECK-SAME:     <td>'i' is 0</td>
+// CHECK-SAME:   </tr>
+// CHECK-SAME:   <tr>
+// CHECK-SAME:     <td valign='top'>
+// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>4.1</div>
+// CHECK-SAME:     </td>
+// CHECK-SAME:     <td>'i' is 1</td>
+// CHECK-SAME:   </tr></tbody></table>
+// CHECK-SAME: </span>

clang/test/Analysis/html_diagnostics/variable-popups-simple.c

+// RUN: rm -fR %t
+// RUN: mkdir %t
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:                    -analyzer-output=html -o %t -verify %s
+// RUN: cat %t/report-*.html | FileCheck %s
+
+void bar(int);
+
+void foo2() {
+  int a;
+  int b = 1;
+  if (b)
+    bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
+}
+
+// CHECK:      <span class='variable'>b
+// CHECK-SAME:   <table class='variable_popup'><tbody><tr>
+// CHECK-SAME:     <td valign='top'>
+// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>1.1</div>
+// CHECK-SAME:     </td>
+// CHECK-SAME:     <td>'b' is 1</td>
+// CHECK-SAME:   </tr></tbody></table>
+// CHECK-SAME: </span>