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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a subscriber: Naghasan.
cjdb added a comment.

In D131084#3699223 <https://reviews.llvm.org/D131084#3699223>, @aaron.ballman wrote:

> In D131084#3697678 <https://reviews.llvm.org/D131084#3697678>, @cjdb wrote:
>
>> It seems I got confused and conflated the two this morning.
>>
>> In D131084#3697525 <https://reviews.llvm.org/D131084#3697525>, @vaibhav.y wrote:
>>
>>>> Hmm, we can probably use "informational" for notes, warnings, and remarks, but I'm kinda partial to proposing the latter two upstream.
>>>
>>> Interesting,  I agree that remarks could fit under "informational".
>>>
>>> As for notes: As I understand it they are always child diagnostics of warnings/errors, so seems it would be okay to associate these with the "fail" kind.
>>
>> I don't think classifying warnings as "fail" is right unless `-Werror` is being used. For notes, I think there may be two options, if we don't want to use informational:
>>
>> - inherit from the parent diagnostic
>> - use notApplicable, since they don't have any influence on their own
>>
>>
>>
>> In D131084#3697308 <https://reviews.llvm.org/D131084#3697308>, @denik wrote:
>>
>>> 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".
>>
>> If skipping the kind defaults to fail, then we shouldn't skip it, because getting a warning or remark doesn't necessarily mean that the build has failed.
>
> My suggestion is that we only need to worry about two kinds: `fail` for warnings and errors and `informational` for remarks. When the kind if `fail`, I'd expect we'd use the level `warning` or `error`.
>
> Notes are interesting though. Sometimes they're extra information about the diagnostic on the given line, so I was kind of expecting they'd be specified via `attachments` in that case. But they can also sometimes be used to represent code flow (like in some CFG based warnings where we're showing where something was called from), and they can also sometimes be a related location (like "declared here" notes). Perhaps we may want to expose those to SARIF in different ways depending on the situation?

I proposed relaxing the kind upstream last night. <https://github.com/oasis-tcs/sarif-spec/issues/539>, since I think that it might be good to discern when a warning isn't an error with `warning`/`informational`, because then consumers could trivially treat them differently. I think `review` is the best one for remarks, because some of them do communicate potential problems (`remark_fe_backend_optimization_remark_analysis_aliasing` is the only public one I can see that's like this, but I do recall a few from ComputeCpp too (cc @Naghasan). Perhaps these ones should instead be promoted to warnings?)

Notes that are exposed as SARIF could be `none`/`informational`, while remarks are `none`/`review`? A part of my endgame is to see notes be incorporated into their parents, but that's a long way off methinks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131084