Implement ast checker strategy to enforece leading underscore for private and protected member variables/issue 131

CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 
 ## Next Release
 
+### Features
+
+#### CPP Rules
+
+- Add `memberLeadingUnderscore` AST check flagging private and protected member variables (including static ones) whose name doesn't start with `_`, e.g. `int count;` under `private:`/`protected:` should be `int _count;`. Public members are never checked. Members synthesized entirely by a macro invoked on the same source line (e.g. gtest's `TEST_F(...)` expanding to a fixture class with its own private `test_info_` member) are excluded, since there is no user-typed name to rename. Takes no configuration
+
 ### Documentation
 
 - Add a dedicated "AST-Based C++ Checks" section to the Sphinx docs with an overview table of every AST check and its own sidebar-linked page per check (`paramNameForType`, `macroReplacement`, `noGlobalUsing`, `noGlobalUsingEnum`, `noThrowParen`), each with flagged-code examples and its full configuration reference. The scattered/partial AST-check descriptions previously duplicated across the overview and configuration pages now point to these pages instead

docs/source/sections/ast_checks/enforce_member_leading_underscore.rst

@@ -0,0 +1,39 @@
+memberLeadingUnderscore
+========================
+
+Flags any non-static or static data member declared under a ``private:`` or
+``protected:`` access specifier whose name doesn't start with ``_``. Public
+members are never flagged.
+
+.. code-block:: cpp
+
+   class Widget {
+   public:
+       int size;      // Good — public members are not checked.
+
+   private:
+       int count;      // Bad — flagged, missing leading underscore.
+       int _count;      // Good.
+
+   protected:
+       int flag;        // Bad — flagged, missing leading underscore.
+       int _flag;        // Good.
+   };
+
+A member synthesized entirely by a macro invoked on that same source line
+(e.g. gtest's ``TEST_F(...)`` expanding to a fixture class with its own
+private ``test_info_`` member) is not flagged — there is no user-typed name
+to rename.
+
+Configuration
+--------------
+
+This check takes no configuration.
+
+Disabling
+----------
+
+.. code-block:: toml
+
+   [cpp]
+   ast_check_disabled_ids = ["memberLeadingUnderscore"]

docs/source/sections/ast_checks/index.rst

@@ -64,6 +64,9 @@ Available checks
    * - Check id
      - Flags
      - Configuration
+   * - :doc:`memberLeadingUnderscore <enforce_member_leading_underscore>`
+     - A private or protected member variable not starting with ``_``.
+     - None
    * - :doc:`paramNameForType <param_name_for_type>`
      - A parameter of a configured type not using its canonical name.
      - Required (``type_to_name``)
@@ -84,6 +87,7 @@ Available checks
    :maxdepth: 1
    :hidden:
 
+   enforce_member_leading_underscore
    param_name_for_type
    macro_replacement
    no_global_using

docs/source/sections/configuration.rst

@@ -216,8 +216,9 @@ Controls the checks run by :ref:`cpp_checks <cli-cpp_checks>`.
      - list of strings
      - ``[]``
      - If non-empty, only AST checks whose ``id`` is in this list run
-       (allowlist). Available check ids: ``paramNameForType``,
-       ``noGlobalUsing``, ``noGlobalUsingEnum`` (each documented below).
+       (allowlist). Available check ids: ``memberLeadingUnderscore``,
+       ``paramNameForType``, ``macroReplacement``, ``noGlobalUsing``,
+       ``noGlobalUsingEnum``, ``noThrowParen`` (each documented below).
    * - ``ast_check_disabled_ids``
      - list of strings
      - ``[]``
@@ -266,8 +267,9 @@ Controls the checks run by :ref:`cpp_checks <cli-cpp_checks>`.
 Per-check configuration lives in sub-tables of ``[cpp.ast_check_config]``,
 one sub-table per check id. Each check defines its own keys; unknown keys are
 silently ignored. See :doc:`ast_checks/index` for what each check flags and
-its full configuration reference (``paramNameForType``, ``macroReplacement``,
-``noGlobalUsing``, ``noGlobalUsingEnum``, ``noThrowParen``).
+its full configuration reference (``memberLeadingUnderscore``,
+``paramNameForType``, ``macroReplacement``, ``noGlobalUsing``,
+``noGlobalUsingEnum``, ``noThrowParen``).
 
 ``[file]``
 ^^^^^^^^^^

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

@@ -0,0 +1,155 @@
+"""Enforce a leading underscore on private and protected member variables.
+
+Any non-static or static data member declared under a ``private:`` or
+``protected:`` access specifier must have a name starting with ``_``
+(e.g. ``_count``, ``_isValid``). Public members are never flagged, since
+they are part of the class's external interface and not the concern of
+this check.
+
+Member variables synthesized by a macro invoked on the same source line
+(e.g. gtest's ``TEST_F(...)`` expanding to a fixture class with its own
+private ``test_info_`` member, declared entirely inside gtest's macro body)
+are not flagged: the name was never actually typed by the user, so there is
+nothing for them to rename.
+
+To disable this check for a project set::
+
+    [cpp]
+    ast_check_disabled_ids = ["memberLeadingUnderscore"]
+"""
+
+from __future__ import annotations
+
+import typing
+
+import clang.cindex as clang
+
+from devops.cpp.ast.base import Check, Diagnostic
+
+# Cursor kinds of a record whose direct data members this check inspects.
+_RECORD_KINDS = frozenset(
+    (
+        clang.CursorKind.CLASS_DECL,
+        clang.CursorKind.STRUCT_DECL,
+        clang.CursorKind.UNION_DECL,
+        clang.CursorKind.CLASS_TEMPLATE,
+        clang.CursorKind.CLASS_TEMPLATE_PARTIAL_SPECIALIZATION,
+    )
+)
+
+# FIELD_DECL covers non-static data members; a static data member is a
+# VAR_DECL whose semantic parent is the record itself (filtered via
+# _RECORD_KINDS below, since VAR_DECL also covers ordinary local/global
+# variables).
+_MEMBER_KINDS = frozenset((clang.CursorKind.FIELD_DECL, clang.CursorKind.VAR_DECL))
+
+_RESTRICTED_ACCESS = frozenset(
+    (clang.AccessSpecifier.PRIVATE, clang.AccessSpecifier.PROTECTED)
+)
+
+
+class _PendingMember(typing.NamedTuple):
+    """A candidate diagnostic buffered until end-of-file macro info is known."""
+
+    name: str
+    access: str
+    line: int
+    column: int
+
+
+class EnforceMemberLeadingUnderscore(Check):
+    """Flag private/protected member variables without a leading underscore.
+
+    Reporting is deferred to `finalize()` because whether a candidate member
+    was synthesized by a macro (and should be skipped) can only be known
+    once every ``MACRO_INSTANTIATION`` cursor in the file has been seen —
+    which, in a single preorder walk, may happen before or after the member
+    itself is visited.
+    """
+
+    id = "memberLeadingUnderscore"
+
+    def __init__(self) -> None:
+        """Initialise with empty per-file buffers."""
+        self._macro_lines: dict[str, set[int]] = {}
+        self._pending: dict[str, list[_PendingMember]] = {}
+
+    def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]:
+        """Record macro-instantiation lines and candidate members for `filename`.
+
+        Parameters
+        ----------
+        cursor: clang.Cursor
+            The AST node currently being visited.
+        filename: str
+            Path of the file being checked.
+
+        Returns
+        -------
+        list[Diagnostic]
+            Always empty — diagnostics are emitted from `finalize()` once
+            the whole file (including its macro instantiations) is known.
+
+        """
+        if cursor.kind == clang.CursorKind.MACRO_INSTANTIATION:
+            lines = self._macro_lines.setdefault(filename, set())
+            lines.update(range(cursor.extent.start.line, cursor.extent.end.line + 1))
+            return []
+
+        if cursor.kind not in _MEMBER_KINDS:
+            return []
+
+        if cursor.access_specifier not in _RESTRICTED_ACCESS:
+            return []
+
+        parent = cursor.semantic_parent
+        if parent is None or parent.kind not in _RECORD_KINDS:
+            return []
+
+        name = cursor.spelling
+        if not name or name.startswith("_"):
+            return []
+
+        access = (
+            "private"
+            if cursor.access_specifier == clang.AccessSpecifier.PRIVATE
+            else "protected"
+        )
+        loc = cursor.location
+        self._pending.setdefault(filename, []).append(
+            _PendingMember(name, access, loc.line, loc.column)
+        )
+        return []
+
+    def finalize(self, filename: str) -> list[Diagnostic]:
+        """Emit diagnostics for buffered members not on a macro-instantiation line.
+
+        Parameters
+        ----------
+        filename: str
+            Path of the file being checked.
+
+        Returns
+        -------
+        list[Diagnostic]
+            One diagnostic per offending member variable whose line isn't
+            covered by a macro instantiation.
+
+        """
+        macro_lines = self._macro_lines.pop(filename, set())
+        pending = self._pending.pop(filename, [])
+        return [
+            Diagnostic(
+                file=filename,
+                line=member.line,
+                column=member.column,
+                message=(
+                    f"{member.access} member variable '{member.name}' should "
+                    f"start with a leading underscore ('_{member.name}')"
+                ),
+                check_id=self.id,
+                severity="style",
+            )
+            for member in pending
+            if member.line not in macro_lines
+        ]

src/devops/cpp/ast/registry.py

@@ -9,6 +9,9 @@
 import copy
 import typing
 
+from devops.cpp.ast.checks.enforce_member_leading_underscore import (
+    EnforceMemberLeadingUnderscore,
+)
 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
@@ -24,6 +27,7 @@
 # Each check's `.id` is the identifier used in CppConfig.ast_check_enabled_ids
 # / ast_check_disabled_ids to turn it on or off from the input file.
 ALL_CHECKS: list[Check] = [
+    EnforceMemberLeadingUnderscore(),
     EnforceParamNameForType(),
     MacroReplacement(),
     NoGlobalUsing(),