Skip to content

Commit

Permalink
[feat] Change review status config YAML format
Browse files Browse the repository at this point in the history 
The format of the review status config is changed. A version number is
also included to identify the format in case further modifications are
needed in the future.
  • Loading branch information
@bruntib
bruntib committed Dec 8, 2023
1 parent 5a07f37 commit 2c6a1d6
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 93 deletions.
9 changes: 7 additions & 2 deletions analyzer/codechecker_analyzer/cmd/analyze.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -985,8 +985,13 @@ def main(args):
# Process the skip list if present.
skip_handlers = __get_skip_handlers(args, compile_commands)
rs_handler = review_status_handler.ReviewStatusHandler(args.output_path)
if 'review_status_config' in args:
rs_handler.set_review_status_config(args.review_status_config)

try:
if 'review_status_config' in args:
rs_handler.set_review_status_config(args.review_status_config)
except ValueError as ex:
LOG.error(ex)
sys.exit(1)

ctu_or_stats_enabled = False
# Skip list is applied only in pre-analysis
Expand Down
146 changes: 87 additions & 59 deletions codechecker_common/review_status_handler.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,14 @@ class ReviewStatusHandler:
'confirmed',
'intentional']

ALLOWED_FIELDS = [
'filepath_filter',
'checker_filter',
'report_hash_filter',
ALLOWED_FILTERS = [
'filepath',
'checker_name',
'report_hash']

ALLOWED_ACTIONS = [
'review_status',
'message',
'reason',
'ignore']

def __init__(self, source_root=''):
Expand Down Expand Up @@ -84,64 +86,92 @@ def __parse_codechecker_review_comment(

return src_comment_data

def __validate_review_status_yaml_data(self):
"""
This function validates the data read from review_status.yaml file and
raises ValueError with the description of the error if the format is
invalid.
"""
if not isinstance(self.__data, list):
def __check_format_version_1(self):
if 'rules' not in self.__data or \
not isinstance(self.__data['rules'], list):
raise ValueError(
f"{self.__review_status_yaml} should be a list of review "
"status descriptor objects.")
f"{self.__review_status_yaml} should contain the key 'rules' "
f"with a non-empty list of (filters, actions) pair.")

for item in self.__data:
if not isinstance(item, dict):
for rule in self.__data['rules']:
if not isinstance(rule, dict) or \
'filters' not in rule or \
'actions' not in rule:
raise ValueError(
f"Format error in {self.__review_status_yaml}: {item} "
"should be a review status descriptor object.")
f"Format error in {self.__review_status_yaml}. The "
f"following section should be an object with keys "
f"'filters' and 'actions':\n"
f"{yaml.dump(rule)}")

for field in rule['filters']:
if field not in ReviewStatusHandler.ALLOWED_FILTERS:
raise ValueError(
f"Format error in {self.__review_status_yaml}: filter "
f"'{field}' is not allowed. Available filters are: "
f"{', '.join(ReviewStatusHandler.ALLOWED_FILTERS)}")

for field in item:
if field not in ReviewStatusHandler.ALLOWED_FIELDS:
for field in rule['actions']:
if field not in ReviewStatusHandler.ALLOWED_ACTIONS:
raise ValueError(
f"Format error in {self.__review_status_yaml}: field "
f"'{field}' is not allowed. Available fields are: "
f"{', '.join(ReviewStatusHandler.ALLOWED_FIELDS)}")
f"Format error in {self.__review_status_yaml}: action "
f"'{field}' is not allowed. Available actions are: "
f"{', '.join(ReviewStatusHandler.ALLOWED_ACTIONS)}")

if 'review_status' not in item and 'ignore' not in item:
if 'review_status' not in rule['actions'] and \
'ignore' not in rule['actions']:
raise ValueError(
f"Format error in {self.__review_status_yaml}: "
f"'review_status' or 'ignore' is required in {item}.")
f"Format error in {self.__review_status_yaml}. "
f"'review_status' or 'ignore' is required in:\n"
f"{yaml.dump(rule)}.")

if 'review_status' in item:
if 'review_status' in rule['actions']:
review_status_options = \
', '.join(ReviewStatusHandler.REVIEW_STATUS_OPTIONS)

if item['review_status'] \
if rule['actions']['review_status'] \
not in ReviewStatusHandler.REVIEW_STATUS_OPTIONS:
raise ValueError(
f"Invalid review status field: "
f"{item['review_status']} at {item} in "
f"{rule['actions']['review_status']} at {rule} in "
f"{self.__review_status_yaml}. Available options are: "
f"{review_status_options}.")

if item['review_status'] == 'suppress':
item['review_status'] = 'false_positive'
if rule['actions']['review_status'] == 'suppress':
rule['actions']['review_status'] = 'false_positive'

def __validate_review_status_yaml_data(self):
"""
This function validates the data read from review_status.yaml file and
raises ValueError with the description of the error if the format is
invalid.
"""
if not isinstance(self.__data, dict) or '$version' not in self.__data:
raise ValueError(
f"{self.__review_status_yaml} should be a dictionary with "
"the key '$version'.")

if not isinstance(self.__data['$version'], int):
raise ValueError(
f"{self.__review_status_yaml} should have an integer value "
"for the key '$version'.")

if self.__data['$version'] == 1:
self.__check_format_version_1()

def __report_matches_item(self, report: Report, item: dict):
if 'filepath_filter' in item and fnmatch.fnmatch(
report.file.original_path, item['filepath_filter']):
return True
def __report_matches_rule(self, report: Report, rule: dict):
if 'filepath' in rule['filters'] and not fnmatch.fnmatch(
report.file.original_path, rule['filters']['filepath']):
return False

if 'checker_filter' in item and \
report.checker_name == item['checker_filter']:
return True
if 'checker_name' in rule['filters'] and \
report.checker_name != rule['filters']['checker_name']:
return False

if 'report_hash_filter' in item and \
report.report_hash.startswith(item['report_hash_filter']):
return True
if 'report_hash' in rule['filters'] and not \
report.report_hash.startswith(rule['filters']['report_hash']):
return False

return False
return True

def get_review_status(self, report: Report) -> SourceReviewStatus:
"""
Expand Down Expand Up @@ -185,10 +215,9 @@ def set_review_status_config(self, config_file):
encoding='utf-8', errors='ignore') as f:
# TODO: Validate format.
# - Can filepath be a list?
# TODO: May throw yaml.scanner.ScannerError.
try:
self.__data = yaml.safe_load(f)
except yaml.scanner.ScannerError as err:
except yaml.YAMLError as err:
raise ValueError(
f"Invalid YAML format in {self.__review_status_yaml}:\n"
f"{err}")
Expand All @@ -202,16 +231,16 @@ def should_ignore(self, report: Report) -> bool:
written to the analysis output files and CodeChecker should consider
them as not existing reports in any context.
TODO: Should "ignore" state be a review status? Currently it's a
standalone field.
TODO: If it's a standalone field, then maybe we should find another
name for this class, because it handles not only review statuses.
TODO: If "ignore" is a standalone field, then maybe we should find
another name for this class, because it handles not only review
statuses.
"""
if self.__data is None:
return False

for item in self.__data:
if self.__report_matches_item(report, item) and item.get('ignore'):
for rule in self.__data['rules']:
if self.__report_matches_rule(report, rule) and \
rule['actions'].get('ignore'):
return True

return False
Expand All @@ -230,19 +259,18 @@ def get_review_status_from_config(
"set_review_status_config()."

# TODO: Document "in_source".
for item in self.__data:
if not self.__report_matches_item(report, item) or \
item.get('ignore'):
for rule in self.__data['rules']:
if not self.__report_matches_rule(report, rule) or \
rule['actions'].get('ignore'):
continue

if any(filt in item for filt in
['filepath_filter', 'checker_filter',
'report_hash_filter']):
if any(filt in rule['filters'] for filt in
['filepath', 'checker_name', 'report_hash']):
return SourceReviewStatus(
status=item['review_status'],
message=item['message']
status=rule['actions']['review_status'],
message=rule['actions']['reason']
.encode(encoding='utf-8', errors='ignore')
if 'message' in item else b'',
if 'reason' in rule['actions'] else b'',
bug_hash=report.report_hash,
in_source=True)

Expand Down
90 changes: 58 additions & 32 deletions docs/analyzer/user_guide.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2610,38 +2610,64 @@ Review status can be configured by a config file in YAML format. This config
file has to represent a list of review status settings:

```yaml
- filepath_filter: /path/to/project/test/*
checker_filter: core.DivideZero
message: Division by zero in test files is automatically intentional.
review_status: intentional
- filepath_filter: /path/to/project/important/module/*
message: All reports in this module should be investigated.
review_status: confirmed
- filepath_filter: "*/project/test/*"
message: If a filter starts with asterix, then it should be quoted due to YAML format.
review_status: suppress
- report_hash_filter: b85851b34789e35c6acfa1a4aaf65382
message: This report is false positive.
review_status: false_positive
```
The fields of a review status settings are:
- `filepath_filter` (optional): A glob to a path where the given review status
is applied. A [https://docs.python.org/3/library/glob.html](glob) is a path
that may contain shell-style wildcards: `*` substitutes zero or more
characters, `?` substitutes exactly one character. This filter option is
applied on the full path of a source file, even if `--trim-path-prefix` flag
is used later.
- `checker_filter` (optional): Set the review status for only these checkers'
$version: 1
rules:
- filters:
filepath: /path/to/project/test/*
checker_name: core.DivideZero
actions:
review_status: intentional
reason: Division by zero in test files is automatically intentional.

- filters:
filepath: /path/to/project/important/module/*
actions:
review_status: confirmed
reason: All reports in this module should be investigated.

- filters:
filepath: "*/project/test/*"
actions:
review_status: suppress
reason: If a filter starts with asterix, then it should be quoted due to YAML format.

- filters:
report_hash: b85851b34789e35c6acfa1a4aaf65382
actions:
review_status: false_positive
reason: This report is false positive.

- filters:
filepath: /path/to/3pp/lib/*
actions:
ignore: true
reason: The reports of this lib are ignored completely.
```
The review settings rules have two componetes: `filters` and `actions`. A rule
is applied for every report that match the filter fields. The following filter
options are available:

- `filepath` (optional): A glob to a path where the given review status is
applied. A [https://docs.python.org/3/library/glob.html](glob) is a path that
may contain shell-style wildcards: `*` substitutes zero or more characters,
`?` substitutes exactly one character. This filter option is applied on the
full path of a source file, even if `--trim-path-prefix` flag is used later.
- `checker_name` (optional): Set the review status for only these checkers'
reports.
- `report_hash_filter` (optional): Set the review status for only the checkers
having this report hash. A prefix match is applied on report hashes, so it is
enough to provide the beginning of a hash. Make sure to use a quite long
prefix so it covers one specific report.
- `message` (optional): A comment message that describes the reason of the
setting.
- `report_hash` (optional): Set the review status for only the reports having
this report hash. A prefix match is applied on report hashes, so it is enough
to provide the beginning of a hash. Make sure to use a quite long prefix so
it covers one specific report.

The following actions are available:

- `review_status`: The review status to set.
- `ignore`: If set to `true` then the report is completely ignored. Such a
report is not even dumped to the .plist files, thus it's completely invisible
for other CodeChecker commands like "parse", "diff", "store", etc.
- `reason` (optional): A comment message that describes the reason of the
setting.

If none of the `_filter` options is provided, then that setting is not applied
on any report.
The `review_status` and `ignore` are mutually exclusive. If none of the filter
options is provided, then that setting is not applied on any report.

0 comments on commit 2c6a1d6

Please sign in to comment.