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

[henrymercer]

The SARIF spec defines errors and warnings as follows:

• "error": A serious problem was found. The condition encountered by the tool resulted in the analysis being halted or caused the results to be incorrect or incomplete.
• "warning": A problem that is not considered serious was found. The condition encountered by the tool is such that it is uncertain whether a problem occurred, or is such that the analysis might be incomplete but the results that were generated are probably valid.

The goal of this PR is to report extraction errors that in most cases won't break the analysis in a significant way as warnings rather than errors. This helps set the right expectations when these messages appear in the diagnostic data output by the CodeQL Action and CLI.

[MathiasVP]

2. Does this need a changelog note?

I think so, yes.

[aschackmull]

I don't claim to fully understand all the moving parts here, but this looks like a serious buildup of tech debt. With this PR the distinction between errors and warnings become really blurred and confusing - extractors distinguish these two categories, but then they're suddenly mixed and matched in the QL. It feels like there are some underlying questions that needs to be answered:

1. Do extractors emit certain types of errors that would be more reasonable as warnings? If so, then the changes should likely be in the extractors rather than ad-hoc translations in the QL.
2. Are all errors emitted by the extractors mostly harmless? And are they at the same time more serious than the current warnings? Then should we perhaps have a different severity level in-between? I.e. are the current set of severity levels sufficient?

[henrymercer]

Hi @aschackmull, thank you for your comment and I agree with your concerns. I think most of your comments are best addressed by language teams, however for the following:

2. Then should we perhaps have a different severity level in-between? I.e. are the current set of severity levels sufficient?

I'll note that it would be beneficial to stick with the severity levels that are part of the SARIF spec to avoid the need for consumers of our results to introduce custom support to process the severities of diagnostic messages.

The motivation for this change is somewhat of a stopgap solution to address customer concerns. I think the kind of changes that you're proposing to extractors make sense conceptually, but they need to be owned by the language teams, and they will take more time to implement.

A proposal that could address customer concerns quickly while addressing some of the problems you identified with the current state of the PR is to reduce the SARIF level of each extractor diagnostic. For example, extractor errors would be mapped to the warning SARIF level, and extractor warnings would be mapped to the note SARIF level. This perhaps gives a more appropriate semantics to extractor diagnostics (extractor errors are often not serious problems that break the analysis in a significant way), while also preserving the distinction between errors and warnings.

@aschackmull @yo-h @turbo @AlonaHlobina @adityasharad @calumgrant @jbj What are your thoughts?
@aschackmull's proposal seems to me the more sensible long term solution, however do language teams have the capacity to pick it up?

[jbj]

For @aschackmull's questions, I'm guessing this is highly language-specific. For C++, I'd say that the extractor errors reported by ExtractionErrors.ql should be seen as warnings from a user perspective, and so the proposed change looks good to me. Just yesterday we had a support escalation caused by a customer misunderstanding the severity.

We have another diagnostic query, FailedExtractorInvocations.ql, for the cases where the extractor aborts completely. It turns out this query has no severity column, and I think "error" would be appropriate for this query.

[jbj]

Today, the diagnostic summary for C/C++ looks like this:

| Severity |                           Message                            |
+----------+--------------------------------------------------------------+
| error    | (313 results for diagnostic "Extraction errors")             |
| none     | (84 results for diagnostic "Failed extractor invocations")   |
| none     | (2487 results for diagnostic "Successfully extracted files") |

I'd like the three severities to instead be warning, error, none (in order).

[aschackmull]

For C++, I'd say that the extractor errors reported by ExtractionErrors.ql should be seen as warnings from a user perspective, and so the proposed change looks good to me

Then at the very least shouldn't some files/queries/predicates be renamed as well? Having a query named ExtractionErrors.ql emit warnings instead of errors seems quite confusing to me.

[RasmusWL]

From a user perspective I think it could make sense to treat extractor errors as warnings for Python. (they can't really do anything about it if the problem is on our parser). However, I would like to discuss this with the rest of the Python team, which we will do on Monday afternoon. Leaving a blocked review until then.

[igfoo]

Do you mean you want

| warning | (313 results for diagnostic "Extraction errors")             |

@jbj? I'm not sure how much having only 1, rather than 2, instances of "error" would have helped.

[jbj]

Together with renaming files, predicates and metadata as suggested in #6830 (comment), I think the table should become

| Severity |                           Message                            |
+----------+--------------------------------------------------------------+
| error    | (84 results for diagnostic "Failed extractor invocations")   |
| warning  | (313 results for diagnostic "Extraction warnings")           |
| none     | (2487 results for diagnostic "Successfully extracted files") |

Maybe none should become note, depending on how that's described in the SARIF spec.

[henrymercer]

Copying from the SARIF spec:

• "error": A serious problem was found. The condition encountered by the tool resulted in the analysis being halted or caused the results to be incorrect or incomplete.
• "warning": A problem that is not considered serious was found. The condition encountered by the tool is such that it is uncertain whether a problem occurred, or is such that the analysis might be incomplete but the results that were generated are probably valid.
• "note": The notification is purely informational. There is no required action.
• "none": This is a trace notification (typically, debug output from the tool).

Given the quantity of results it produces, I would weakly suggest that the "Successfully extracted files" diagnostic seems like debug output and therefore should have severity none. I don't have a strong opinion against using note though.

[AlonaHlobina]

Given the quantity of results it produces, I would weakly suggest that the "Successfully extracted files" diagnostic seems like debug output and therefore should have severity none. I don't have a strong opinion against using note though.

I tend to agree with @henrymercer. From the customer's perspective, it will be less confusing. We do not necessarily want them to fix these errors. In many cases, it is not even possible for customers to do something about them. Reducing the criticality of the message we send here will help to manage the expectations.

[henrymercer]

Thanks @jbj @aschackmull @RasmusWL @igfoo @AlonaHlobina for your input. There's a clear way forward for C++, so I'm going to retarget this PR to just C++.

For the other languages, it's great to see that the discussion has started on this. I'll hand over making any adjustments to the severity of the extractor diagnostics to you, and let this PR serve as an example for how we made these adjustments for one language.

[henrymercer]

I'm looking for some databases I can use to test the changes I've made to the C++ extractor diagnostics, since there don't appear to be any QL tests for these queries. @criemen I believe you implemented the bulk of these queries in #5414 — do you have any databases you used for testing that you could send me? Thanks!

[adityasharad]

I'm looking for some databases I can use to test the changes I've made to the C++ extractor diagnostics, since there don't appear to be any QL tests for these queries. @criemen I believe you implemented the bulk of these queries in #5414 — do you have any databases you used for testing that you could send me? Thanks!

Try a DCA run on the default repos there?

[criemen]

@henrymercer There's integration tests for these queries I believe? I don't have any DBs handy, sorry.

[henrymercer]

I had just noticed the CI run and was about to comment — thanks! I'll look into this next week.

This PR no longer changes Python

[henrymercer]

Checks failure is unrelated.

[jbj]

Thanks! LGTM.

[jbj]

I'd like to merge this PR (and its corresponding internal PR), but I'm not allowed to click the button when Checks doesn't pass. I can't see what the error is. I'll try to re-run it.

[henrymercer]

@adityasharad (or another admin) please could you merge this along with the corresponding internal PR? I'm not able to due to the Checks failure, which is unrelated. Thanks!

[henrymercer]

@jbj It's a bug which occurs when we trigger the checks job via Qlucie so it uses a custom branch of our internal code. I've linked the internal issue with the bug report above. I don't think the rerun will help, but will be glad to be proven wrong :)

[adityasharad]

Internal PR has passed all checks.