From d9ff250f82dd15dcc6fe56f3ec3a79a4dc6eee27 Mon Sep 17 00:00:00 2001 From: Valerii Koval Date: Sun, 19 Mar 2023 00:45:59 +0200 Subject: [PATCH] Improved file filtering for the Static Analysis feature (#4570) * Improved file filtering for the Static Analysis feature * Better handling of legacy "check_patterns" option * Rename "check_src_filter" to plural form to better represent functionality * Move to plural forms of filter variables --- HISTORY.rst | 1 + platformio/check/cli.py | 28 +++- platformio/check/tools/base.py | 44 ++++-- platformio/check/tools/clangtidy.py | 4 +- platformio/check/tools/cppcheck.py | 10 +- platformio/check/tools/pvsstudio.py | 2 +- platformio/project/options.py | 3 +- tests/commands/test_check.py | 208 +++++++++++++++++++++------- 8 files changed, 224 insertions(+), 76 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 9d4b7277..57126d54 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -20,6 +20,7 @@ PlatformIO Core 6 * Show detailed library dependency tree only in the `verbose mode `__ (`issue #4517 `_) * Prevented shell injection when converting INO file to CPP (`issue #4532 `_) * Restored project generator for `NetBeans IDE `__ +* Improved source file filtering functionality for `Static Code Analysis `__ feature 6.1.6 (2023-01-23) ~~~~~~~~~~~~~~~~~~ diff --git a/platformio/check/cli.py b/platformio/check/cli.py index e293e8e0..215a6b8a 100644 --- a/platformio/check/cli.py +++ b/platformio/check/cli.py @@ -49,7 +49,12 @@ from platformio.project.helpers import find_project_dir_above, get_project_dir exists=True, file_okay=True, dir_okay=False, readable=True, resolve_path=True ), ) -@click.option("--pattern", multiple=True) +@click.option("--pattern", multiple=True, hidden=True) +@click.option( + "-f", + "--src-filters", + multiple=True +) @click.option("--flags", multiple=True) @click.option( "--severity", multiple=True, type=click.Choice(DefectItem.SEVERITY_LABELS.values()) @@ -67,6 +72,7 @@ def cli( environment, project_dir, project_conf, + src_filters, pattern, flags, severity, @@ -105,14 +111,24 @@ def cli( "%s: %s" % (k, ", ".join(v) if isinstance(v, list) else v) ) - default_patterns = [ - config.get("platformio", "src_dir"), - config.get("platformio", "include_dir"), + default_src_filters = [ + "+<%s>" % os.path.basename(config.get("platformio", "src_dir")), + "+<%s>" % os.path.basename(config.get("platformio", "include_dir")), ] + + src_filters = ( + src_filters + or pattern + or env_options.get( + "check_src_filters", + env_options.get("check_patterns", default_src_filters), + ) + ) + tool_options = dict( verbose=verbose, silent=silent, - patterns=pattern or env_options.get("check_patterns", default_patterns), + src_filters=src_filters, flags=flags or env_options.get("check_flags"), severity=[DefectItem.SEVERITY_LABELS[DefectItem.SEVERITY_HIGH]] if silent @@ -265,7 +281,7 @@ def print_defects_stats(results): tabular_data.append(total) headers = ["Component"] - headers.extend([l.upper() for l in severity_labels]) + headers.extend([label.upper() for label in severity_labels]) headers = [click.style(h, bold=True) for h in headers] click.echo(tabulate(tabular_data, headers=headers, numalign="center")) click.echo() diff --git a/platformio/check/tools/base.py b/platformio/check/tools/base.py index d51b3158..392a19af 100644 --- a/platformio/check/tools/base.py +++ b/platformio/check/tools/base.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import glob import os import tempfile @@ -30,6 +29,7 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes self.config = config self.envname = envname self.options = options + self.project_dir = project_dir self.cc_flags = [] self.cxx_flags = [] self.cpp_includes = [] @@ -41,7 +41,7 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes self._defects = [] self._on_defect_callback = None self._bad_input = False - self._load_cpp_data(project_dir) + self._load_cpp_data() # detect all defects by default if not self.options.get("severity"): @@ -56,8 +56,8 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes for s in self.options["severity"] ] - def _load_cpp_data(self, project_dir): - data = load_build_metadata(project_dir, self.envname) + def _load_cpp_data(self): + data = load_build_metadata(self.project_dir, self.envname) if not data: return self.cc_flags = click.parser.split_arg_string(data.get("cc_flags", "")) @@ -99,6 +99,13 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes includes_file, ) result = proc.exec_command(cmd, shell=True) + + if result["returncode"] != 0: + click.echo("Warning: Failed to extract toolchain defines!") + if self.options.get("verbose"): + click.echo(result["out"]) + click.echo(result["err"]) + for line in result["out"].split("\n"): tokens = line.strip().split(" ", 2) if not tokens or tokens[0] != "#define": @@ -201,7 +208,7 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes return result @staticmethod - def get_project_target_files(patterns): + def get_project_target_files(project_dir, src_filters): c_extension = (".c",) cpp_extensions = (".cc", ".cpp", ".cxx", ".ino") header_extensions = (".h", ".hh", ".hpp", ".hxx") @@ -216,13 +223,9 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes elif path.endswith(cpp_extensions): result["c++"].append(os.path.abspath(path)) - for pattern in patterns: - for item in glob.glob(pattern, recursive=True): - if not os.path.isdir(item): - _add_file(item) - for root, _, files in os.walk(item, followlinks=True): - for f in files: - _add_file(os.path.join(root, f)) + src_filters = normalize_src_filters(src_filters) + for f in fs.match_src_files(project_dir, src_filters): + _add_file(f) return result @@ -243,3 +246,20 @@ class CheckToolBase: # pylint: disable=too-many-instance-attributes self.clean_up() return self._bad_input + + +# +# Helpers +# + + +def normalize_src_filters(src_filters): + def _normalize(src_filters): + return ( + src_filters if src_filters.startswith(("+<", "-<")) else "+<%s>" % src_filters + ) + + if isinstance(src_filters, (list, tuple)): + return " ".join([_normalize(f) for f in src_filters]) + + return _normalize(src_filters) diff --git a/platformio/check/tools/clangtidy.py b/platformio/check/tools/clangtidy.py index 1dd4165e..496acc7c 100644 --- a/platformio/check/tools/clangtidy.py +++ b/platformio/check/tools/clangtidy.py @@ -64,7 +64,9 @@ class ClangtidyCheckTool(CheckToolBase): ): cmd.append("--checks=*") - project_files = self.get_project_target_files(self.options["patterns"]) + project_files = self.get_project_target_files( + self.project_dir, self.options["src_filters"] + ) src_files = [] for items in project_files.values(): diff --git a/platformio/check/tools/cppcheck.py b/platformio/check/tools/cppcheck.py index 0f8db402..c3c59537 100644 --- a/platformio/check/tools/cppcheck.py +++ b/platformio/check/tools/cppcheck.py @@ -96,7 +96,7 @@ class CppcheckCheckTool(CheckToolBase): ) click.echo() self._bad_input = True - self._buffer = "" + self._buffer = "" return None self._buffer = "" @@ -214,7 +214,9 @@ class CppcheckCheckTool(CheckToolBase): if not self.is_flag_set("--addon", self.get_flags("cppcheck")): return - for files in self.get_project_target_files(self.options["patterns"]).values(): + for files in self.get_project_target_files( + self.project_dir, self.options["src_filters"] + ).values(): for f in files: dump_file = f + ".dump" if os.path.isfile(dump_file): @@ -243,7 +245,9 @@ class CppcheckCheckTool(CheckToolBase): def check(self, on_defect_callback=None): self._on_defect_callback = on_defect_callback - project_files = self.get_project_target_files(self.options["patterns"]) + project_files = self.get_project_target_files( + self.project_dir, self.options["src_filters"] + ) src_files_scope = ("c", "c++") if not any(project_files[t] for t in src_files_scope): click.echo("Error: Nothing to check.") diff --git a/platformio/check/tools/pvsstudio.py b/platformio/check/tools/pvsstudio.py index ded65d1c..20979a00 100644 --- a/platformio/check/tools/pvsstudio.py +++ b/platformio/check/tools/pvsstudio.py @@ -227,7 +227,7 @@ class PvsStudioCheckTool(CheckToolBase): # pylint: disable=too-many-instance-at def check(self, on_defect_callback=None): self._on_defect_callback = on_defect_callback for scope, files in self.get_project_target_files( - self.options["patterns"] + self.project_dir, self.options["src_filters"] ).items(): if scope not in ("c", "c++"): continue diff --git a/platformio/project/options.py b/platformio/project/options.py index 5e200629..53775b5d 100644 --- a/platformio/project/options.py +++ b/platformio/project/options.py @@ -649,7 +649,8 @@ ProjectOptions = OrderedDict( ), ConfigEnvOption( group="check", - name="check_patterns", + name="check_src_filters", + oldnames=["check_patterns"], description=( "Configure a list of target files or directories for checking " "(Unix shell-style wildcards)" diff --git a/tests/commands/test_check.py b/tests/commands/test_check.py index be6042ca..cb6f7777 100644 --- a/tests/commands/test_check.py +++ b/tests/commands/test_check.py @@ -15,8 +15,8 @@ # pylint: disable=redefined-outer-name import json +import os import sys -from os.path import isfile, join import pytest @@ -83,12 +83,12 @@ def check_dir(tmpdir_factory): def count_defects(output): error, warning, style = 0, 0, 0 - for l in output.split("\n"): - if "[high:error]" in l: + for line in output.split("\n"): + if "[high:error]" in line: error += 1 - elif "[medium:warning]" in l: + elif "[medium:warning]" in line: warning += 1 - elif "[low:style]" in l: + elif "[low:style]" in line: style += 1 return error, warning, style @@ -240,9 +240,9 @@ def test_check_includes_passed(clirunner, check_dir): result = clirunner.invoke(cmd_check, ["--project-dir", str(check_dir), "--verbose"]) inc_count = 0 - for l in result.output.split("\n"): - if l.startswith("Includes:"): - inc_count = l.count("-I") + for line in result.output.split("\n"): + if line.startswith("Includes:"): + inc_count = line.count("-I") # at least 1 include path for default mode assert inc_count > 0 @@ -259,46 +259,6 @@ def test_check_silent_mode(clirunner, validate_cliresult, check_dir): assert style == 0 -def test_check_custom_pattern_absolute_path( - clirunner, validate_cliresult, tmpdir_factory -): - project_dir = tmpdir_factory.mktemp("project") - project_dir.join("platformio.ini").write(DEFAULT_CONFIG) - - check_dir = tmpdir_factory.mktemp("custom_src_dir") - check_dir.join("main.cpp").write(TEST_CODE) - - result = clirunner.invoke( - cmd_check, ["--project-dir", str(project_dir), "--pattern=" + str(check_dir)] - ) - validate_cliresult(result) - - errors, warnings, style = count_defects(result.output) - - assert errors == EXPECTED_ERRORS - assert warnings == EXPECTED_WARNINGS - assert style == EXPECTED_STYLE - - -def test_check_custom_pattern_relative_path( - clirunner, validate_cliresult, tmpdir_factory -): - tmpdir = tmpdir_factory.mktemp("project") - tmpdir.join("platformio.ini").write(DEFAULT_CONFIG) - - tmpdir.mkdir("app").join("main.cpp").write(TEST_CODE) - tmpdir.mkdir("prj").join("test.cpp").write(TEST_CODE) - - result = clirunner.invoke( - cmd_check, ["--project-dir", str(tmpdir), "--pattern=app", "--pattern=prj"] - ) - validate_cliresult(result) - - errors, warnings, style = count_defects(result.output) - - assert errors + warnings + style == EXPECTED_DEFECTS * 2 - - def test_check_no_source_files(clirunner, tmpdir): tmpdir.join("platformio.ini").write(DEFAULT_CONFIG) tmpdir.mkdir("src") @@ -427,7 +387,7 @@ R21.4 text. validate_cliresult(result) assert "R21.3 Found MISRA defect" in result.output - assert not isfile(join(str(check_dir), "src", "main.cpp.dump")) + assert not os.path.isfile(os.path.join(str(check_dir), "src", "main.cpp.dump")) def test_check_fails_on_defects_only_with_flag(clirunner, validate_cliresult, tmpdir): @@ -607,10 +567,10 @@ framework = arduino validate_cliresult(result) project_path = fs.to_unix_path(str(tmpdir)) - for l in result.output.split("\n"): - if not l.startswith("Includes:"): + for line in result.output.split("\n"): + if not line.startswith("Includes:"): continue - for inc in l.split(" "): + for inc in line.split(" "): if inc.startswith("-I") and project_path not in inc: pytest.fail("Detected an include path from packages: " + inc) @@ -656,3 +616,147 @@ def test_check_handles_spaces_in_paths(clirunner, validate_cliresult, tmpdir_fac default_result = clirunner.invoke(cmd_check, ["--project-dir", str(tmpdir)]) validate_cliresult(default_result) + + +# +# Files filtering functionality +# + + +@pytest.mark.parametrize( + "src_filter,number_of_checked_files", + [ + (["+"], 1), + (["+"], 1), + (["+", "-"], 2), + (["-<*> + + +"], 3), + ], + ids=["Single file", "Glob pattern", "Exclude pattern", "Filter as string"], +) +def test_check_src_filter( + clirunner, + validate_cliresult, + tmpdir_factory, + src_filter, + number_of_checked_files, +): + tmpdir = tmpdir_factory.mktemp("project") + tmpdir.join("platformio.ini").write(DEFAULT_CONFIG) + + src_dir = tmpdir.mkdir("src") + src_dir.join("main.cpp").write(TEST_CODE) + src_dir.join("app.cpp").write(TEST_CODE) + src_dir.mkdir("uart").join("uart.cpp").write(TEST_CODE) + src_dir.mkdir("spi").join("spi.cpp").write(TEST_CODE) + tmpdir.mkdir("tests").join("test.cpp").write(TEST_CODE) + + cmd_args = ["--project-dir", str(tmpdir)] + [ + "--src-filters=%s" % f for f in src_filter + ] + + result = clirunner.invoke(cmd_check, cmd_args) + validate_cliresult(result) + + errors, warnings, style = count_defects(result.output) + + assert errors + warnings + style == EXPECTED_DEFECTS * number_of_checked_files + + +def test_check_src_filter_from_config(clirunner, validate_cliresult, tmpdir_factory): + tmpdir = tmpdir_factory.mktemp("project") + + config = ( + DEFAULT_CONFIG + + """ +check_src_filters = + + + + + """ + ) + tmpdir.join("platformio.ini").write(config) + + src_dir = tmpdir.mkdir("src") + src_dir.join("main.cpp").write(TEST_CODE) + src_dir.mkdir("spi").join("spi.cpp").write(TEST_CODE) + tmpdir.mkdir("tests").join("test.cpp").write(TEST_CODE) + + result = clirunner.invoke(cmd_check, ["--project-dir", str(tmpdir)]) + validate_cliresult(result) + + errors, warnings, style = count_defects(result.output) + + assert errors + warnings + style == EXPECTED_DEFECTS * 2 + assert "main.cpp" not in result.output + + +def test_check_custom_pattern_absolute_path_legacy( + clirunner, validate_cliresult, tmpdir_factory +): + project_dir = tmpdir_factory.mktemp("project") + project_dir.join("platformio.ini").write(DEFAULT_CONFIG) + + check_dir = tmpdir_factory.mktemp("custom_src_dir") + check_dir.join("main.cpp").write(TEST_CODE) + + result = clirunner.invoke( + cmd_check, ["--project-dir", str(project_dir), "--pattern=" + str(check_dir)] + ) + + validate_cliresult(result) + + errors, warnings, style = count_defects(result.output) + + assert errors == EXPECTED_ERRORS + assert warnings == EXPECTED_WARNINGS + assert style == EXPECTED_STYLE + + +def test_check_custom_pattern_relative_path_legacy( + clirunner, validate_cliresult, tmpdir_factory +): + tmpdir = tmpdir_factory.mktemp("project") + tmpdir.join("platformio.ini").write(DEFAULT_CONFIG) + + src_dir = tmpdir.mkdir("src") + src_dir.join("main.cpp").write(TEST_CODE) + src_dir.mkdir("uart").join("uart.cpp").write(TEST_CODE) + src_dir.mkdir("spi").join("spi.cpp").write(TEST_CODE) + + result = clirunner.invoke( + cmd_check, + ["--project-dir", str(tmpdir), "--pattern=src/uart", "--pattern=src/spi"], + ) + validate_cliresult(result) + + errors, warnings, style = count_defects(result.output) + + assert errors + warnings + style == EXPECTED_DEFECTS * 2 + + +def test_check_src_filter_from_config_legacy( + clirunner, validate_cliresult, tmpdir_factory +): + tmpdir = tmpdir_factory.mktemp("project") + + config = ( + DEFAULT_CONFIG + + """ +check_patterns = + src/spi/*.c* + tests/test.cpp + """ + ) + tmpdir.join("platformio.ini").write(config) + + src_dir = tmpdir.mkdir("src") + src_dir.join("main.cpp").write(TEST_CODE) + src_dir.mkdir("spi").join("spi.cpp").write(TEST_CODE) + tmpdir.mkdir("tests").join("test.cpp").write(TEST_CODE) + + result = clirunner.invoke(cmd_check, ["--project-dir", str(tmpdir)]) + validate_cliresult(result) + + errors, warnings, style = count_defects(result.output) + + assert errors + warnings + style == EXPECTED_DEFECTS * 2 + assert "main.cpp" not in result.output