pw_presubmit: Add ifndef/define check

Also add tests for pragma_once, and make some minor changes that make
testing easier.

Bug: b/287529705
Change-Id: I9d1c408f5fc829362fca41c530f15a64448dc71b
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/152173
Reviewed-by: Wyatt Hepler <[email protected]>
Commit-Queue: Rob Mohr <[email protected]>
Reviewed-by: Jonathon Reinhart <[email protected]>

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

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",

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()

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,14 +40,111 @@ 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
             else:
                 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."""

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):

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)