Update to SARIF 11-28

[aaron.ballman]

Summary

This updates our SARIF support from 10-10 to 11-28. Functional changes include:

• The run.files property is now an array instead of a mapping.
• fileLocation objects now have a fileIndex property specifying the array index into run.files.
• The resource.rules property is now an array instead of a mapping.
• The result object was given a ruleIndex property that is an index into the resource.rules array.
• rule objects now have their "id" field filled out in addition to the name field.
• Updated the schema and spec version numbers to 11-28.

(Note, the SARIF viewer plugins for Visual Studio and VSCode have not caught up to 11-28 yet, but are expected to be updated.)

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

218

Ignore this line, it is debugging cruft and will be removed.

[NoQ]

Yup, looks good, and i keep passively cheering for this project to be successful.

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

129

This sounds like find_if to me.

143

I suspect that the LHS of || implies the RHS of || and is therefore redundant.

289

Aha, ok, so now instead of an alphabetical order we have the order in which reports of the respective checkers are squeezed into the consumer. Which is probably deterministic, so it's fine. I just enjoy talking to myself on phabricator sometimes.

318

I wonder why we didn't mark Diags as const & in this callback.

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

129

The problem is: we need the Index. It felt a bit weird to have llvm::find_if() (a const operation) also mutating a locally-captured variable. WDYT?

143

Nope, this is needed for a boundary condition. In the case where Files is empty, we don't loop over anything in the range-based for loop, and so Index is zero, which means Index == Files.size(). On the other end of that boundary, if Files is nonempty but FE hasn't been added to it yet, you'll loop over the entire list of Files in the range-based for loop, which also triggers Index == Files.size().

289

Yes, this should be deterministic -- good thought to check!

318

It would be a nice refactoring for someday. :-)

[aaron.ballman]

Committed in r349188.

(@NoQ -- if you'd like to see a switch to llvm::find_if(), I'm happy to handle it post-commit.)

[NoQ]

Sure, np.

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

129

The way i see it:

auto I = std::find_if(Files.begin(), Files.end(), [&](const json::Value &File {
  if (const json::Object *Obj = File.getAsObject())
    if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
      Optional<StringRef> URI = FileLoc->getString("uri");
      return URI && URI->equals(FileURI);
    }
  return false;
});

if (I == Files.end())
  Files.push_back(createFile(FE));

return std::distance(Files.begin(), I);

Or pre-compute the index before push_back if the container invalidates iterators upon push_back.

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

129

I dig it! Implemented in r349197.

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 
 using namespace llvm;
 using namespace clang;
 using namespace ento;
 
 namespace {

 static json::Object createFile(const FileEntry &FE) {
   return json::Object{{"fileLocation", createFileLocation(FE)},
                       {"roles", json::Array{"resultFile"}},
                       {"length", FE.getSize()},
                       {"mimeType", "text/plain"}};
 }
 
 static json::Object createFileLocation(const FileEntry &FE,
-                                       json::Object &Files) {
+                                       json::Array &Files) {
   std::string FileURI = fileNameToURI(getFileName(FE));
-  if (!Files.get(FileURI))
-    Files[FileURI] = createFile(FE);
 
-  return json::Object{{"uri", FileURI}};
+  // See if the Files array contains this URI already. If it does not, create
+  // a new file object to add to the array. Calculate the index within the file
+  // location array so it can be stored in the JSON object.
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+    if (const json::Object *Obj = File.getAsObject()) {
+      if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
+        Optional<StringRef> URI = FileLoc->getString("uri");
+        if (URI && URI->equals(FileURI))
+          break;
+      }
+    }
+    ++Index;
+  }
+
+  // If we reached the end of the array, then add the file to the list of files
+  // we're tracking; Index then points to the last element of the array. Note
+  // that an empty file lists always causes a file to be added.
+  if (Files.empty() || Index == Files.size())
+    Files.push_back(createFile(FE));
+
+  return json::Object{{"uri", FileURI}, {"fileIndex", Index}};
 }
 
 static json::Object createTextRegion(SourceRange R, const SourceManager &SM) {
   return json::Object{
       {"startLine", SM.getExpansionLineNumber(R.getBegin())},
       {"endLine", SM.getExpansionLineNumber(R.getEnd())},
       {"startColumn", SM.getExpansionColumnNumber(R.getBegin())},
       {"endColumn", SM.getExpansionColumnNumber(R.getEnd())}};
 }
 
 static json::Object createPhysicalLocation(SourceRange R, const FileEntry &FE,
                                            const SourceManager &SMgr,
-                                           json::Object &Files) {
+                                           json::Array &Files) {
   return json::Object{{{"fileLocation", createFileLocation(FE, Files)},
                        {"region", createTextRegion(R, SMgr)}}};
 }
 
 enum class Importance { Important, Essential, Unimportant };
 
 static StringRef importanceToStr(Importance I) {
   switch (I) {
return Piece.getTagStr() == "ConditionBRVisitor" ? Importance::Important
                                                      : Importance::Essential;
   case PathDiagnosticPiece::Kind::ControlFlow:
     return Importance::Unimportant;
   }
   return Importance::Unimportant;
 }
 
 static json::Object createThreadFlow(const PathPieces &Pieces,
-                                     json::Object &Files) {
+                                     json::Array &Files) {
   const SourceManager &SMgr = Pieces.front()->getLocation().getManager();
   json::Array Locations;
   for (const auto &Piece : Pieces) {
     const PathDiagnosticLocation &P = Piece->getLocation();
+    const auto *CP = dyn_cast<PathDiagnosticCallPiece>(Piece.get());
     Locations.push_back(createThreadFlowLocation(
         createLocation(createPhysicalLocation(P.asRange(),
                                               *P.asLocation().getFileEntry(),
                                               SMgr, Files),
                        Piece->getString()),
         calculateImportance(*Piece)));
   }
   return json::Object{{"locations", std::move(Locations)}};
 }
 
 static json::Object createCodeFlow(const PathPieces &Pieces,
-                                   json::Object &Files) {
+                                   json::Array &Files) {
   return json::Object{
       {"threadFlows", json::Array{createThreadFlow(Pieces, Files)}}};
 }
 
 static json::Object createTool() {
   return json::Object{{"name", "clang"},
                       {"fullName", "clang static analyzer"},
                       {"language", "en-US"},
                       {"version", getClangFullVersion()}};
 }
 
-static json::Object createResult(const PathDiagnostic &Diag,
-                                 json::Object &Files) {
+static json::Object createResult(const PathDiagnostic &Diag, json::Array &Files,
+                                 const StringMap<unsigned> &RuleMapping) {
   const PathPieces &Path = Diag.path.flatten(false);
   const SourceManager &SMgr = Path.front()->getLocation().getManager();
 
+  auto Iter = RuleMapping.find(Diag.getCheckName());
+  assert(Iter != RuleMapping.end() && "Rule ID is not in the array index map?");
+
   return json::Object{
       {"message", createMessage(Diag.getVerboseDescription())},
       {"codeFlows", json::Array{createCodeFlow(Path, Files)}},
       {"locations",
        json::Array{createLocation(createPhysicalLocation(
            Diag.getLocation().asRange(),
            *Diag.getLocation().asLocation().getFileEntry(), SMgr, Files))}},
+      {"ruleIndex", Iter->getValue()},
       {"ruleId", Diag.getCheckName()}};
 }
 
 static StringRef getRuleDescription(StringRef CheckName) {
   return llvm::StringSwitch<StringRef>(CheckName)
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT)                                     \
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER
 #undef GET_CHECKERS
       ;
 }
 
 static json::Object createRule(const PathDiagnostic &Diag) {
   StringRef CheckName = Diag.getCheckName();
   return json::Object{
       {"fullDescription", createMessage(getRuleDescription(CheckName))},
-      {"name", createMessage(CheckName)}};
+      {"name", createMessage(CheckName)},
+      {"id", CheckName}};
 }
 
-static json::Object createRules(std::vector<const PathDiagnostic *> &Diags) {
-  json::Object Rules;
+static json::Array createRules(std::vector<const PathDiagnostic *> &Diags,
+                               StringMap<unsigned> &RuleMapping) {
+  json::Array Rules;
   llvm::StringSet<> Seen;
 
   llvm::for_each(Diags, [&](const PathDiagnostic *D) {
     StringRef RuleID = D->getCheckName();
     std::pair<llvm::StringSet<>::iterator, bool> P = Seen.insert(RuleID);
-    if (P.second)
-      Rules[RuleID] = createRule(*D);
+    if (P.second) {
+      RuleMapping[RuleID] = Rules.size(); // Maps RuleID to an Array Index.
+      Rules.push_back(createRule(*D));
+    }
   });
 
   return Rules;
 }
 
-static json::Object
-createResources(std::vector<const PathDiagnostic *> &Diags) {
-  return json::Object{{"rules", createRules(Diags)}};
+static json::Object createResources(std::vector<const PathDiagnostic *> &Diags,
+                                    StringMap<unsigned> &RuleMapping) {
+  return json::Object{{"rules", createRules(Diags, RuleMapping)}};
 }
 
 static json::Object createRun(std::vector<const PathDiagnostic *> &Diags) {
-  json::Array Results;
-  json::Object Files;
+  json::Array Results, Files;
+  StringMap<unsigned> RuleMapping;
+  json::Object Resources = createResources(Diags, RuleMapping);
   
   llvm::for_each(Diags, [&](const PathDiagnostic *D) {
-    Results.push_back(createResult(*D, Files));
+    Results.push_back(createResult(*D, Files, RuleMapping));
   });
 
   return json::Object{{"tool", createTool()},
-                      {"resources", createResources(Diags)},
+                      {"resources", std::move(Resources)},
                       {"results", std::move(Results)},
                       {"files", std::move(Files)}};
 }
 
 void SarifDiagnostics::FlushDiagnosticsImpl(
     std::vector<const PathDiagnostic *> &Diags, FilesMade *) {
   // We currently overwrite the file if it already exists. However, it may be
   // useful to add a feature someday that allows the user to append a run to an
   // existing SARIF file. One danger from that approach is that the size of the
   // file can become large very quickly, so decoding into JSON to append a run
   // may be an expensive operation.
   std::error_code EC;
   llvm::raw_fd_ostream OS(OutputFile, EC, llvm::sys::fs::F_Text);
   if (EC) {
     llvm::errs() << "warning: could not create file: " << EC.message() << '\n';
     return;
   }
   json::Object Sarif{
       {"$schema",
-       "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-10-10"},
-      {"version", "2.0.0-csd.2.beta.2018-10-10"},
+       "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28"},
+      {"version", "2.0.0-csd.2.beta.2018-11-28"},
       {"runs", json::Array{createRun(Diags)}}};
   OS << llvm::formatv("{0:2}", json::Value(std::move(Sarif)));
 }

test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif

 {
-  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-10-10",
+  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",
   "runs": [
     {
-      "files": {
-        "file:sarif-diagnostics-taint-test.c": {
+      "files": [
+        {
           "fileLocation": {
             "uri": "file:sarif-diagnostics-taint-test.c"
           },
           "length": 413,
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
-      },
+      ],
       "resources": {
-        "rules": {
-          "debug.TaintTest": {
+        "rules": [
+          {
             "fullDescription": {
               "text": "Mark tainted symbols as such."
             },
+            "id": "debug.TaintTest",
             "name": {
               "text": "debug.TaintTest"
             }
           }
-        }
+        ]
       },      
       "results": [
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Calling 'f'"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-diagnostics-taint-test.c"
                           },
                           "region": {
                             "endColumn": 5,
                             "endLine": 13,
                             "startColumn": 3,
                             "startLine": 13
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "tainted"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-diagnostics-taint-test.c"
                           },
                           "region": {
                             "endColumn": 17,
                             "endLine": 9,
                             "startColumn": 11,
                             "startLine": 9
                           }
                         }
                       }
                     }
                   ]
                 }
               ]
             }
           ],
           "locations": [
             {
               "physicalLocation": {
                 "fileLocation": {
+                  "fileIndex": 0,
                   "uri": "file:sarif-diagnostics-taint-test.c"
                 },
                 "region": {
                   "endColumn": 17,
                   "endLine": 9,
                   "startColumn": 11,
                   "startLine": 9
                 }
               }
             }
           ],
           "message": {
             "text": "tainted"
           },
-          "ruleId": "debug.TaintTest"
+          "ruleId": "debug.TaintTest",
+          "ruleIndex": 0
         }
       ],
       "tool": {
         "fullName": "clang static analyzer",
         "language": "en-US",
         "name": "clang",
         "version": "clang version 8.0.0 (https://github.com/llvm-project/clang.git a5ccb257a7a70928ede717a7c282f5fc8cbed310) (https://github.com/llvm-mirror/llvm.git 73cebd79c512f7129eca16b0f3a7abd21d2881e8)"
       }
     }
   ],
-  "version": "2.0.0-csd.2.beta.2018-10-10"
+  "version": "2.0.0-csd.2.beta.2018-11-28"
 }

test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif

 {
-  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-10-10",
+  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",
   "runs": [
     {
-      "files": {
-        "file:sarif-multi-diagnostic-test.c": {
+      "files": [      
+        {
           "fileLocation": {
             "uri": "file:sarif-multi-diagnostic-test.c"
           },
           "length": 665,
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
-      },
+      ],
       "resources": {
-        "rules": {
-          "core.CallAndMessage": {
+        "rules": [
+          {
             "fullDescription": {
-              "text": "Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)"
+              "text": "Mark tainted symbols as such."
             },
+            "id": "debug.TaintTest",
             "name": {
-              "text": "core.CallAndMessage"
+              "text": "debug.TaintTest"
             }
-          },
-          "core.DivideZero": {
+          },        
+          {
             "fullDescription": {
-              "text": "Check for division by zero"
+              "text": "Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)"
             },
+            "id": "core.CallAndMessage",
             "name": {
-              "text": "core.DivideZero"
+              "text": "core.CallAndMessage"
             }
           },
-          "debug.TaintTest": {
+          {
             "fullDescription": {
-              "text": "Mark tainted symbols as such."
+              "text": "Check for division by zero"
             },
+            "id": "core.DivideZero",
             "name": {
-              "text": "debug.TaintTest"
-            }
+              "text": "core.DivideZero"
             }
           }
+        ]
       },
       "results": [
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Calling 'f'"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 5,
                             "endLine": 24,
                             "startColumn": 3,
                             "startLine": 24
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "tainted"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 17,
                             "endLine": 9,
                             "startColumn": 11,
                             "startLine": 9
                           }
                         }
                       }
                     }
                   ]
                 }
               ]
             }
           ],
           "locations": [
             {
               "physicalLocation": {
                 "fileLocation": {
+                  "fileIndex": 0,
                   "uri": "file:sarif-multi-diagnostic-test.c"
                 },
                 "region": {
                   "endColumn": 17,
                   "endLine": 9,
                   "startColumn": 11,
                   "startLine": 9
                 }
               }
             }
           ],
           "message": {
             "text": "tainted"
           },
-          "ruleId": "debug.TaintTest"
+          "ruleId": "debug.TaintTest",
+          "ruleIndex": 0
         },
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Calling 'g'"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 5,
                             "endLine": 25,
                             "startColumn": 3,
                             "startLine": 25
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "'fp' declared without an initial value"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 10,
                             "endLine": 13,
                             "startColumn": 3,
                             "startLine": 13
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Called function pointer is an uninitialized pointer value"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 8,
                             "endLine": 14,
                             "startColumn": 3,
                             "startLine": 14
                           }
                         }
                       }
                     }
                   ]
                 }
               ]
             }
           ],
           "locations": [
             {
               "physicalLocation": {
                 "fileLocation": {
+                  "fileIndex": 0,
                   "uri": "file:sarif-multi-diagnostic-test.c"
                 },
                 "region": {
                   "endColumn": 8,
                   "endLine": 14,
                   "startColumn": 3,
                   "startLine": 14
                 }
               }
             }
           ],
           "message": {
             "text": "Called function pointer is an uninitialized pointer value"
           },
-          "ruleId": "core.CallAndMessage"
+          "ruleId": "core.CallAndMessage",
+          "ruleIndex": 1
         },
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {
                       "importance": "important",
                       "location": {
                         "message": {
                           "text": "Assuming 'i' is equal to 0"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 12,
                             "endLine": 18,
                             "startColumn": 7,
                             "startLine": 18
                           }
                         }
                       }
                     },
                     {
                       "importance": "unimportant",
                       "location": {
                         "message": {
                           "text": "Taking true branch"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 3,
                             "endLine": 18,
                             "startColumn": 3,
                             "startLine": 18
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Division by zero"
                         },
                         "physicalLocation": {
                           "fileLocation": {
+                            "fileIndex": 0,
                             "uri": "file:sarif-multi-diagnostic-test.c"
                           },
                           "region": {
                             "endColumn": 14,
                             "endLine": 19,
                             "startColumn": 14,
                             "startLine": 19
                           }
                         }
                       }
                     }
                   ]
                 }
               ]
             }
           ],
           "locations": [
             {
               "physicalLocation": {
                 "fileLocation": {
+                  "fileIndex": 0,
                   "uri": "file:sarif-multi-diagnostic-test.c"
                 },
                 "region": {
                   "endColumn": 14,
                   "endLine": 19,
                   "startColumn": 14,
                   "startLine": 19
                 }
               }
             }
           ],
           "message": {
             "text": "Division by zero"
           },
-          "ruleId": "core.DivideZero"
+          "ruleId": "core.DivideZero",
+          "ruleIndex": 2
         }
       ],
       "tool": {
         "fullName": "clang static analyzer",
         "language": "en-US",
         "name": "clang",
         "version": "clang version 8.0.0 (trunk 345822) (llvm/trunk 345824)"
       }
     }
   ],
-  "version": "2.0.0-csd.2.beta.2018-10-10"
+  "version": "2.0.0-csd.2.beta.2018-11-28"
 }