From e6bb08fd625953e2d8c0b8ec0e06124594352afc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= Date: Tue, 12 Apr 2022 14:41:28 +0200 Subject: [PATCH] [config][CLI] Merge, and don't override when multiple --analyzer-configs are specified Previous to this patch, multiple analyzer configs could be specified like this: CodeChecker analyze ./compile_commands.json -o ./reports -c --verbose debug \ --analyzers clang-tidy \ --analyzer-config clang-tidy:HeaderFilterRegex=".*" clang-tidy:AnalyzeTemporaryDtors="true" However, if you specifiy --analyzer-config more than once, the only the last one counts, like many other flags. The problem is that --analyzer-config may be specified from both the CLI and a CodeChecker config file, and even in this case, only one of these would count. Also, the -analyzer-config flag from the Clang Static Analyzer works differently, as multiple -analyzer-configs in the invocation and appended, not overwritten. Coming from that space, that philosophy feels more intuitive to me, and I think other users may be out these who might be suprised by this behaviour. This patch merges --analyzer-config (we append the list of configs) values instead of overwriting them. --- .../analyzers/clangtidy/analyzer.py | 4 +- analyzer/codechecker_analyzer/arg.py | 32 +++++ analyzer/codechecker_analyzer/cmd/analyze.py | 5 +- analyzer/codechecker_analyzer/cmd/check.py | 5 +- .../tests/functional/config/test_config.py | 123 ++++++++++++++++++ 5 files changed, 165 insertions(+), 4 deletions(-) diff --git a/analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py b/analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py index 62d5268c9e..376209c0ff 100644 --- a/analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py +++ b/analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py @@ -362,7 +362,7 @@ def construct_config_handler(cls, args, context): LOG.debug_analyzer(aerr) analyzer_config = {} - # TODO: This extra "isinsrance" check is needed for + # TODO: This extra "isinstance" check is needed for # CodeChecker analyzers --analyzer-config. This command also # runs this function in order to construct a config handler. if 'analyzer_config' in args and \ @@ -394,7 +394,7 @@ def construct_config_handler(cls, args, context): del handler.analyzer_extra_arguments[i:i + arg_num] break - # TODO: This extra "isinsrance" check is needed for + # TODO: This extra "isinstance" check is needed for # CodeChecker checkers --checker-config. This command also # runs this function in order to construct a config handler. if 'checker_config' in args and isinstance(args.checker_config, list): diff --git a/analyzer/codechecker_analyzer/arg.py b/analyzer/codechecker_analyzer/arg.py index 96d90dbea5..8d04cac69a 100644 --- a/analyzer/codechecker_analyzer/arg.py +++ b/analyzer/codechecker_analyzer/arg.py @@ -36,6 +36,38 @@ def __call__(self, parser, namespace, value, option_string=None): if 'ordered_checkers' not in namespace: namespace.ordered_checkers = [] ordered_checkers = namespace.ordered_checkers + # Map each checker to whether its enabled or not. ordered_checkers.append((value, self.dest == 'enable')) namespace.ordered_checkers = ordered_checkers + + +class OrderedConfigAction(argparse.Action): + """ + Action to store --analyzer-config and --checker-config values. These may + come from many sources, including the CLI, the CodeChecker config file, + the saargs file and the tidyargs file, or some other places. + """ + + def __init__(self, option_strings, dest, nargs=None, **kwargs): + if nargs is not '*': + raise ValueError("nargs must be '*' for backward compatibility " + "reasons!") + super(OrderedConfigAction, self).__init__(option_strings, dest, + nargs, **kwargs) + + def __call__(self, parser, namespace, value, option_string=None): + + assert isinstance(value, list), \ + f"--analyzer-config or --checker-config value ({value}) is " \ + "not a list, but should be if nargs is not None!" + + if not hasattr(namespace, self.dest): + setattr(namespace, self.dest, []) + + dest = getattr(namespace, self.dest) + + for flag in value: + if flag in dest: + dest.remove(flag) + dest.append(flag) diff --git a/analyzer/codechecker_analyzer/cmd/analyze.py b/analyzer/codechecker_analyzer/cmd/analyze.py index a1570b5e5a..00bd77e658 100644 --- a/analyzer/codechecker_analyzer/cmd/analyze.py +++ b/analyzer/codechecker_analyzer/cmd/analyze.py @@ -26,7 +26,8 @@ from codechecker_analyzer import analyzer, analyzer_context, env from codechecker_analyzer.analyzers import analyzer_types, clangsa -from codechecker_analyzer.arg import OrderedCheckersAction +from codechecker_analyzer.arg import \ + OrderedCheckersAction, OrderedConfigAction from codechecker_analyzer.buildlog import log_parser from codechecker_common import arg, logger, cmd_config @@ -401,6 +402,7 @@ def add_arguments_to_parser(parser): analyzer_opts.add_argument('--analyzer-config', dest='analyzer_config', nargs='*', + action=OrderedConfigAction, default=["clang-tidy:HeaderFilterRegex=.*"], help="Analyzer configuration options in the " "following format: analyzer:key=value. " @@ -423,6 +425,7 @@ def add_arguments_to_parser(parser): analyzer_opts.add_argument('--checker-config', dest='checker_config', nargs='*', + action=OrderedConfigAction, default=argparse.SUPPRESS, help="Checker configuration options in the " "following format: analyzer:key=value. " diff --git a/analyzer/codechecker_analyzer/cmd/check.py b/analyzer/codechecker_analyzer/cmd/check.py index 4ac0f28ebd..977f0620f0 100644 --- a/analyzer/codechecker_analyzer/cmd/check.py +++ b/analyzer/codechecker_analyzer/cmd/check.py @@ -20,7 +20,8 @@ from codechecker_analyzer import analyzer_context from codechecker_analyzer.analyzers import analyzer_types -from codechecker_analyzer.arg import OrderedCheckersAction +from codechecker_analyzer.arg import \ + OrderedCheckersAction, OrderedConfigAction from codechecker_common import arg, cmd_config, logger from codechecker_report_converter.source_code_comment_handler import \ @@ -349,6 +350,7 @@ def add_arguments_to_parser(parser): analyzer_opts.add_argument('--analyzer-config', dest='analyzer_config', nargs='*', + action=OrderedConfigAction, default=["clang-tidy:HeaderFilterRegex=.*"], help="Analyzer configuration options in the " "following format: analyzer:key=value. " @@ -371,6 +373,7 @@ def add_arguments_to_parser(parser): analyzer_opts.add_argument('--checker-config', dest='checker_config', nargs='*', + action=OrderedConfigAction, default=argparse.SUPPRESS, help="Checker configuration options in the " "following format: analyzer:key=value. " diff --git a/analyzer/tests/functional/config/test_config.py b/analyzer/tests/functional/config/test_config.py index 4ef822dd3c..b353f6536c 100644 --- a/analyzer/tests/functional/config/test_config.py +++ b/analyzer/tests/functional/config/test_config.py @@ -118,6 +118,129 @@ def test_only_clangsa_config(self): self.assertIn("clangsa analyzed simple.cpp", out) self.assertNotIn("clang-tidy analyzed simple.cpp", out) + def test_config_file_multiple_analyzer_config_resolution(self): + """ + Test whether multiple --analyzer-config arguments from a CodeChecker + config file are merged, and don't overwrite one another. + """ + with open(self.config_file_json, 'w+', + encoding="utf-8", errors="ignore") as config_f: + json.dump({ + 'analyzer': [ + "--analyzer-config", + "clangsa:track-conditions=false", + "--analyzer-config", + "clang-tidy:HeaderFilterRegex=.*" + ]}, config_f) + + out, returncode = self.__run_analyze(self.config_file_json, + ["--verbose", "debug_analyzer"]) + + self.assertNotEqual(returncode, 1) + self.assertIn("track-conditions=false", out) + self.assertIn("{\"HeaderFilterRegex\": \".*\"}", out) + + def test_config_file_and_cmd_resolution(self): + """ + Test whether multiple --analyzer-config arguments from *both* a + CodeChecker config file and the CLI are merged, and don't overwrite one + another. + """ + with open(self.config_file_json, 'w+', + encoding="utf-8", errors="ignore") as config_f: + json.dump({ + 'analyzer': [ + "--analyzer-config", + "clangsa:track-conditions=false" + ]}, config_f) + + out, returncode = self.__run_analyze(self.config_file_json, + ["--analyzer-config", + "clang-tidy:" + "HeaderFilterRegex=.*", + "--verbose", "debug_analyzer"]) + + self.assertNotEqual(returncode, 1) + self.assertIn("track-conditions=false", out) + self.assertIn("{\"HeaderFilterRegex\": \".*\"}", out) + + def test_cmd_multiple_analyzer_config_resolution(self): + """ + Test whether multiple --analyzer-config arguments from the command line + are merged, and don't overwrite one another. + """ + + with open(self.config_file_json, 'w+', + encoding="utf-8", errors="ignore") as config_f: + config_f.write("") + + out, returncode = self.__run_analyze(self.config_file_json, + ["--analyzer-config", + "clang-tidy:" + "HeaderFilterRegex=.*", + "--analyzer-config", + "clangsa:track-conditions=false", + "--verbose", "debug_analyzer"]) + + self.assertNotEqual(returncode, 1) + self.assertIn("track-conditions=false", out) + self.assertIn("{\"HeaderFilterRegex\": \".*\"}", out) + + def test_cmd_multiple_checker_config_resolution(self): + """ + Test whether multiple --checker-config arguments from the command line + are merged, and don't overwrite one another. + """ + + with open(self.config_file_json, 'w+', + encoding="utf-8", errors="ignore") as config_f: + config_f.write("") + + out, returncode = self.__run_analyze(self.config_file_json, + ["--checker-config", + "clangsa:" + "core.CallAndMessage:" + "CXXDeallocationArg=true", + "--checker-config", + "clangsa:" + "core.CallAndMessage:" + "ParameterCount=true", + "--verbose", "debug_analyzer"]) + + self.assertNotEqual(returncode, 1) + self.assertIn("core.CallAndMessage:CXXDeallocationArg=true", out) + self.assertIn("core.CallAndMessage:ParameterCount=true", out) + + def test_cmd_overrides_config_file(self): + """ + Test the precedence of multiple --analyzer-config arguments specify the + same option from both a CodeChecker config file and the CLI, but with a + different value. CLI arguments should in effect override the config + file (is the later argument in the invocation). + """ + with open(self.config_file_json, 'w+', + encoding="utf-8", errors="ignore") as config_f: + json.dump({ + 'analyze': [ + '--analyzers', 'clangsa', + "--analyzer-config", + "clangsa:track-conditions=false" + ]}, config_f) + + out, returncode = self.__run_analyze(self.config_file_json, + ["--analyzer-config", + "clangsa:track-conditions=true", + "--verbose", "debug_analyzer"]) + + self.assertNotEqual(returncode, 1) + + # If the config from the config file is positioned behind the config + # from the CLI, it will override the config file. As intended. + cli_idx = out.rfind("track-conditions=true") + conf_file_idx = out.rfind("track-conditions=false") + self.assertLess(conf_file_idx, cli_idx) + self.assertEqual(out.count("track-conditions=true"), 1) + def test_only_clangsa_config_backward_compatible_mixed(self): """ Test the 'analyzer' configuration option backward compatibility.