[feat] Introduce review status config file #4054

bruntib · 2023-10-23T23:07:02Z

[fix] Source code comment over review status rule

When a report is suppressed by a source code comment, then it should be
stronger than a review status rule with the same bug hash. This is
working fine when a new review status rule is set in the GUI for
existing reports. Setting a review status rule has effect on the reports
which don't have a review status provided in a source code comment.

However, if a new report is stored, then its review status is set based
on the review status rule, even if it has a source code comment. This is
fixed by this patch: the source code comment should be stronger even in
a run store process.

[NFC] ReviewStatusHandler for source code comments

Currently there are 2 ways of handling review statuses for reports:

In source code comments:
// codechecker_suppress [checker_name] message...
In the GUI:
This sets a "review status rule" which means a relationship between
bug hashes and review statuses. All reports with the same bug hash get
the same review status.

We are planning a 3rd way of setting review statuses:
A review_status.yaml config file which contains review status mappings
to files or directories.

The long-term plan is to create a ReviewStatusHandler class that handles
the review status of a report, in all three scenarios above.

This commit is just a refactoring which moves the 1st scenario-related
code (review status from source code comment) to the ReviewStatusHandler
class.

[NFC] Move source code comment handler from report-converter to codechecker_common

The class SourceCodeCommentHandler which parses in-source codechecker
suppressions and review status settings has been moved to
codechecker_common. The reason is that we'd like to gather all review
status setter methods to one place. So either Report objects handle the
logic of in-source suppressions, review status config (future feature)
and review status rules, or none of these. Since report-converter is a
standalone tool, it can't depend on CodeChecker modules (for example the
ones which determine review status rules). For this reason the source
code comment handler moves to CodeChecker.

(TODO: Unfortunately plaintext.convert() gets a ReviewStatusHandler
object as parameter, so report-converter depends on CodeChecker. This
dependency has to be eliminated later.)

[feat] Introduce review status config file

The review statuses can be provided by a yaml config file. This file can
be used for setting the review status of reports in a given directory or
a checker name.

When a report is suppressed by a source code comment, then it should be stronger than a review status rule with the same bug hash. This is working fine when a new review status rule is set in the GUI for existing reports. Setting a review status rule has effect on the reports which don't have a review status provided in a source code comment. However, if a new report is stored, then its review status is set based on the review status rule, even if it has a source code comment. This is fixed by this patch: the source code comment should be stronger even in a run store process.

Currently there are 2 ways of handling review statuses for reports: - In source code comments: // codechecker_suppress [checker_name] message... - In the GUI: This sets a "review status rule" which means a relationship between bug hashes and review statuses. All reports with the same bug hash get the same review status. We are planning a 3rd way of setting review statuses: A review_status.yaml config file which contains review status mappings to files or directories. The long-term plan is to create a ReviewStatusHandler class that handles the review status of a report, in all three scenarios above. This commit is just a refactoring which moves the 1st scenario-related code (review status from source code comment) to the ReviewStatusHandler class.

bruntib · 2023-10-23T23:11:28Z

There is a lot of work with this patch yet (like adding tests and extending documentation, etc.) However, the review process can begin. All comments are welcome. Thanks in advance!

…hecker_common The class SourceCodeCommentHandler which parses in-source codechecker suppressions and review status settings has been moved to codechecker_common. The reason is that we'd like to gather all review status setter methods to one place. So either Report objects handle the logic of in-source suppressions, review status config (future feature) and review status rules, or none of these. Since report-converter is a standalone tool, it can't depend on CodeChecker modules (for example the ones which determine review status rules). For this reason the source code comment handler moves to CodeChecker. (TODO: Unfortunately plaintext.convert() gets a ReviewStatusHandler object as parameter, so report-converter depends on CodeChecker. This dependency has to be eliminated later.)

dkrupp

When I tried this config file on tinyxml2 I got the following error:

CodeChecker analyze ./compile_commands.json --review-status-config ./review_status_config.yml -c -o reports2

cat review_status_config.yml

filepath: **
checker: misc-include-cleaner
message: Not interesting findings
review_status: intentional
filepath: /path/to/project/important/module/**
message: All reports in this module should be investigated.
review_status: confirmed

CodeChecker parse ./reports2
Traceback (most recent call last):
  File "/local/workspace/codechecker/build/CodeChecker/lib/python3/codechecker_common/cli.py", line 209, in main
    sys.exit(args.func(args))
  File "/local/workspace/codechecker/build/CodeChecker/lib/python3/codechecker_analyzer/cmd/parse.py", line 399, in main
    review_status_handler.set_review_status_config(review_status_cfg)
  File "/local/workspace/codechecker/build/CodeChecker/lib/python3/codechecker_common/review_status_handler.py", line 131, in set_review_status_config
    self.__data = yaml.safe_load(f)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/__init__.py", line 125, in safe_load
    return load(stream, SafeLoader)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/__init__.py", line 81, in load
    return loader.get_single_data()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/constructor.py", line 49, in get_single_data
    node = self.get_single_node()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 36, in get_single_node
    document = self.compose_document()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 55, in compose_document
    node = self.compose_node(None, None)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 82, in compose_node
    node = self.compose_sequence_node(anchor)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 111, in compose_sequence_node
    node.value.append(self.compose_node(node, index))
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/composer.py", line 64, in compose_node
    if self.check_event(AliasEvent):
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/parser.py", line 98, in check_event
    self.current_event = self.state()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/parser.py", line 449, in parse_block_mapping_value
    if not self.check_token(KeyToken, ValueToken, BlockEndToken):
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/scanner.py", line 116, in check_token
    self.fetch_more_tokens()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/scanner.py", line 227, in fetch_more_tokens
    return self.fetch_alias()
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/scanner.py", line 610, in fetch_alias
    self.tokens.append(self.scan_anchor(AliasToken))
  File "/local/workspace/codechecker/venv_dev/lib/python3.8/site-packages/yaml/scanner.py", line 922, in scan_anchor
    raise ScannerError("while scanning an %s" % name, start_mark,
yaml.scanner.ScannerError: while scanning an alias
  in "/local/workspace/test-projects/tinyxml2/reports2/review_status.yaml", line 1, column 13
expected alphabetic or numeric character, but found '*'
  in "/local/workspace/test-projects/tinyxml2/reports2/review_status.yaml", line 1, column 14

dkrupp · 2023-10-24T18:21:12Z

docs/analyzer/user_guide.md

+file has to represent a list of review status settings:
+
+```yaml
+- filepath: /path/to/project/test/**


Can you describe the exact blob format which can be used in this YAML file?

dkrupp · 2023-10-24T18:22:18Z

docs/analyzer/user_guide.md

+file has to represent a list of review status settings:
+
+```yaml
+- filepath: /path/to/project/test/**


I would prefer the _filter postfix to every fieald which is a filter

filepath_filter checker_filter bug_hash_filter (this is missing from these examples, but should be included if supported)

dkrupp · 2023-10-24T18:22:49Z

docs/analyzer/user_guide.md

+  setting.
+- `review_status`: The review status to set.
+
+If neither `filepath` nor `checker` is provided, then that setting is not


bug_hash isn't supported as a filter?

I added this functionality now. I called this report_has_filter, because we use the term "report hash" at other places in CodeChecker.

dkrupp · 2023-10-24T18:23:16Z

docs/analyzer/user_guide.md

+- `checker` (optional): Set the review status for only these checkers' reports.
+- `message` (optional): A comment message that describes the reason of the
+  setting.
+- `review_status`: The review status to set.


What are the possible values?

Is ignore supported?

Now I validate the possible values. ignore is not supported yet.

Please create a follow-up issue for this.

dkrupp · 2023-10-24T18:27:32Z

docs/analyzer/user_guide.md

+
+The fields of a review status settings are:
+
+- `filepath` (optional): A glob to a path where the given review status is


Should these be absolute filepaths?
If yes, would this file be transferrable between designers?

Should we use here --trim-path-prefix <project> root also for the analyze command?
or we recommend the users to use * in the prefix of the paths?
eg:
*/test_lib/src/*

Yes, the filter has to match the absolute path, even if --trim-path-prefix is used. I added this information to the documentation.

bruntib · 2023-10-25T07:46:14Z

The asterisk (*) character has a special meaning in YAML format. So, in case a filter option starts with asterisk, then it has to be quoted:

- filepath_filter: "*"
  review_status: confirmed

I added this information to the documentation.

Szelethus

I bow before the deadline. Don't hate the player, hate the game, it really is not on you. With that said, its pretty hard to review a patch of this size, but I tried my best. I mostly focused on source code reviews, I didn't thoroughly test it like @dkrupp and @vodorok did.

Szelethus · 2023-10-26T13:44:11Z

codechecker_common/util.py

@@ -84,3 +85,15 @@ def load_json(path: str, default=None, lock=False, display_warning=True):
            LOG.warning(ex)

    return ret
+
+def get_linef(fp: TextIO, line_no: int) -> str:


Is this used anywhere?

Yes, it is used in source_code_comment_handler.py.

codechecker_common/review_status_handler.py

analyzer/codechecker_analyzer/cmd/analyze.py

Szelethus · 2023-10-26T13:52:19Z

analyzer/codechecker_analyzer/cmd/analyze.py

@@ -853,6 +861,18 @@ def __update_skip_file(args):
        shutil.copyfile(args.skipfile, skip_file_to_send)


+def __update_review_status_config(args):
+    rs_config_to_send = os.path.join(args.output_path, 'review_status.yaml')


Do we ever check if the name of the file given to --review-status-config is not review_status.yaml? If we hardcode it here, we should totally do that.

We assume that the config file in the report directory is named review_status.yaml. CodeChecker ensures this. The user can provide the config file with different name after --review-status-config, but it will be copied to the report directory with this hard-coded name.

Szelethus · 2023-10-26T14:03:51Z

codechecker_common/review_status_handler.py

+                    status=status,
+                    message=message.encode('utf-8'),
+                    bug_hash=report.report_hash,
+                    in_source=True)


Why did you chose to turn get_review_status_from_source, but not the assigment to author and date?

Because ReviewStatusHandler doesn't know about users. This class is used in client-side commands too, like "CodeChecker parse". Here we don't have a user, only "CodeChecker store" provides a user name. The goal of ReviewStatusHandler is to gather the review status from different sources (e.g. source code or config file).

Szelethus · 2023-10-26T14:04:19Z

codechecker_common/review_status_handler.py

+                raise ValueError(
+                    f"Multiple source code comments can be found for "
+                    f"'{report.checker_name}' checker in '{source_file_name}' "
+                    f"at line {report.line}.")


Why do you use exceptions? Aren't exceptions supposed to be, you know, exceptional (and somewhat unforeseen) errors? The user making a source code suppression spaghetti seems anything but exception-worthy.

I couldn't find another way of communicating the type of the error. In an exception object I could describe to the user what is the problem. As a common rule-of-thumb I agree with throwing exceptions on exceptional events. However, according to my experiences in Python different kind of errors are also indicated with exceptions. This is why I chose ValueError which tells that the value of some config options is wrong.
Off topic: it would be more elegant in Go language, where it is conventional to return an error message which is handled specifically for communicating errors to the caller :)

The review statuses can be provided by a yaml config file. This file can be used for setting the review status of reports in a given directory or a checker name.

dkrupp

LGTM

bruntib added enhancement 🌟 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands config ⚙️ labels Oct 23, 2023

bruntib added this to the release 6.23.0 milestone Oct 23, 2023

bruntib requested a review from Szelethus October 23, 2023 23:07

bruntib requested review from dkrupp and vodorok as code owners October 23, 2023 23:07

bruntib added 2 commits October 24, 2023 01:07

bruntib force-pushed the review_status_handler_1_source_code branch from 03d1f17 to cf3f49b Compare October 23, 2023 23:10

bruntib force-pushed the review_status_handler_1_source_code branch 3 times, most recently from c7c6879 to 2c08477 Compare October 23, 2023 23:36

bruntib force-pushed the review_status_handler_1_source_code branch from 2c08477 to ed37831 Compare October 24, 2023 11:08

dkrupp requested changes Oct 24, 2023

View reviewed changes

bruntib force-pushed the review_status_handler_1_source_code branch from ed37831 to 4f7e3a3 Compare October 25, 2023 07:42

bruntib force-pushed the review_status_handler_1_source_code branch 2 times, most recently from 6e272ca to 5f19a73 Compare October 25, 2023 08:03

Szelethus changed the title ~~Review status handler 1 source code~~ [feat] Introduce review status config file Oct 26, 2023

bruntib force-pushed the review_status_handler_1_source_code branch from 5f19a73 to 2082a65 Compare October 26, 2023 13:56

Szelethus reviewed Oct 26, 2023

View reviewed changes

bruntib force-pushed the review_status_handler_1_source_code branch 2 times, most recently from 48c22dd to 3c6823a Compare October 26, 2023 16:03

[feat] Introduce review status config file

c040e60

The review statuses can be provided by a yaml config file. This file can be used for setting the review status of reports in a given directory or a checker name.

bruntib force-pushed the review_status_handler_1_source_code branch from 3c6823a to c040e60 Compare October 26, 2023 16:15

dkrupp approved these changes Oct 26, 2023

View reviewed changes

dkrupp merged commit afb3bd6 into Ericsson:master Oct 26, 2023
8 checks passed

bruntib deleted the review_status_handler_1_source_code branch October 28, 2023 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Introduce review status config file #4054

[feat] Introduce review status config file #4054

bruntib commented Oct 23, 2023 •

edited by Szelethus

bruntib commented Oct 23, 2023

dkrupp left a comment

dkrupp Oct 24, 2023

bruntib Oct 25, 2023

dkrupp Oct 24, 2023

bruntib Oct 25, 2023

dkrupp Oct 24, 2023

bruntib Oct 25, 2023

dkrupp Oct 24, 2023

bruntib Oct 25, 2023

dkrupp Oct 26, 2023

dkrupp Oct 24, 2023

bruntib Oct 25, 2023

bruntib commented Oct 25, 2023 •

edited

Szelethus left a comment

Szelethus Oct 26, 2023

bruntib Oct 26, 2023

Szelethus Oct 26, 2023

bruntib Oct 26, 2023

Szelethus Oct 26, 2023

bruntib Oct 26, 2023

Szelethus Oct 26, 2023

bruntib Oct 26, 2023

dkrupp left a comment


		The fields of a review status settings are:

		- `filepath` (optional): A glob to a path where the given review status is

[feat] Introduce review status config file #4054

[feat] Introduce review status config file #4054

Conversation

bruntib commented Oct 23, 2023 • edited by Szelethus

bruntib commented Oct 23, 2023

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruntib commented Oct 25, 2023 • edited

Szelethus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrupp left a comment

Choose a reason for hiding this comment

bruntib commented Oct 23, 2023 •

edited by Szelethus

bruntib commented Oct 25, 2023 •

edited