diff --git a/CHANGELOG.md b/CHANGELOG.md index cd615ea..5774f17 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/source/sections/ast_checks/enforce_member_leading_underscore.rst b/docs/source/sections/ast_checks/enforce_member_leading_underscore.rst new file mode 100644 index 0000000..4d7e5cc --- /dev/null +++ b/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"] diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index 4776a7e..aa40396 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -64,6 +64,9 @@ Available checks * - Check id - Flags - Configuration + * - :doc:`memberLeadingUnderscore ` + - A private or protected member variable not starting with ``_``. + - None * - :doc:`paramNameForType ` - 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 diff --git a/docs/source/sections/configuration.rst b/docs/source/sections/configuration.rst index a5601df..888d373 100644 --- a/docs/source/sections/configuration.rst +++ b/docs/source/sections/configuration.rst @@ -216,8 +216,9 @@ Controls the checks run by :ref:`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 `. 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]`` ^^^^^^^^^^ diff --git a/src/devops/cpp/ast/checks/enforce_member_leading_underscore.py b/src/devops/cpp/ast/checks/enforce_member_leading_underscore.py new file mode 100644 index 0000000..a84b1d2 --- /dev/null +++ b/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 + ] diff --git a/src/devops/cpp/ast/registry.py b/src/devops/cpp/ast/registry.py index f3fd06b..c557816 100644 --- a/src/devops/cpp/ast/registry.py +++ b/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(), diff --git a/tests/cpp/ast/test_enforce_member_leading_underscore.py b/tests/cpp/ast/test_enforce_member_leading_underscore.py new file mode 100644 index 0000000..0222e8b --- /dev/null +++ b/tests/cpp/ast/test_enforce_member_leading_underscore.py @@ -0,0 +1,168 @@ +"""Tests for the EnforceMemberLeadingUnderscore AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.enforce_member_leading_underscore import ( + EnforceMemberLeadingUnderscore, +) +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [EnforceMemberLeadingUnderscore()] +_ARGS = ["-std=c++17"] + + +def _diags(code: str, tmp_path: Path) -> list[str]: + p = tmp_path / "test.cpp" + p.write_text(code) + return [d.message for d in run_ast_checks(p, code, _ARGS, checks=_CHECK)] + + +class TestPrivateMembersFlagged: + """Private data members without a leading underscore are flagged.""" + + def test_private_field_flagged(self, tmp_path: Path) -> None: + """Test private field flagged.""" + code = "class C {\nprivate:\n int count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "private member variable 'count'" in diags[0] + assert "'_count'" in diags[0] + + def test_private_static_field_flagged(self, tmp_path: Path) -> None: + """Test private static field flagged.""" + code = "class C {\nprivate:\n static int count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "count" in diags[0] + + def test_private_struct_field_flagged(self, tmp_path: Path) -> None: + """Test private struct field flagged.""" + code = "struct S {\nprivate:\n int count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + +class TestProtectedMembersFlagged: + """Protected data members without a leading underscore are flagged.""" + + def test_protected_field_flagged(self, tmp_path: Path) -> None: + """Test protected field flagged.""" + code = "class C {\nprotected:\n int count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "protected member variable 'count'" in diags[0] + + def test_protected_static_const_field_flagged(self, tmp_path: Path) -> None: + """Test protected static const field flagged.""" + code = "class C {\nprotected:\n static const int count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + +class TestMembersAllowed: + """Public members and already-prefixed members are never flagged.""" + + def test_public_field_allowed(self, tmp_path: Path) -> None: + """Test public field allowed.""" + code = "class C {\npublic:\n int count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_default_public_struct_field_allowed(self, tmp_path: Path) -> None: + """Test default public struct field allowed.""" + code = "struct S {\n int count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_private_field_with_underscore_allowed(self, tmp_path: Path) -> None: + """Test private field with underscore allowed.""" + code = "class C {\nprivate:\n int _count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_protected_field_with_underscore_allowed(self, tmp_path: Path) -> None: + """Test protected field with underscore allowed.""" + code = "class C {\nprotected:\n int _count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_local_and_global_variables_allowed(self, tmp_path: Path) -> None: + """Test local and global variables allowed.""" + code = "int global;\nvoid f() {\n int local;\n (void)local;\n}\n" + assert _diags(code, tmp_path) == [] + + def test_macro_synthesized_member_allowed(self, tmp_path: Path) -> None: + """A member declared entirely inside a macro body is not flagged. + + Mirrors gtest's ``TEST_F(...)`` expanding to a fixture class with its + own private ``test_info_`` member: the whole class is synthesized on + the macro-invocation line, so there is no user-typed name to rename. + """ + code = ( + "#define DECLARE_FIXTURE(name) \\\n" + " class name { \\\n" + " private: \\\n" + " static int test_info_; \\\n" + " };\n" + "DECLARE_FIXTURE(Foo)\n" + ) + assert _diags(code, tmp_path) == [] + + def test_field_sharing_a_macro_call_line_still_allowed( + self, tmp_path: Path + ) -> None: + """A macro-instantiation line's own range covers the whole invocation.""" + code = "#define NOOP(x)\nclass C {\nprivate:\n NOOP(1) int count;\n};\n" + # The macro invocation and the field share line 4, so this is the + # documented trade-off: skipped rather than flagged. + assert _diags(code, tmp_path) == [] + + def test_lambda_capture_allowed(self, tmp_path: Path) -> None: + """Test lambda capture allowed.""" + code = ( + "void f() {\n" + " int count = 1;\n" + " auto l = [count]() { return count; };\n" + " (void)l;\n" + "}\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestMixedAccessSpecifiers: + """Only members under a private/protected section are flagged.""" + + def test_only_restricted_members_flagged(self, tmp_path: Path) -> None: + """Test only restricted members flagged.""" + code = ( + "class C {\n" + "public:\n" + " int pub;\n" + "private:\n" + " int priv;\n" + "protected:\n" + " int prot;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 2 + assert any("priv" in d for d in diags) + assert any("prot" in d for d in diags) + + +class TestEnforceMemberLeadingUnderscoreCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert EnforceMemberLeadingUnderscore().id == "memberLeadingUnderscore" + + def test_clean_file_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = "class C {\nprivate:\n int _count;\nprotected:\n int _flag;\n};\n" + assert _diags(code, tmp_path) == []