Disallow EXPECT THROW and ASSERT THROW/issue 126

.vscode/settings.json

@@ -5,5 +5,34 @@
         "levelname",
         "MSTD",
         "unittests"
-    ]
+    ],
+    "workbench.colorCustomizations": {
+        "activityBar.activeBackground": "#ab307e",
+        "activityBar.activeBorder": "#e7e7e7",
+        "activityBar.background": "#ab307e",
+        "activityBar.foreground": "#e7e7e7",
+        "activityBar.inactiveForeground": "#e7e7e799",
+        "activityBarBadge.background": "#25320e",
+        "activityBarBadge.foreground": "#e7e7e7",
+        "activityBarTop.activeBackground": "#ab307e",
+        "activityBarTop.activeBorder": "#e7e7e7",
+        "activityBarTop.background": "#ab307e",
+        "activityBarTop.foreground": "#e7e7e7",
+        "activityBarTop.inactiveForeground": "#e7e7e799",
+        "commandCenter.border": "#e7e7e799",
+        "commandCenter.foreground": "#e7e7e7",
+        "sash.hoverBorder": "#ab307e",
+        "statusBar.background": "#832561",
+        "statusBar.debuggingBackground": "#832561",
+        "statusBar.debuggingForeground": "#e7e7e7",
+        "statusBar.foreground": "#e7e7e7",
+        "statusBarItem.hoverBackground": "#ab307e",
+        "statusBarItem.remoteBackground": "#121907",
+        "statusBarItem.remoteForeground": "#e7e7e7",
+        "titleBar.activeBackground": "#832561",
+        "titleBar.activeForeground": "#e7e7e7",
+        "titleBar.inactiveBackground": "#83256199",
+        "titleBar.inactiveForeground": "#e7e7e799"
+    },
+    "peacock.remoteColor": "#832561"
 }

CHANGELOG.md

@@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file.
 
 ## Next Release
 
+### Features
+
+#### CPP Rules
+
+- Add `macroReplacement` AST check flagging invocations of a banned macro and suggesting its replacement, e.g. disallowing gtest's `EXPECT_THROW`/`ASSERT_THROW` in favor of custom `EXPECT_THROW_MSG`/`ASSERT_THROW_MSG` macros that also require a failure message. Ships with that mapping as a built-in default (no configuration required) and is fully configurable/extensible via `[cpp.ast_check_config.macroReplacement].macro_to_replacement`. Detection matches the macro name exactly and works whether the macro is defined in the same file or an included header
+
+### Fixes
+
+#### CPP Rules
+
+- Fix a libclang crash (`astParseError` with no useful diagnostic) when parsing a file whose compile args force-include a PCH header via the `-include <file>` compiler flag (as kept by the earlier `-include`-handling fix). Pip's libclang build has been observed to hard-crash on some real PCH headers when force-included this way — even though the exact same header content parses cleanly as an ordinary `#include`, and even though the project's own real compiler accepts the identical flag without issue. `-include <file>` is now stripped from the compiler args and instead turned into a real `#include` line ahead of the checked file in a synthetic wrapper (the same technique already used for header checks), which avoids the crash while preserving the checked file's own path and line numbers exactly
+
 <!-- insertion marker -->
 ## [0.4.2](https://github.com/repo/owner/releases/tag/0.4.2) - 2026-09-26
 

docs/source/sections/configuration.rst

@@ -376,6 +376,52 @@ with::
    [cpp]
    ast_check_disabled_ids = ["noThrowParen"]
 
+.. rubric:: ``macroReplacement``
+
+Flags invocations of a banned macro and suggests the replacement macro that
+should be used instead. Ships with a built-in default mapping and requires no
+configuration to use:
+
+.. list-table::
+   :header-rows: 1
+   :widths: 30 30
+
+   * - Banned macro
+     - Use instead
+   * - ``EXPECT_THROW``
+     - ``EXPECT_THROW_MSG``
+   * - ``ASSERT_THROW``
+     - ``ASSERT_THROW_MSG``
+
+Detection matches the macro name exactly (``EXPECT_THROW`` never matches
+``EXPECT_THROW_MSG``) and works regardless of whether the macro is defined in
+the same file or an included header (e.g. a gtest header).
+
+.. list-table::
+   :header-rows: 1
+   :widths: 20 15 15 50
+
+   * - Key
+     - Type
+     - Default
+     - Description
+   * - ``macro_to_replacement``
+     - table of string → string
+     - the built-in mapping above
+     - Replaces the built-in mapping entirely when set — extend it by
+       re-listing the defaults you want to keep alongside your own.
+
+.. code-block:: toml
+
+   [cpp.ast_check_config.macroReplacement]
+   macro_to_replacement = { EXPECT_THROW = "EXPECT_THROW_MSG",
+                            ASSERT_THROW = "ASSERT_THROW_MSG" }
+
+To disable this check for a project set::
+
+   [cpp]
+   ast_check_disabled_ids = ["macroReplacement"]
+
 ``[file]``
 ^^^^^^^^^^
 

src/devops/config/config_cpp.py

@@ -148,6 +148,9 @@ def to_toml_lines(self) -> list[str]:
             '#disabled_names = ["std::literals"]\n'
             "#[cpp.ast_check_config.noGlobalUsingEnum]\n"
             '#disabled_names = ["molsys::LegacyZone"]\n'
+            "#[cpp.ast_check_config.macroReplacement]\n"
+            '#macro_to_replacement = { EXPECT_THROW = "EXPECT_THROW_MSG", '
+            'ASSERT_THROW = "ASSERT_THROW_MSG" }\n'
         )
 
         isf = (

src/devops/cpp/ast/checks/macro_replacement.py

@@ -0,0 +1,121 @@
+"""Flag banned macro invocations and suggest their replacement.
+
+Any macro invocation whose name matches a configured key in
+``macro_to_replacement`` is reported, naming the replacement macro that
+should be used instead. Built for cases like disallowing gtest's
+``EXPECT_THROW``/``ASSERT_THROW`` in favor of custom
+``EXPECT_THROW_MSG``/``ASSERT_THROW_MSG`` macros that also require a
+failure message, but works for any macro pair.
+
+Detection is based on libclang's ``MACRO_INSTANTIATION`` cursor (available
+because the AST engine always parses with
+``PARSE_DETAILED_PROCESSING_RECORD``), so it works regardless of whether the
+macro is defined in the same file or an included header, and it matches the
+macro name exactly — ``EXPECT_THROW`` never accidentally matches
+``EXPECT_THROW_MSG``.
+
+Ships with a built-in default mapping and requires no configuration to use::
+
+    EXPECT_THROW -> EXPECT_THROW_MSG
+    ASSERT_THROW -> ASSERT_THROW_MSG
+
+Replace or extend it via::
+
+    [cpp.ast_check_config.macroReplacement]
+    macro_to_replacement = { EXPECT_THROW = "EXPECT_THROW_MSG",
+                             ASSERT_THROW = "ASSERT_THROW_MSG" }
+
+To disable this check for a project set::
+
+    [cpp]
+    ast_check_disabled_ids = ["macroReplacement"]
+"""
+
+from __future__ import annotations
+
+import clang.cindex as clang
+
+from devops.config.base import ConfigError
+from devops.cpp.ast.base import Check, Diagnostic
+
+_DEFAULT_MACRO_TO_REPLACEMENT = {
+    "EXPECT_THROW": "EXPECT_THROW_MSG",
+    "ASSERT_THROW": "ASSERT_THROW_MSG",
+}
+
+
+class MacroReplacement(Check):
+    """Flag invocations of a banned macro and suggest its replacement."""
+
+    id = "macroReplacement"
+
+    def __init__(self) -> None:
+        """Initialize with the built-in default macro-to-replacement mapping."""
+        self.macro_to_replacement: dict[str, str] = dict(_DEFAULT_MACRO_TO_REPLACEMENT)
+
+    def configure(self, config: dict) -> None:
+        """Replace the default macro-to-replacement mapping from the TOML config.
+
+        Parameters
+        ----------
+        config: dict
+            Expected shape:
+            ``{"macro_to_replacement": {"OLD_MACRO": "NEW_MACRO", ...}}``.
+            When ``macro_to_replacement`` is absent, the built-in default
+            mapping is kept unchanged.
+
+        Raises
+        ------
+        ConfigError
+            If ``macro_to_replacement`` is present but has an invalid shape.
+
+        """
+        if "macro_to_replacement" not in config:
+            return
+        raw = config["macro_to_replacement"]
+        if not isinstance(raw, dict) or not all(
+            isinstance(k, str) and isinstance(v, str) for k, v in raw.items()
+        ):
+            msg = (
+                "macroReplacement: 'macro_to_replacement' must be a table of "
+                "string -> string mappings"
+            )
+            raise ConfigError(msg)
+        self.macro_to_replacement = dict(raw)
+
+    def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]:
+        """Flag the cursor if it is an invocation of a banned macro.
+
+        Parameters
+        ----------
+        cursor: clang.Cursor
+            The AST node currently being visited.
+        filename: str
+            Path of the file being checked.
+
+        Returns
+        -------
+        list[Diagnostic]
+            A single diagnostic if `cursor` is a banned macro invocation,
+            otherwise an empty list.
+
+        """
+        if cursor.kind != clang.CursorKind.MACRO_INSTANTIATION:
+            return []
+
+        name = cursor.spelling
+        replacement = self.macro_to_replacement.get(name)
+        if replacement is None:
+            return []
+
+        loc = cursor.location
+        return [
+            Diagnostic(
+                file=filename,
+                line=loc.line,
+                column=loc.column,
+                message=f"do not use '{name}' — use '{replacement}' instead",
+                check_id=self.id,
+                severity="style",
+            )
+        ]

src/devops/cpp/ast/engine.py

@@ -120,6 +120,45 @@ def _with_fallback_resource_dir(args: list[str]) -> list[str]:
     return _with_auto_resource_dir(args, clang_exe)
 
 
+def _extract_plain_include(compile_args: list[str]) -> tuple[list[str], str | None]:
+    """Pull a bare ``-include <file>`` pair out of `compile_args`, if present.
+
+    `rule.py` normalizes any force-include it decides to keep (CMake PCH
+    headers, in both GCC's bare and Clang's ``-Xclang``-wrapped spellings)
+    to exactly this bare pair, so that's the only shape handled here.
+
+    Passing that file to libclang as the ``-include`` compiler flag is
+    avoided: pip's libclang build has been observed to hard-crash
+    (``TranslationUnitLoadError``, no diagnostics at all) on some real PCH
+    headers when force-included this way, even though the exact same
+    header parses cleanly as an ordinary ``#include`` — and even though
+    the project's own real compiler parses the identical flag with no
+    problem. The caller turns the extracted file into a real ``#include``
+    line instead (see `run_ast_checks`), which sidesteps the crash and
+    parses correctly.
+
+    Parameters
+    ----------
+    compile_args: list[str]
+        The compile args to search.
+
+    Returns
+    -------
+    tuple[list[str], str | None]
+        `compile_args` with the ``-include <file>`` pair removed (or
+        unchanged, if none was found) and the extracted file path (or
+        None).
+
+    """
+    if "-include" not in compile_args:
+        return compile_args, None
+    idx = compile_args.index("-include")
+    file_arg = compile_args[idx + 1] if idx + 1 < len(compile_args) else None
+    if file_arg is None:
+        return compile_args, None
+    return compile_args[:idx] + compile_args[idx + 2 :], file_arg
+
+
 def run_ast_checks(
     path: Path,
     content: str,
@@ -152,6 +191,7 @@ def run_ast_checks(
     """
     checks = ALL_CHECKS if checks is None else checks
     compile_args = _with_fallback_resource_dir(compile_args)
+    compile_args, prelude_include = _extract_plain_include(compile_args)
 
     # Use the display/diagnostic path as-is; resolve to absolute for libclang
     # so that unsaved-file lookup and AST node locations are consistent.
@@ -166,15 +206,23 @@ def run_ast_checks(
     parse_options = clang.TranslationUnit.PARSE_DETAILED_PROCESSING_RECORD
 
     index = clang.Index.create()
-    if is_header:
-        # Parse a virtual .cpp wrapper that #includes the header so that
-        # libclang gets a proper translation-unit context.  Parsing a header
-        # directly often causes TranslationUnitLoadError because libclang
-        # expects a complete translation unit as its entry point.
+    if is_header or prelude_include is not None:
+        # Parse a virtual .cpp wrapper that #includes the file(s) so that
+        # libclang gets a proper translation-unit context, rather than
+        # passing them as compiler flags. For a header, parsing it directly
+        # often causes TranslationUnitLoadError because libclang expects a
+        # complete translation unit as its entry point. For a force-included
+        # PCH header (`prelude_include`), passing it via the `-include`
+        # compiler flag instead of an ordinary #include has been observed to
+        # crash libclang outright on some real headers (see
+        # `_extract_plain_include`). Either way, the real file is registered
+        # under its own real path with its own unmodified content, so
+        # per-file line numbers and locations are unaffected by the wrapper.
         # Use absolute paths so libclang's internal path resolution can match
         # our unsaved-file entries (it normalises to absolute before lookup).
-        wrapper_name = str(Path.cwd() / "__devops_ast_header_check__.cpp")
-        wrapper_content = f'#include "{filename_abs}"\n'
+        wrapper_name = str(Path.cwd() / "__devops_ast_wrapper__.cpp")
+        prelude = f'#include "{prelude_include}"\n' if prelude_include else ""
+        wrapper_content = f'{prelude}#include "{filename_abs}"\n'
         unsaved = [(filename_abs, content), (wrapper_name, wrapper_content)]
         parse_name = wrapper_name
         filter_name = filename_abs

src/devops/cpp/ast/registry.py

@@ -10,6 +10,7 @@
 import typing
 
 from devops.cpp.ast.checks.enforce_param_name_for_type import EnforceParamNameForType
+from devops.cpp.ast.checks.macro_replacement import MacroReplacement
 from devops.cpp.ast.checks.no_global_using import NoGlobalUsing
 from devops.cpp.ast.checks.no_global_using_enum import NoGlobalUsingEnum
 from devops.cpp.ast.checks.no_throw_paren import NoThrowParen
@@ -24,6 +25,7 @@
 # / ast_check_disabled_ids to turn it on or off from the input file.
 ALL_CHECKS: list[Check] = [
     EnforceParamNameForType(),
+    MacroReplacement(),
     NoGlobalUsing(),
     NoGlobalUsingEnum(),
     NoThrowParen(),