Refactor PIO Check feature (#3478)

* Add new option --skip-packages for check command

Check tools might fail if they're not able to preprocess source
files, for example, Cppcheck uses a custom preprocessor that is
not able to parse complex preprocessor code in zephyr framework).
Instead user can specify this option to skip headers included from
packages and only check project sources.

* Fix toolchain built-in include paths order
C++ and fixed directories should have higher priority

* Refactor check feature

The main purpose is to prepare a more comprehensive build environment.
It's crucial for cppcheck to be able to check complex frameworks like
zephyr, esp-idf, etc. Also detect a special case when cppcheck fails to check
the entire project (e.g. a syntax error due to custom preprocessor)

* Add new test for check feature

Tests ststm32 platform all tools and the main frameworks

* Update check tools to the latest available versions

* Test check tools and Zephyr framework only with Python 3

* Tidy up code

* Add history entry

HISTORY.rst

@@ -11,6 +11,7 @@ PlatformIO Core 4
 
 * New `Account Management System <https://docs.platformio.org/page/plus/pio-account.html>`__ with "username" and social providers (preview)
 * Open source `PIO Remote <http://docs.platformio.org/page/plus/pio-remote.html>`__ client
+* Improved `PIO Check <http://docs.platformio.org/page/plus/pio-check.html>`__ with more accurate project processing
 * Echo what is typed when ``send_on_enter`` device monitor filter <https://docs.platformio.org/page/projectconf/section_env_monitor.html#monitor-filters>`__ is used (`issue #3452 <https://github.com/platformio/platformio-core/issues/3452>`_)
 * Fixed PIO Unit Testing for Zephyr RTOS
 * Fixed UnicodeDecodeError on Windows when network drive (NAS) is used (`issue #3417 <https://github.com/platformio/platformio-core/issues/3417>`_)

platformio/__init__.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-VERSION = (4, 3, "2b1")
+VERSION = (4, 3, "2a4")
 __version__ = ".".join([str(s) for s in VERSION])
 
 __title__ = "platformio"

platformio/builder/tools/pioide.py

@@ -50,10 +50,10 @@ def _dump_includes(env):
             continue
         toolchain_dir = glob_escape(p.get_package_dir(name))
         toolchain_incglobs = [
-            os.path.join(toolchain_dir, "*", "include*"),
             os.path.join(toolchain_dir, "*", "include", "c++", "*"),
             os.path.join(toolchain_dir, "*", "include", "c++", "*", "*-*-*"),
             os.path.join(toolchain_dir, "lib", "gcc", "*", "*", "include*"),
+            os.path.join(toolchain_dir, "*", "include*"),
         ]
         for g in toolchain_incglobs:
             includes["toolchain"].extend([os.path.realpath(inc) for inc in glob(g)])

platformio/commands/check/command.py

@@ -61,6 +61,7 @@ from platformio.project.helpers import find_project_dir_above, get_project_dir
     multiple=True,
     type=click.Choice(DefectItem.SEVERITY_LABELS.values()),
 )
+@click.option("--skip-packages", is_flag=True)
 def cli(
     environment,
     project_dir,
@@ -72,6 +73,7 @@ def cli(
     verbose,
     json_output,
     fail_on_defect,
+    skip_packages,
 ):
     app.set_session_var("custom_project_conf", project_conf)
 
@@ -114,6 +116,7 @@ def cli(
                 severity=[DefectItem.SEVERITY_LABELS[DefectItem.SEVERITY_HIGH]]
                 if silent
                 else severity or config.get("env:" + envname, "check_severity"),
+                skip_packages=skip_packages or env_options.get("check_skip_packages"),
             )
 
             for tool in config.get("env:" + envname, "check_tool"):
@@ -222,7 +225,7 @@ def collect_component_stats(result):
         component = dirname(defect.file) or defect.file
         _append_defect(component, defect)
 
-        if component.startswith(get_project_dir()):
+        if component.lower().startswith(get_project_dir().lower()):
             while os.sep in component:
                 component = dirname(component)
                 _append_defect(component, defect)

platformio/commands/check/defect.py

@@ -51,7 +51,7 @@ class DefectItem(object):
         self.cwe = cwe
         self.id = id
         self.file = file
-        if file.startswith(get_project_dir()):
+        if file.lower().startswith(get_project_dir().lower()):
             self.file = os.path.relpath(file, get_project_dir())
 
     def __repr__(self):

platformio/commands/check/tools/base.py

@@ -14,12 +14,13 @@
 
 import glob
 import os
+from tempfile import NamedTemporaryFile
 
 import click
 
 from platformio import fs, proc
 from platformio.commands.check.defect import DefectItem
-from platformio.project.helpers import get_project_dir, load_project_ide_data
+from platformio.project.helpers import load_project_ide_data
 
 
 class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
@@ -32,12 +33,13 @@ class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
         self.cpp_includes = []
         self.cpp_defines = []
         self.toolchain_defines = []
+        self._tmp_files = []
         self.cc_path = None
         self.cxx_path = None
         self._defects = []
         self._on_defect_callback = None
         self._bad_input = False
-        self._load_cpp_data(project_dir, envname)
+        self._load_cpp_data(project_dir)
 
         # detect all defects by default
         if not self.options.get("severity"):
@@ -52,17 +54,17 @@ class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
             for s in self.options["severity"]
         ]
 
-    def _load_cpp_data(self, project_dir, envname):
-        data = load_project_ide_data(project_dir, envname)
+    def _load_cpp_data(self, project_dir):
+        data = load_project_ide_data(project_dir, self.envname)
         if not data:
             return
-        self.cc_flags = data.get("cc_flags", "").split(" ")
-        self.cxx_flags = data.get("cxx_flags", "").split(" ")
+        self.cc_flags = click.parser.split_arg_string(data.get("cc_flags", ""))
+        self.cxx_flags = click.parser.split_arg_string(data.get("cxx_flags", ""))
         self.cpp_includes = self._dump_includes(data.get("includes", {}))
         self.cpp_defines = data.get("defines", [])
         self.cc_path = data.get("cc_path")
         self.cxx_path = data.get("cxx_path")
-        self.toolchain_defines = self._get_toolchain_defines(self.cc_path)
+        self.toolchain_defines = self._get_toolchain_defines()
 
     def get_flags(self, tool):
         result = []
@@ -75,21 +77,43 @@ class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
 
         return result
 
-    @staticmethod
-    def _get_toolchain_defines(cc_path):
-        defines = []
-        result = proc.exec_command("echo | %s -dM -E -x c++ -" % cc_path, shell=True)
+    def _get_toolchain_defines(self):
+        def _extract_defines(language, includes_file):
+            build_flags = self.cxx_flags if language == "c++" else self.cc_flags
+            defines = []
+            cmd = "echo | %s -x %s %s %s -dM -E -" % (
+                self.cc_path,
+                language,
+                " ".join([f for f in build_flags if f.startswith(("-m", "-f"))]),
+                includes_file,
+            )
+            result = proc.exec_command(cmd, shell=True)
+            for line in result["out"].split("\n"):
+                tokens = line.strip().split(" ", 2)
+                if not tokens or tokens[0] != "#define":
+                    continue
+                if len(tokens) > 2:
+                    defines.append("%s=%s" % (tokens[1], tokens[2]))
+                else:
+                    defines.append(tokens[1])
 
-        for line in result["out"].split("\n"):
-            tokens = line.strip().split(" ", 2)
-            if not tokens or tokens[0] != "#define":
-                continue
-            if len(tokens) > 2:
-                defines.append("%s=%s" % (tokens[1], tokens[2]))
-            else:
-                defines.append(tokens[1])
+            return defines
 
-        return defines
+        incflags_file = self._long_includes_hook(self.cpp_includes)
+        return {lang: _extract_defines(lang, incflags_file) for lang in ("c", "c++")}
+
+    def _create_tmp_file(self, data):
+        with NamedTemporaryFile("w", delete=False) as fp:
+            fp.write(data)
+            self._tmp_files.append(fp.name)
+            return fp.name
+
+    def _long_includes_hook(self, includes):
+        data = []
+        for inc in includes:
+            data.append('-I"%s"' % fs.to_unix_path(inc))
+
+        return '@"%s"' % self._create_tmp_file(" ".join(data))
 
     @staticmethod
     def _dump_includes(includes_map):
@@ -138,18 +162,27 @@ class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
         return raw_line
 
     def clean_up(self):
-        pass
+        for f in self._tmp_files:
+            if os.path.isfile(f):
+                os.remove(f)
 
-    def get_project_target_files(self):
-        allowed_extensions = (".h", ".hpp", ".c", ".cc", ".cpp", ".ino")
-        result = []
+    @staticmethod
+    def get_project_target_files(patterns):
+        c_extension = (".c",)
+        cpp_extensions = (".cc", ".cpp", ".cxx", ".ino")
+        header_extensions = (".h", ".hh", ".hpp", ".hxx")
+
+        result = {"c": [], "c++": [], "headers": []}
 
         def _add_file(path):
-            if not path.endswith(allowed_extensions):
-                return
-            result.append(os.path.realpath(path))
+            if path.endswith(header_extensions):
+                result["headers"].append(os.path.realpath(path))
+            elif path.endswith(c_extension):
+                result["c"].append(os.path.realpath(path))
+            elif path.endswith(cpp_extensions):
+                result["c++"].append(os.path.realpath(path))
 
-        for pattern in self.options["patterns"]:
+        for pattern in patterns:
             for item in glob.glob(pattern):
                 if not os.path.isdir(item):
                     _add_file(item)
@@ -159,27 +192,23 @@ class CheckToolBase(object):  # pylint: disable=too-many-instance-attributes
 
         return result
 
-    def get_source_language(self):
-        with fs.cd(get_project_dir()):
-            for _, __, files in os.walk(self.config.get_optional_dir("src")):
-                for name in files:
-                    if "." not in name:
-                        continue
-                    if os.path.splitext(name)[1].lower() in (".cpp", ".cxx", ".ino"):
-                        return "c++"
-            return "c"
-
     def check(self, on_defect_callback=None):
         self._on_defect_callback = on_defect_callback
         cmd = self.configure_command()
-        if self.options.get("verbose"):
-            click.echo(" ".join(cmd))
+        if cmd:
+            if self.options.get("verbose"):
+                click.echo(" ".join(cmd))
 
-        proc.exec_command(
-            cmd,
-            stdout=proc.LineBufferedAsyncPipe(self.on_tool_output),
-            stderr=proc.LineBufferedAsyncPipe(self.on_tool_output),
-        )
+            proc.exec_command(
+                cmd,
+                stdout=proc.LineBufferedAsyncPipe(self.on_tool_output),
+                stderr=proc.LineBufferedAsyncPipe(self.on_tool_output),
+            )
+
+        else:
+            if self.options.get("verbose"):
+                click.echo("Error: Couldn't configure command")
+            self._bad_input = True
 
         self.clean_up()
 

platformio/commands/check/tools/clangtidy.py

@@ -57,11 +57,28 @@ class ClangtidyCheckTool(CheckToolBase):
         if not self.is_flag_set("--checks", flags):
             cmd.append("--checks=*")
 
+        project_files = self.get_project_target_files(self.options["patterns"])
+
+        src_files = []
+        for scope in project_files:
+            src_files.extend(project_files[scope])
+
         cmd.extend(flags)
-        cmd.extend(self.get_project_target_files())
+        cmd.extend(src_files)
         cmd.append("--")
 
-        cmd.extend(["-D%s" % d for d in self.cpp_defines + self.toolchain_defines])
-        cmd.extend(["-I%s" % inc for inc in self.cpp_includes])
+        cmd.extend(
+            ["-D%s" % d for d in self.cpp_defines + self.toolchain_defines["c++"]]
+        )
+
+        includes = []
+        for inc in self.cpp_includes:
+            if self.options.get("skip_packages") and inc.lower().startswith(
+                self.config.get_optional_dir("packages").lower()
+            ):
+                continue
+            includes.append(inc)
+
+        cmd.append("--extra-arg=" + self._long_includes_hook(includes))
 
         return cmd

platformio/commands/check/tools/cppcheck.py

@@ -12,10 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from os import remove
-from os.path import isfile, join
-from tempfile import NamedTemporaryFile
+import os
 
+import click
+
+from platformio import proc
 from platformio.commands.check.defect import DefectItem
 from platformio.commands.check.tools.base import CheckToolBase
 from platformio.managers.core import get_core_package_dir
@@ -23,7 +24,6 @@ from platformio.managers.core import get_core_package_dir
 
 class CppcheckCheckTool(CheckToolBase):
     def __init__(self, *args, **kwargs):
-        self._tmp_files = []
         self.defect_fields = [
             "severity",
             "message",
@@ -74,10 +74,32 @@ class CppcheckCheckTool(CheckToolBase):
         else:
             args["severity"] = DefectItem.SEVERITY_LOW
 
+        # Skip defects found in third-party software, but keep in mind that such defects
+        # might break checking process so defects from project files are not reported
+        breaking_defect_ids = ("preprocessorErrorDirective", "syntaxError")
+        if (
+            args.get("file", "")
+            .lower()
+            .startswith(self.config.get_optional_dir("packages").lower())
+        ):
+            if args["id"] in breaking_defect_ids:
+                if self.options.get("verbose"):
+                    click.echo(
+                        "Error: Found a breaking defect '%s' in %s:%s\n"
+                        "Please note: check results might not be valid!\n"
+                        "Try adding --skip-packages"
+                        % (args.get("message"), args.get("file"), args.get("line"))
+                    )
+                    click.echo()
+                self._bad_input = True
+            return None
+
         return DefectItem(**args)
 
-    def configure_command(self):
-        tool_path = join(get_core_package_dir("tool-cppcheck"), "cppcheck")
+    def configure_command(
+        self, language, src_files
+    ):  # pylint: disable=arguments-differ
+        tool_path = os.path.join(get_core_package_dir("tool-cppcheck"), "cppcheck")
 
         cmd = [
             tool_path,
@@ -108,51 +130,112 @@ class CppcheckCheckTool(CheckToolBase):
             cmd.append("--enable=%s" % ",".join(enabled_checks))
 
         if not self.is_flag_set("--language", flags):
-            if self.get_source_language() == "c++":
-                cmd.append("--language=c++")
+            cmd.append("--language=" + language)
 
-                if not self.is_flag_set("--std", flags):
-                    for f in self.cxx_flags + self.cc_flags:
-                        if "-std" in f:
-                            # Standards with GNU extensions are not allowed
-                            cmd.append("-" + f.replace("gnu", "c"))
+        build_flags = self.cxx_flags if language == "c++" else self.cc_flags
+
+        for flag in build_flags:
+            if "-std" in flag:
+                # Standards with GNU extensions are not allowed
+                cmd.append("-" + flag.replace("gnu", "c"))
+
+        cmd.extend(
+            ["-D%s" % d for d in self.cpp_defines + self.toolchain_defines[language]]
+        )
 
-        cmd.extend(["-D%s" % d for d in self.cpp_defines + self.toolchain_defines])
         cmd.extend(flags)
 
-        cmd.append("--file-list=%s" % self._generate_src_file())
+        cmd.extend(
+            "--include=" + inc
+            for inc in self.get_forced_includes(build_flags, self.cpp_includes)
+        )
+        cmd.append("--file-list=%s" % self._generate_src_file(src_files))
         cmd.append("--includes-file=%s" % self._generate_inc_file())
 
-        core_dir = self.config.get_optional_dir("packages")
-        cmd.append("--suppress=*:%s*" % core_dir)
-        cmd.append("--suppress=unmatchedSuppression:%s*" % core_dir)
-
         return cmd
 
-    def _create_tmp_file(self, data):
-        with NamedTemporaryFile("w", delete=False) as fp:
-            fp.write(data)
-            self._tmp_files.append(fp.name)
-            return fp.name
+    @staticmethod
+    def get_forced_includes(build_flags, includes):
+        def _extract_filepath(flag, include_options, build_flags):
+            path = ""
+            for option in include_options:
+                if not flag.startswith(option):
+                    continue
+                if flag.split(option)[1].strip():
+                    path = flag.split(option)[1].strip()
+                elif build_flags.index(flag) + 1 < len(build_flags):
+                    path = build_flags[build_flags.index(flag) + 1]
+            return path
 
-    def _generate_src_file(self):
-        src_files = [
-            f for f in self.get_project_target_files() if not f.endswith((".h", ".hpp"))
-        ]
+        def _search_include_dir(filepath, include_paths):
+            for inc_path in include_paths:
+                path = os.path.join(inc_path, filepath)
+                if os.path.isfile(path):
+                    return path
+            return ""
+
+        result = []
+        include_options = ("-include", "-imacros")
+        for f in build_flags:
+            if f.startswith(include_options):
+                filepath = _extract_filepath(f, include_options, build_flags)
+                if not os.path.isabs(filepath):
+                    filepath = _search_include_dir(filepath, includes)
+                if os.path.isfile(filepath):
+                    result.append(filepath)
+
+        return result
+
+    def _generate_src_file(self, src_files):
         return self._create_tmp_file("\n".join(src_files))
 
     def _generate_inc_file(self):
-        return self._create_tmp_file("\n".join(self.cpp_includes))
+        result = []
+        for inc in self.cpp_includes:
+            if self.options.get("skip_packages") and inc.lower().startswith(
+                self.config.get_optional_dir("packages").lower()
+            ):
+                continue
+            result.append(inc)
+        return self._create_tmp_file("\n".join(result))
 
     def clean_up(self):
-        for f in self._tmp_files:
-            if isfile(f):
-                remove(f)
+        super(CppcheckCheckTool, self).clean_up()
 
         # delete temporary dump files generated by addons
         if not self.is_flag_set("--addon", self.get_flags("cppcheck")):
             return
-        for f in self.get_project_target_files():
-            dump_file = f + ".dump"
-            if isfile(dump_file):
-                remove(dump_file)
+
+        for files in self.get_project_target_files(self.options["patterns"]).values():
+            for f in files:
+                dump_file = f + ".dump"
+                if os.path.isfile(dump_file):
+                    os.remove(dump_file)
+
+    def check(self, on_defect_callback=None):
+        self._on_defect_callback = on_defect_callback
+        project_files = self.get_project_target_files(self.options["patterns"])
+
+        languages = ("c", "c++")
+        if not any([project_files[t] for t in languages]):
+            click.echo("Error: Nothing to check.")
+            return True
+        for language in languages:
+            if not project_files[language]:
+                continue
+            cmd = self.configure_command(language, project_files[language])
+            if not cmd:
+                self._bad_input = True
+                continue
+            if self.options.get("verbose"):
+                click.echo(" ".join(cmd))
+
+            proc.exec_command(
+                cmd,
+                stdout=proc.LineBufferedAsyncPipe(self.on_tool_output),
+                stderr=proc.LineBufferedAsyncPipe(self.on_tool_output),
+            )
+
+        self.clean_up()
+
+        return self._bad_input

platformio/commands/check/tools/pvsstudio.py

@@ -199,32 +199,37 @@ class PvsStudioCheckTool(CheckToolBase):  # pylint: disable=too-many-instance-at
             self._bad_input = True
 
     def clean_up(self):
+        super(PvsStudioCheckTool, self).clean_up()
         if os.path.isdir(self._tmp_dir):
             shutil.rmtree(self._tmp_dir)
 
     def check(self, on_defect_callback=None):
         self._on_defect_callback = on_defect_callback
-        src_files = [
-            f for f in self.get_project_target_files() if not f.endswith((".h", ".hpp"))
-        ]
-
-        for src_file in src_files:
-            self._prepare_preprocessed_file(src_file)
-            cmd = self.configure_command(src_file)
-            if self.options.get("verbose"):
-                click.echo(" ".join(cmd))
-            if not cmd:
-                self._bad_input = True
+        for scope, files in self.get_project_target_files(
+            self.options["patterns"]
+        ).items():
+            if scope not in ("c", "c++"):
                 continue
+            for src_file in files:
+                self._prepare_preprocessed_file(src_file)
+                cmd = self.configure_command(src_file)
+                if self.options.get("verbose"):
+                    click.echo(" ".join(cmd))
+                if not cmd:
+                    self._bad_input = True
+                    continue
 
-            result = proc.exec_command(cmd)
-            # pylint: disable=unsupported-membership-test
-            if result["returncode"] != 0 or "License was not entered" in result["err"]:
-                self._bad_input = True
-                click.echo(result["err"])
-                continue
+                result = proc.exec_command(cmd)
+                # pylint: disable=unsupported-membership-test
+                if (
+                    result["returncode"] != 0
+                    or "license" in result["err"].lower()
+                ):
+                    self._bad_input = True
+                    click.echo(result["err"])
+                    continue
 
-            self._process_defects(self.parse_defects(self._tmp_output_file))
+                self._process_defects(self.parse_defects(self._tmp_output_file))
 
         self.clean_up()
 

platformio/commands/device/filters/send_on_enter.py

@@ -12,8 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import click
-
 from platformio.commands.device import DeviceMonitorFilter
 
 
@@ -25,7 +23,6 @@ class SendOnEnter(DeviceMonitorFilter):
         self._buffer = ""
 
     def tx(self, text):
-        click.echo(text, nl=False)
         self._buffer += text
         if self._buffer.endswith("\r\n"):
             text = self._buffer[:-2]

platformio/managers/core.py

@@ -28,9 +28,9 @@ CORE_PACKAGES = {
     "contrib-pysite": "~2.%d%d.0" % (sys.version_info.major, sys.version_info.minor),
     "tool-unity": "~1.20500.0",
     "tool-scons": "~2.20501.7" if PY2 else "~3.30102.0",
-    "tool-cppcheck": "~1.189.0",
-    "tool-clangtidy": "^1.80000.0",
-    "tool-pvs-studio": "~7.5.0",
+    "tool-cppcheck": "~1.190.0",
+    "tool-clangtidy": "~1.100000.0",
+    "tool-pvs-studio": "~7.7.0",
 }
 
 # pylint: disable=arguments-differ

platformio/project/options.py

@@ -566,6 +566,13 @@ ProjectOptions = OrderedDict(
                 type=click.Choice(["low", "medium", "high"]),
                 default=["low", "medium", "high"],
             ),
+            ConfigEnvOption(
+                group="check",
+                name="check_skip_packages",
+                description="Skip checking includes from packages directory",
+                type=click.BOOL,
+                default=False,
+            ),
             # Test
             ConfigEnvOption(
                 group="test",

tests/commands/test_check.py

@@ -14,6 +14,7 @@
 
 import json
 from os.path import isfile, join
+import sys
 
 import pytest
 
@@ -383,3 +384,47 @@ check_tool = pvs-studio
     assert errors != 0
     assert warnings != 0
     assert style == 0
+
+
+def test_check_embedded_platform_all_tools(clirunner, tmpdir):
+    config = """
+[env:test]
+platform = ststm32
+board = nucleo_f401re
+framework = %s
+check_tool = %s
+"""
+    # tmpdir.join("platformio.ini").write(config)
+    tmpdir.mkdir("src").join("main.c").write(
+        """// This is an open source non-commercial project. Dear PVS-Studio, please check it.
+// PVS-Studio Static Code Analyzer for C, C++, C#, and Java: http://www.viva64.com
+#include <stdlib.h>
+
+void unused_function(int val){
+    int unusedVar = 0;
+    int* iP = &unusedVar;
+    *iP++;
+}
+
+int main() {
+}
+"""
+    )
+
+    frameworks = ["arduino", "mbed", "stm32cube"]
+    if sys.version_info[0] == 3:
+        # Zephyr only supports Python 3
+        frameworks.append("zephyr")
+
+    for framework in frameworks:
+        for tool in ("cppcheck", "clangtidy", "pvs-studio"):
+            tmpdir.join("platformio.ini").write(config % (framework, tool))
+
+            result = clirunner.invoke(cmd_check, ["--project-dir", str(tmpdir)])
+
+            defects = sum(count_defects(result.output))
+
+            assert result.exit_code == 0 and defects > 0, "Failed %s with %s" % (
+                framework,
+                tool,
+            )