[cmd] parse extends sarif output with extra fields - #4925
Conversation
8d36e16 to
e3ec728
Compare
dkrupp
left a comment
dkrupp
left a comment
There was a problem hiding this comment.
The implementation works nicely, Please check my comments related to the default filters.
| self.__log_and_analyze() | ||
|
|
||
| parse_sarif_cmd = [self._codechecker_cmd, "parse", self.report_dir, | ||
| "-e", "sarif", "--review-status", "false_positive"] |
There was a problem hiding this comment.
For sarif export I think the convenient default would be to export everything into sarif and not just the ['confirmed', 'unreviewed'] reports. For standard command line outptut and possible from html, the current default is meaningful (you dont want to see dismissed reports).
Can you please add a filter option "everything" and make that as the default for sarif export?
This way the users would not need to list all review status possibilities at command line invocation if they want to export everything.
There was a problem hiding this comment.
This will be fixed in a followup TR
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
The tests cases were not executed for some reason. Can you try force pushing a commit that would trigger the CI?
| a.o: a.cpp | ||
| $(CXX) -c a.cpp -o /dev/null | ||
|
|
||
| clean: |
There was a problem hiding this comment.
This clean target is pointless. On the line above, the output of the compilation is sent to /dev/null.
There was a problem hiding this comment.
analyzer/tests/functional/parse_status does the same, let's patch them together if we think this extra foolproofness is not necessary.
There was a problem hiding this comment.
I think we shouldn't commit code to the repository that would be eventually removed anyway.
Cleaning up analyzer/tests/functional/parse_status should be a separate task, unreleated to this change.
|
|
||
| severity = checker_labels.severity( | ||
| checker_name, | ||
| report.analyzer_name) # type: ignore[call-arg] |
There was a problem hiding this comment.
Instead of ignoring type warnings, the current type stub should be updated in file tools/report-converter/codechecker_report_converter/report/checker_labels.py.
There was a problem hiding this comment.
Adding an Optional parameter to the stub does not satisfy mypy for some reason. It errors with "Too many|few arguments" either way. Note that the other method in the stub has the same problem in the first place, but label_of_checker is called with the same number of arguments everywhere (when typed with this stub at least), so the problem has not surfaced yet (if you try it, mypy complains right away).
There was a problem hiding this comment.
The problem is that you cannot specify the default None value for Optional arguments in type hints, and therefore this was an incorrect stub for the actual CheckerLabels implementation in codechecker_common. The only solution seems to be to provide placeholder implementations for the methods, instead of mere Callable hints.
| build_json = os.path.join(self.test_workspace, "build.json") | ||
|
|
||
| clean_cmd = ["make", "clean"] | ||
| out = subprocess.check_output(clean_cmd, |
There was a problem hiding this comment.
In this case, why not reuse the existing __run_cmd function?
| if process.returncode != (2 if "parse" in cmd else 0): | ||
| return err | ||
|
|
||
| return ''.join(out) |
There was a problem hiding this comment.
out is already a string, consider:
| return ''.join(out) | |
| return out |
| a.o: a.cpp | ||
| $(CXX) -c a.cpp -o /dev/null | ||
|
|
||
| clean: |
There was a problem hiding this comment.
I think we shouldn't commit code to the repository that would be eventually removed anyway.
Cleaning up analyzer/tests/functional/parse_status should be a separate task, unreleated to this change.
barnabasdomozi
left a comment
barnabasdomozi
left a comment
There was a problem hiding this comment.
Currently, some of the test cases are failing in the CI.
0d1620c to
0474abd
Compare
331259f to
fc2c5dd
Compare
fc2c5dd to
a2de619
Compare
a2de619 to
cb88192
Compare
|
Please rebase to master, the linter error is already fixed on it. |
cb88192 to
d07c26a
Compare
Still not okay. |
|
@andyg319 I believe its fixed now. Try to rebase again. |
d07c26a to
7af842b
Compare
3 participants
There are a few fields in the SARIF specification which would be meaningful to be filled out, but today are not generated into the exported files.
CodeChecker parsealready populates Report objects with review status information, but currently only JSON export makes use of it. This PR addssuppressionobjects to individual results in the SARIF file, according to the spec, including suppression source (source code comment or review config file) and justification message.location.physicalLocationmay be added in a follow-up PR.SARIF
rules[](~checkers) are extended withdefaultConfiguration.levelandhelpUrifields (spec), corresponding to theseverityanddoc_urlchecker labels in CodeChecker config.Note that this only applies to the
CodeChecker parsecommand at the moment, which has the analyzer context (including aCheckerLabelobject) readily available. Doing the same for the standalone report-converter is an orthogonal task, and might entail bigger refactorings.