diff --git a/pw_presubmit/docs.rst b/pw_presubmit/docs.rst index 9aee1ff797..aa8630fdc5 100644 --- a/pw_presubmit/docs.rst +++ b/pw_presubmit/docs.rst @@ -28,6 +28,8 @@ system. If it's not Bazel formatting passes without checking.) The ``pw_presubmit`` package includes presubmit checks that can be used with any project. These checks include: +.. todo-check: disable + * Check code format of several languages including C, C++, and Python * Initialize a Python environment * Run all Python tests @@ -36,7 +38,15 @@ project. These checks include: * Ensure source files are included in the GN and Bazel builds * Build and run all tests with GN * Build and run all tests with Bazel -* Ensure all header files contain ``#pragma once`` +* Ensure all header files contain ``#pragma once`` (or, that they have matching + ``#ifndef``/``#define`` lines) +* Ensure lists are kept in alphabetical order +* Forbid non-inclusive language +* Check format of TODO lines +* Apply various rules to ``.gitmodules`` or ``OWNERS`` files +* Ensure all source files are in the build + +.. todo-check: enable ------------- Compatibility @@ -342,6 +352,17 @@ There's a ``pragma_once`` check that confirms the first non-comment line of C/C++ headers is ``#pragma once``. This is enabled by adding ``pw_presubmit.cpp_checks.pragma_once`` to a presubmit program. +#ifndef/#define +^^^^^^^^^^^^^^^ +There's an ``ifndef_guard`` check that confirms the first two non-comment lines +of C/C++ headers are ``#ifndef HEADER_H`` and ``#define HEADER_H``. This is +enabled by adding ``pw_presubmit.cpp_checks.include_guard_check()`` to a +presubmit program. ``include_guard_check()`` has options for specifying what the +header guard should be based on the path. + +This check is not used in Pigweed itself but is available to projects using +Pigweed. + .. todo-check: disable TODO(b/###) Formatting diff --git a/pw_presubmit/py/BUILD.gn b/pw_presubmit/py/BUILD.gn index e080753575..e928e061c9 100644 --- a/pw_presubmit/py/BUILD.gn +++ b/pw_presubmit/py/BUILD.gn @@ -49,6 +49,7 @@ pw_python_package("py") { ] tests = [ "bazel_parser_test.py", + "cpp_checks_test.py", "git_repo_test.py", "gitmodules_test.py", "keep_sorted_test.py", diff --git a/pw_presubmit/py/cpp_checks_test.py b/pw_presubmit/py/cpp_checks_test.py new file mode 100644 index 0000000000..77e4482271 --- /dev/null +++ b/pw_presubmit/py/cpp_checks_test.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +# Copyright 2023 The Pigweed Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""Tests for cpp_checks.""" + +import os +from pathlib import Path +import unittest +from unittest.mock import MagicMock, mock_open, patch + +from pw_presubmit import cpp_checks + +# pylint: disable=attribute-defined-outside-init + + +def _temproot(): + root = Path(os.environ['_PW_ACTUAL_ENVIRONMENT_ROOT']) / 'temp' + root.mkdir(exist_ok=True) + return root + + +class TestPragmaOnce(unittest.TestCase): + """Test pragma_once check.""" + + def _run(self, contents: str) -> None: + self.ctx = MagicMock() + self.ctx.fail = MagicMock() + self.ctx.format_options.filter_paths = lambda x: x + path = MagicMock(spec=Path('include/foo.h')) + + def mocked_open_read(*args, **kwargs): + return mock_open(read_data=contents)(*args, **kwargs) + + with patch.object(path, 'open', mocked_open_read): + self.ctx.paths = [path] + cpp_checks.pragma_once(self.ctx) + + def test_empty(self) -> None: + self._run('') + self.ctx.fail.assert_called() + + def test_simple(self) -> None: + self._run('#pragma once') + self.ctx.fail.assert_not_called() + + def test_not_first(self) -> None: + self._run('1\n2\n3\n#pragma once') + self.ctx.fail.assert_not_called() + + def test_different_pragma(self) -> None: + self._run('#pragma thrice') + self.ctx.fail.assert_called() + + +def guard(path: Path) -> str: + return str(path.name).replace('.', '_').upper() + + +class TestIncludeGuard(unittest.TestCase): + """Test pragma_once check.""" + + def _run(self, contents: str, **kwargs) -> None: + self.ctx = MagicMock() + self.ctx.format_options.filter_paths = lambda x: x + self.ctx.fail = MagicMock() + path = MagicMock(spec=Path('abc/def/foo.h')) + path.name = 'foo.h' + + def mocked_open_read(*args, **kwargs): + return mock_open(read_data=contents)(*args, **kwargs) + + with patch.object(path, 'open', mocked_open_read): + self.ctx.paths = [path] + check = cpp_checks.include_guard_check(**kwargs) + check(self.ctx) + + def test_empty(self) -> None: + self._run('') + self.ctx.fail.assert_called() + + def test_simple(self) -> None: + self._run('#ifndef FOO_H\n#define FOO_H', guard=guard) + self.ctx.fail.assert_not_called() + + def test_simple_any(self) -> None: + self._run('#ifndef BAR_H\n#define BAR_H') + self.ctx.fail.assert_not_called() + + def test_simple_comments(self) -> None: + self._run('// abc\n#ifndef BAR_H\n// def\n#define BAR_H\n// ghi') + self.ctx.fail.assert_not_called() + + def test_simple_whitespace(self) -> None: + self._run( + ' // abc\n #ifndef BAR_H\n// def\n' + ' #define BAR_H\n// ghi\n #endif' + ) + self.ctx.fail.assert_not_called() + + def test_simple_not_expected(self) -> None: + self._run('#ifnef BAR_H\n#define BAR_H', guard=guard) + self.ctx.fail.assert_called() + + def test_no_define(self) -> None: + self._run('#ifndef FOO_H') + self.ctx.fail.assert_called() + + def test_non_matching_define(self) -> None: + self._run('#ifndef FOO_H\n#define BAR_H') + self.ctx.fail.assert_called() + + +if __name__ == '__main__': + unittest.main() diff --git a/pw_presubmit/py/pw_presubmit/cpp_checks.py b/pw_presubmit/py/pw_presubmit/cpp_checks.py index 66446d92da..164065004a 100644 --- a/pw_presubmit/py/pw_presubmit/cpp_checks.py +++ b/pw_presubmit/py/pw_presubmit/cpp_checks.py @@ -14,6 +14,9 @@ """C++-related checks.""" import logging +from pathlib import Path +import re +from typing import Callable, Optional, Iterable, Iterator from pw_presubmit.presubmit import ( Check, @@ -37,7 +40,7 @@ def pragma_once(ctx: PresubmitContext) -> None: for path in ctx.paths: _LOG.debug('Checking %s', path) - with open(path) as file: + with path.open() as file: for line in file: if line.startswith('#pragma once'): break @@ -45,6 +48,103 @@ def pragma_once(ctx: PresubmitContext) -> None: ctx.fail('#pragma once is missing!', path=path) +def include_guard_check( + guard: Optional[Callable[[Path], str]] = None, + allow_pragma_once: bool = True, +) -> Check: + """Create an include guard check. + + Args: + guard: Callable that generates an expected include guard name for the + given Path. If None, any include guard is acceptable, as long as + it's consistent between the '#ifndef' and '#define' lines. + allow_pragma_once: Whether to allow headers to use '#pragma once' + instead of '#ifndef'/'#define'. + """ + + def stripped_non_comment_lines(iterable: Iterable[str]) -> Iterator[str]: + """Yield non-comment non-empty lines from a C++ file.""" + multi_line_comment = False + for line in iterable: + line = line.strip() + if not line: + continue + if line.startswith('//'): + continue + if line.startswith('/*'): + multi_line_comment = True + if multi_line_comment: + if line.endswith('*/'): + multi_line_comment = False + continue + yield line + + def check_path(ctx: PresubmitContext, path: Path) -> None: + """Check if path has a valid include guard.""" + + _LOG.debug('checking %s', path) + expected: Optional[str] = None + if guard: + expected = guard(path) + _LOG.debug('expecting guard %r', expected) + + with path.open() as ins: + iterable = stripped_non_comment_lines(ins) + first_line = next(iterable, '') + _LOG.debug('first line %r', first_line) + + if allow_pragma_once and first_line.startswith('#pragma once'): + _LOG.debug('found %r', first_line) + return + + if expected: + ifndef_expected = f'#ifndef {expected}' + if not re.match(rf'^#ifndef {expected}$', first_line): + ctx.fail( + 'Include guard is missing! Expected: ' + f'{ifndef_expected!r}, Found: {first_line!r}', + path=path, + ) + return + + else: + match = re.match( + r'^#\s*ifndef\s+([a-zA-Z_][a-zA-Z_0-9]*)$', + first_line, + ) + if not match: + ctx.fail( + 'Include guard is missing! Expected "#ifndef" line, ' + f'Found: {first_line!r}', + path=path, + ) + return + expected = match.group(1) + + second_line = next(iterable, '') + _LOG.debug('second line %r', second_line) + + if not re.match(rf'^#\s*define\s+{expected}$', second_line): + define_expected = f'#define {expected}' + ctx.fail( + 'Include guard is missing! Expected: ' + f'{define_expected!r}, Found: {second_line!r}', + path=path, + ) + return + + _LOG.debug('passed') + + @filter_paths(endswith=format_code.CPP_HEADER_EXTS, exclude=(r'\.pb\.h$',)) + def include_guard(ctx: PresubmitContext): + """Check that all header files contain an include guard.""" + ctx.paths = presubmit_context.apply_exclusions(ctx) + for path in ctx.paths: + check_path(ctx, path) + + return include_guard + + @Check def asan(ctx: PresubmitContext) -> None: """Test with the address sanitizer.""" diff --git a/pw_presubmit/py/pw_presubmit/format_code.py b/pw_presubmit/py/pw_presubmit/format_code.py index 36778221bb..c7c75e44a5 100755 --- a/pw_presubmit/py/pw_presubmit/format_code.py +++ b/pw_presubmit/py/pw_presubmit/format_code.py @@ -604,18 +604,6 @@ def extensions(self): CODE_FORMATS_WITH_YAPF: Tuple[CodeFormat, ...] = CODE_FORMATS -def _filter_paths( - paths: Iterable[Path], - filters: Sequence[re.Pattern], -) -> Tuple[Path, ...]: - root = Path(pw_cli.env.pigweed_environment().PW_PROJECT_ROOT) - relpaths = [x.relative_to(root) for x in paths] - - for filt in filters: - relpaths = [x for x in relpaths if not filt.search(str(x))] - return tuple(root / x for x in relpaths) - - def presubmit_check( code_format: CodeFormat, *, @@ -692,9 +680,7 @@ def __init__( self.package_root = package_root or output_dir / 'packages' self._format_options = FormatOptions.load() raw_paths = files - self.paths: Tuple[Path, ...] = _filter_paths( - files, self._format_options.exclude - ) + self.paths: Tuple[Path, ...] = self._format_options.filter_paths(files) filtered_paths = set(raw_paths) - set(self.paths) for path in sorted(filtered_paths): diff --git a/pw_presubmit/py/pw_presubmit/presubmit_context.py b/pw_presubmit/py/pw_presubmit/presubmit_context.py index b11e9fad1d..9c7b69ab12 100644 --- a/pw_presubmit/py/pw_presubmit/presubmit_context.py +++ b/pw_presubmit/py/pw_presubmit/presubmit_context.py @@ -40,6 +40,7 @@ import urllib import pw_cli.color +import pw_cli.env import pw_env_setup.config_file if TYPE_CHECKING: @@ -69,6 +70,14 @@ def load(env: Optional[Dict[str, str]] = None) -> 'FormatOptions': exclude=tuple(re.compile(x) for x in fmt.get('exclude', ())), ) + def filter_paths(self, paths: Iterable[Path]) -> Tuple[Path, ...]: + root = Path(pw_cli.env.pigweed_environment().PW_PROJECT_ROOT) + relpaths = [x.relative_to(root) for x in paths] + + for filt in self.exclude: + relpaths = [x for x in relpaths if not filt.search(str(x))] + return tuple(root / x for x in relpaths) + def get_buildbucket_info(bbid) -> Dict[str, Any]: if not bbid or not shutil.which('bb'): @@ -542,9 +551,4 @@ def apply_exclusions( ctx: PresubmitContext, paths: Optional[Sequence[Path]] = None, ) -> Tuple[Path, ...]: - root = Path(pw_cli.env.pigweed_environment().PW_PROJECT_ROOT) - relpaths = [x.relative_to(root) for x in paths or ctx.paths] - - for filt in ctx.format_options.exclude: - relpaths = [x for x in relpaths if not filt.search(str(x))] - return tuple(root / x for x in relpaths) + return ctx.format_options.filter_paths(paths or ctx.paths)