Merge pull request #6830 from github/henrymercer/report-extraction-errors-as-warnings

C++: Improve SARIF severity level reporting of extractor diagnostics

cpp/change-notes/2021-10-07-extraction-errors.md

@@ -0,0 +1,3 @@
+codescanning
+* Problems with extraction that in most cases won't break the analysis in a significant way are now reported as warnings rather than errors.
+* The failed extractor invocations query now has severity `error`.

cpp/ql/src/Diagnostics/ExtractionErrors.ql

@@ -1,16 +0,0 @@
-/**
- * @name Extraction errors
- * @description List all extraction errors for files in the source code directory.
- * @kind diagnostic
- * @id cpp/diagnostics/extraction-errors
- */
-
-import cpp
-import ExtractionErrors
-
-from ExtractionError error
-where
-  error instanceof ExtractionUnknownError or
-  exists(error.getFile().getRelativePath())
-select error, "Extraction failed in " + error.getFile() + " with error " + error.getErrorMessage(),
-  error.getSeverity()

cpp/ql/src/Diagnostics/ExtractionProblems.qll

@@ -1,12 +1,12 @@
 /**
- * Provides a common hierarchy of all types of errors that can occur during extraction.
+ * Provides a common hierarchy of all types of problems that can occur during extraction.
  */
 
 import cpp
 
 /*
  * A note about how the C/C++ extractor emits diagnostics:
- * When the extractor frontend encounters an error, it emits a diagnostic message,
+ * When the extractor frontend encounters a problem, it emits a diagnostic message,
  * that includes a message, location and severity.
  * However, that process is best-effort and may fail (e.g. due to lack of memory).
  * Thus, if the extractor emitted at least one diagnostic of severity discretionary
@@ -15,17 +15,17 @@ import cpp
  * In the common case, this means that a compilation during which one or more errors happened also gets
  * the catch-all diagnostic.
  * This diagnostic has the empty string as file path.
- * We filter out these useless diagnostics if there is at least one error-level diagnostic
+ * We filter out these useless diagnostics if there is at least one warning-level diagnostic
  * for the affected compilation in the database.
  * Otherwise, we show it to indicate that something went wrong and that we
  * don't know what exactly happened.
  */
 
 /**
- * An error that, if present, leads to a file being marked as non-successfully extracted.
+ * A problem with a file that, if present, leads to a file being marked as non-successfully extracted.
  */
-class ReportableError extends Diagnostic {
-  ReportableError() {
+class ReportableWarning extends Diagnostic {
+  ReportableWarning() {
     (
       this instanceof CompilerDiscretionaryError or
       this instanceof CompilerError or
@@ -36,39 +36,35 @@ class ReportableError extends Diagnostic {
   }
 }
 
-private newtype TExtractionError =
-  TReportableError(ReportableError err) or
+private newtype TExtractionProblem =
+  TReportableWarning(ReportableWarning err) or
   TCompilationFailed(Compilation c, File f) {
     f = c.getAFileCompiled() and not c.normalTermination()
   } or
   // Show the catch-all diagnostic (see note above) only if we haven't seen any other error-level diagnostic
   // for that compilation
-  TUnknownError(CompilerError err) {
-    not exists(ReportableError e | e.getCompilation() = err.getCompilation())
+  TUnknownProblem(CompilerError err) {
+    not exists(ReportableWarning e | e.getCompilation() = err.getCompilation())
   }
 
 /**
- * Superclass for the extraction error hierarchy.
+ * Superclass for the extraction problem hierarchy.
  */
-class ExtractionError extends TExtractionError {
-  /** Gets the string representation of the error. */
+class ExtractionProblem extends TExtractionProblem {
+  /** Gets the string representation of the problem. */
   string toString() { none() }
 
-  /** Gets the error message for this error. */
-  string getErrorMessage() { none() }
+  /** Gets the problem message for this problem. */
+  string getProblemMessage() { none() }
 
-  /** Gets the file this error occured in. */
+  /** Gets the file this problem occured in. */
   File getFile() { none() }
 
-  /** Gets the location this error occured in. */
+  /** Gets the location this problem occured in. */
   Location getLocation() { none() }
 
-  /** Gets the SARIF severity of this error. */
-  int getSeverity() {
-    // Unfortunately, we can't distinguish between errors and fatal errors in SARIF,
-    // so all errors have severity 2.
-    result = 2
-  }
+  /** Gets the SARIF severity of this problem. */
+  int getSeverity() { none() }
 }
 
 /**
@@ -79,7 +75,7 @@ class ExtractionError extends TExtractionError {
  *  - stack overflow
  *  - out of memory
  */
-class ExtractionUnrecoverableError extends ExtractionError, TCompilationFailed {
+class ExtractionUnrecoverableError extends ExtractionProblem, TCompilationFailed {
   Compilation c;
   File f;
 
@@ -89,49 +85,67 @@ class ExtractionUnrecoverableError extends ExtractionError, TCompilationFailed {
     result = "Unrecoverable extraction error while compiling " + f.toString()
   }
 
-  override string getErrorMessage() { result = "unrecoverable compilation failure." }
+  override string getProblemMessage() { result = "unrecoverable compilation failure." }
 
   override File getFile() { result = f }
 
   override Location getLocation() { result = f.getLocation() }
+
+  override int getSeverity() {
+    // These extractor errors break the analysis, so we mark them in SARIF as
+    // [errors](https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10541338).
+    result = 2
+  }
 }
 
 /**
- * A recoverable extraction error.
+ * A recoverable extraction warning.
  * These are compiler errors from the frontend.
  * Upon encountering one of these, we still continue extraction, but the
  * database will be incomplete for that file.
  */
-class ExtractionRecoverableError extends ExtractionError, TReportableError {
-  ReportableError err;
+class ExtractionRecoverableWarning extends ExtractionProblem, TReportableWarning {
+  ReportableWarning err;
 
-  ExtractionRecoverableError() { this = TReportableError(err) }
+  ExtractionRecoverableWarning() { this = TReportableWarning(err) }
 
   override string toString() { result = "Recoverable extraction error: " + err }
 
-  override string getErrorMessage() { result = err.getFullMessage() }
+  override string getProblemMessage() { result = err.getFullMessage() }
 
   override File getFile() { result = err.getFile() }
 
   override Location getLocation() { result = err.getLocation() }
+
+  override int getSeverity() {
+    // Recoverable extraction problems don't tend to break the analysis, so we mark them in SARIF as
+    // [warnings](https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10541338).
+    result = 1
+  }
 }
 
 /**
- * An unknown error happened during extraction.
- * These are only displayed if we know that we encountered an error during extraction,
+ * An unknown problem happened during extraction.
+ * These are only displayed if we know that we encountered an problem during extraction,
  * but, for some reason, failed to emit a proper diagnostic with location information
- * and error message.
+ * and problem message.
  */
-class ExtractionUnknownError extends ExtractionError, TUnknownError {
+class ExtractionUnknownProblem extends ExtractionProblem, TUnknownProblem {
   CompilerError err;
 
-  ExtractionUnknownError() { this = TUnknownError(err) }
+  ExtractionUnknownProblem() { this = TUnknownProblem(err) }
 
-  override string toString() { result = "Unknown extraction error: " + err }
+  override string toString() { result = "Unknown extraction problem: " + err }
 
-  override string getErrorMessage() { result = err.getFullMessage() }
+  override string getProblemMessage() { result = err.getFullMessage() }
 
   override File getFile() { result = err.getFile() }
 
   override Location getLocation() { result = err.getLocation() }
+
+  override int getSeverity() {
+    // Unknown extraction problems don't tend to break the analysis, so we mark them in SARIF as
+    // [warnings](https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10541338).
+    result = 1
+  }
 }

cpp/ql/src/Diagnostics/ExtractionWarnings.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Extraction warnings
+ * @description List all extraction warnings for files in the source code directory.
+ * @kind diagnostic
+ * @id cpp/diagnostics/extraction-warnings
+ */
+
+import cpp
+import ExtractionProblems
+
+from ExtractionProblem warning
+where
+  warning instanceof ExtractionRecoverableWarning and exists(warning.getFile().getRelativePath())
+  or
+  warning instanceof ExtractionUnknownProblem
+select warning,
+  "Extraction failed in " + warning.getFile() + " with warning " + warning.getProblemMessage(),
+  warning.getSeverity()

cpp/ql/src/Diagnostics/FailedExtractorInvocations.ql

@@ -13,6 +13,9 @@ string describe(Compilation c) {
   else result = "extractor invocation " + concat(int i | | c.getArgument(i), " " order by i)
 }
 
+/** Gets the SARIF severity level that indicates an error. */
+private int getErrorSeverity() { result = 2 }
+
 from Compilation c
 where not c.normalTermination()
-select "Extraction aborted for " + describe(c)
+select "Extraction aborted for " + describe(c), getErrorSeverity()

cpp/ql/src/Diagnostics/SuccessfullyExtractedFiles.ql

@@ -1,15 +1,15 @@
 /**
  * @name Successfully extracted files
- * @description Lists all files in the source code directory that were extracted without encountering an error in the file.
+ * @description Lists all files in the source code directory that were extracted without encountering a problem in the file.
  * @kind diagnostic
  * @id cpp/diagnostics/successfully-extracted-files
  */
 
 import cpp
-import ExtractionErrors
+import ExtractionProblems
 
 from File f
 where
-  not exists(ExtractionError e | e.getFile() = f) and
+  not exists(ExtractionProblem e | e.getFile() = f) and
   exists(f.getRelativePath())
-select f, ""
+select f, "File successfully extracted"