[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Abraham Corea Diaz via Phabricator via cfe-commits]

abrahamcd added a comment.

In D131084#3697211 <https://reviews.llvm.org/D131084#3697211>, @vaibhav.y wrote:

> Submitting for review:
>
> Some notes:
>
> There are a couple of ways I think we can acheive this, per the spec:
>
> 1. The reportingDescriptor (rule) objects can be given a default configuration property <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317850>, which can set the default warning level and other data such as rule parameters etc.
> 2. The reportingDescriptor objects can omit the default configuration (which then allows operating with warning as default), and the level is then set when the result is reported.
>
> The first approach would be "more correct", what are your thoughts on this? Would we benefit from having per-diagnostic configuration?
>
> There is also the question about the "kind" of results in clang. From my reading of the spec <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317647>, it seems that "fail" is the only case that applies to us because:
>
> - "pass": Implies no issue was found.
> - "open": This value is used by proof-based tools. It could also mean additional assertions required
> - "informational": The specified rule was evaluated and produced a purely informational result that does not indicate the presence of a problem
> - "notApplicable": The rule specified by ruleId was not evaluated, because it does not apply to the analysis target.
>
> Of these "open" and "notApplicable" seem to be ones that *could* come to use but I'm not sure where / what kind of diagnostics would use these. Probably clang-tidy's `bugprone-*` suite?
>
> Let me know what you think is a good way to approach this wrt clang's diagnostics system.

@cjdb, @denik, and I discussed the two approaches yesterday, and we thought the first would be the better option (add setDefaultConfig to SarifRule instead of setDiagnosticLevel in SarifResult). We could make use of the defaultConfiguration's "level", "enabled", and "rank" properties to define the different diagnostic types that are currently present in Clang, and it would consolidate that information in a single rule rather than copying it across every result related to that rule. Then we could just have a small set of defaultConfigurations that correspond to the current Clang diagnostic categories that we can choose from when making new rules.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084