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

Wed Aug 3 12:22:08 PDT 2022

denik added a comment.

In D131084#3697256 <https://reviews.llvm.org/D131084#3697256>, @cjdb wrote:

> 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.
>
> Hmm, we can probably use "informational" for notes, warnings, and remarks, but I'm kinda partial to proposing the latter two upstream.

I think we can skip `kind` in the clang diagnostic. I found this:

> If kind is absent, it SHALL default to "fail".
> If level has any value other than "none" and kind is present, then kind SHALL have the value "fail".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131084