diff --git a/CHANGELOG.md b/CHANGELOG.md index 46777a3..30571c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file. #### CPP Rules +- Add `noPublicLeadingUnderscore` AST check flagging public member variables and member functions (including static and template ones) whose name starts with `_`, e.g. `int _count;` or `void _compute();` under `public:` (or under a `struct`'s default access) should drop the leading underscore. Private/protected members are never checked here, since that's the concern of `memberLeadingUnderscore`/`memberFunctionLeadingUnderscore`. Constructors/destructors, operator overloads/conversions, and methods overriding a base-class virtual method are always exempt, and members synthesized entirely by a macro invoked on the same source line are excluded too. Takes no configuration - Add `classMemberOrder` AST check enforcing a fixed section order within each class/struct/union body: public, protected, then private member variables, followed by public, protected, then private member functions. Only in-class declarations count — out-of-line member-function definitions don't affect ordering — and declarations outside that list (nested types, `using` declarations, enums, friend declarations, ...) are ignored rather than resetting the sequence. Takes an optional `excluded_macros` list (e.g. `["Q_OBJECT"]`) so members/access-specifier changes synthesized by a named macro invocation are excluded from ordering entirely diff --git a/docs/source/sections/ast_checks/enforce_no_public_leading_underscore.rst b/docs/source/sections/ast_checks/enforce_no_public_leading_underscore.rst new file mode 100644 index 0000000..00bd05f --- /dev/null +++ b/docs/source/sections/ast_checks/enforce_no_public_leading_underscore.rst @@ -0,0 +1,43 @@ +noPublicLeadingUnderscore +=========================== + +Flags any data member or member function declared under a ``public:`` +access specifier (or under no access specifier at all in a +``struct``/``union``) whose name starts with ``_``. Private/protected +members are never flagged here — that's the concern of +:doc:`memberLeadingUnderscore ` and +:doc:`memberFunctionLeadingUnderscore +`. + +.. code-block:: cpp + + class Widget { + public: + int size; // Good. + int _size; // Bad — flagged, leading underscore on a public member. + + void compute(); // Good. + void _compute(); // Bad — flagged, leading underscore on a public method. + + private: + int _count; // Good — not checked here. + }; + +Constructors/destructors, operator overloads/conversions, and methods +overriding a base-class virtual method are always exempt, since their names +aren't the author's to change. A member synthesized entirely by a macro +invoked on that same source line is not flagged either — there is no +user-typed name to rename. + +Configuration +-------------- + +This check takes no configuration. + +Disabling +---------- + +.. code-block:: toml + + [cpp] + ast_check_disabled_ids = ["noPublicLeadingUnderscore"] diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index 43792a2..6805936 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -70,6 +70,9 @@ Available checks * - :doc:`memberFunctionLeadingUnderscore ` - A private or protected member function not starting with ``_``. - None + * - :doc:`noPublicLeadingUnderscore ` + - A public member variable/function starting with ``_``. + - None * - :doc:`classMemberOrder ` - A member variable/function declared before a section that must precede it (public/protected/private members, then @@ -97,6 +100,7 @@ Available checks enforce_member_leading_underscore enforce_member_function_leading_underscore + enforce_no_public_leading_underscore enforce_class_member_order param_name_for_type macro_replacement diff --git a/src/devops/cpp/ast/checks/enforce_no_public_leading_underscore.py b/src/devops/cpp/ast/checks/enforce_no_public_leading_underscore.py new file mode 100644 index 0000000..abe2dee --- /dev/null +++ b/src/devops/cpp/ast/checks/enforce_no_public_leading_underscore.py @@ -0,0 +1,275 @@ +"""Forbid a leading underscore on public member variables and functions. + +Any data member or member function (including static and template ones) +declared under a ``public:`` access specifier — or under no access +specifier at all in a ``struct``/``union`` — must have a name that does +*not* start with ``_`` (e.g. ``count``, ``compute()``, not ``_count`` / +``_compute()``). A leading underscore is reserved for private/protected +members by :doc:`memberLeadingUnderscore +` and +:doc:`memberFunctionLeadingUnderscore +`, so on a +public member it signals the wrong access level or a stray rename rather +than being the author's real intent. Private/protected members are never +flagged here — that's the concern of the two checks above. + +A handful of member-function kinds are never flagged, because their name is +fixed by the language or by a base class and renaming them isn't something +the author can freely do: + +- Constructors and destructors — their name is always the class's own name + (or ``~ClassName``). +- Operator overloads and conversion functions (``operator==``, + ``operator[]``, ``operator int() const``, ...) — the operator's spelling + is fixed by the language grammar. +- Methods that override a base class's virtual method — the override must + keep the base method's exact name to bind at all. + +A member declared entirely inside a macro invoked on the same source line +(e.g. gtest's ``TEST_F(...)`` expanding to a fixture class with its own +public members) is not flagged either: 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 = ["noPublicLeadingUnderscore"] +""" + +from __future__ import annotations + +import ctypes +import typing + +import clang.cindex as clang +from clang.cindex import conf + +from devops.cpp.ast.base import Check, Diagnostic + +# Cursor kinds of a record whose direct members/methods 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)) + +# CXX_METHOD covers ordinary (including static and operator) methods, +# CONVERSION_FUNCTION covers `operator T() const`-style conversions, and +# FUNCTION_TEMPLATE covers member function templates. Constructors and +# destructors are deliberately excluded — see the module docstring. +_MEMBER_FUNCTION_KINDS = frozenset( + ( + clang.CursorKind.CXX_METHOD, + clang.CursorKind.CONVERSION_FUNCTION, + clang.CursorKind.FUNCTION_TEMPLATE, + ) +) + + +def _bind_overridden_cursors_ctypes() -> None: + """Bind the ctypes signatures for the two libclang functions used below. + + Deferred to first use — rather than run at import time — so that + importing this module (e.g. when Sphinx documents it with ``clang`` + mocked out because libclang isn't installed) doesn't require a real + ``clang.Cursor`` ctypes type. Idempotent: checks whether the signature + is already bound instead of relying on a mutable module-level flag. + """ + if conf.lib.clang_getOverriddenCursors.argtypes is not None: + return + conf.lib.clang_getOverriddenCursors.restype = None + conf.lib.clang_getOverriddenCursors.argtypes = [ + clang.Cursor, + ctypes.POINTER(ctypes.POINTER(clang.Cursor)), + ctypes.POINTER(ctypes.c_uint), + ] + conf.lib.clang_disposeOverriddenCursors.restype = None + conf.lib.clang_disposeOverriddenCursors.argtypes = [ctypes.POINTER(clang.Cursor)] + + +def _overrides_base_method(cursor: clang.Cursor) -> bool: + """Return whether `cursor` overrides at least one base-class method. + + Parameters + ---------- + cursor: clang.Cursor + A ``CXX_METHOD`` (or similar) cursor to check. + + Returns + ------- + bool + True if `cursor` overrides one or more base-class virtual methods. + + """ + _bind_overridden_cursors_ctypes() + overridden = ctypes.POINTER(clang.Cursor)() + count = ctypes.c_uint() + conf.lib.clang_getOverriddenCursors( + cursor, ctypes.byref(overridden), ctypes.byref(count) + ) + has_override = count.value > 0 + if has_override: + conf.lib.clang_disposeOverriddenCursors(overridden) + return has_override + + +def _is_public_candidate_member(cursor: clang.Cursor) -> bool: + """Return whether `cursor` is a public data-member candidate. + + Parameters + ---------- + cursor: clang.Cursor + The AST node to check. + + Returns + ------- + bool + True if `cursor` is a public member variable this check should + inspect for a leading underscore. + + """ + if cursor.kind not in _MEMBER_KINDS: + return False + if cursor.access_specifier != clang.AccessSpecifier.PUBLIC: + return False + parent = cursor.semantic_parent + if parent is None or parent.kind not in _RECORD_KINDS: + return False + return bool(cursor.spelling) + + +def _is_public_candidate_function(cursor: clang.Cursor) -> bool: + """Return whether `cursor` is a public member-function candidate. + + Parameters + ---------- + cursor: clang.Cursor + The AST node to check. + + Returns + ------- + bool + True if `cursor` is a public member function this check should + inspect for a leading underscore. + + """ + if cursor.kind not in _MEMBER_FUNCTION_KINDS: + return False + if cursor.access_specifier != clang.AccessSpecifier.PUBLIC: + return False + parent = cursor.semantic_parent + if parent is None or parent.kind not in _RECORD_KINDS: + return False + name = cursor.spelling + if not name or name.startswith("operator"): + return False + return not _overrides_base_method(cursor) + + +class _PendingMember(typing.NamedTuple): + """A candidate diagnostic buffered until end-of-file macro info is known.""" + + name: str + kind: str + line: int + column: int + + +class EnforceNoPublicLeadingUnderscore(Check): + """Flag public member variables/functions whose name starts with `_`. + + 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 = "noPublicLeadingUnderscore" + + 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 _is_public_candidate_member(cursor): + kind = "member variable" + elif _is_public_candidate_function(cursor): + kind = "member function" + else: + return [] + + name = cursor.spelling + if not name.startswith("_"): + return [] + + loc = cursor.location + self._pending.setdefault(filename, []).append( + _PendingMember(name, kind, 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 public member 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"public {member.kind} '{member.name}' should not start " + f"with a leading underscore ('{member.name.lstrip('_')}')" + ), + 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 e415dce..2b0cfe4 100644 --- a/src/devops/cpp/ast/registry.py +++ b/src/devops/cpp/ast/registry.py @@ -16,6 +16,9 @@ from devops.cpp.ast.checks.enforce_member_leading_underscore import ( EnforceMemberLeadingUnderscore, ) +from devops.cpp.ast.checks.enforce_no_public_leading_underscore import ( + EnforceNoPublicLeadingUnderscore, +) 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 @@ -34,6 +37,7 @@ EnforceClassMemberOrder(), EnforceMemberFunctionLeadingUnderscore(), EnforceMemberLeadingUnderscore(), + EnforceNoPublicLeadingUnderscore(), EnforceParamNameForType(), MacroReplacement(), NoGlobalUsing(), diff --git a/tests/cpp/ast/test_enforce_no_public_leading_underscore.py b/tests/cpp/ast/test_enforce_no_public_leading_underscore.py new file mode 100644 index 0000000..4b6974f --- /dev/null +++ b/tests/cpp/ast/test_enforce_no_public_leading_underscore.py @@ -0,0 +1,241 @@ +"""Tests for the EnforceNoPublicLeadingUnderscore AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.enforce_no_public_leading_underscore import ( + EnforceNoPublicLeadingUnderscore, +) +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [EnforceNoPublicLeadingUnderscore()] +_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 TestPublicMembersFlagged: + """Public data members starting with an underscore are flagged.""" + + def test_public_field_flagged(self, tmp_path: Path) -> None: + """Test public field flagged.""" + code = "class C {\npublic:\n int _count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable '_count'" in diags[0] + assert "'count'" in diags[0] + + def test_public_static_field_flagged(self, tmp_path: Path) -> None: + """Test public static field flagged.""" + code = "class C {\npublic:\n static int _count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + def test_default_public_struct_field_flagged(self, tmp_path: Path) -> None: + """Test default public struct field flagged.""" + code = "struct S {\n int _count;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + +class TestPublicMethodsFlagged: + """Public member functions starting with an underscore are flagged.""" + + def test_public_method_flagged(self, tmp_path: Path) -> None: + """Test public method flagged.""" + code = "class C {\npublic:\n void _compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member function '_compute'" in diags[0] + assert "'compute'" in diags[0] + + def test_public_static_method_flagged(self, tmp_path: Path) -> None: + """Test public static method flagged.""" + code = "class C {\npublic:\n static void _compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + def test_public_template_method_flagged(self, tmp_path: Path) -> None: + """Test public template method flagged.""" + code = "class C {\npublic:\n template void _compute(T);\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + def test_default_public_struct_method_flagged(self, tmp_path: Path) -> None: + """Test default public struct method flagged.""" + code = "struct S {\n void _compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + +class TestMembersAllowed: + """Private/protected members and already-clean names are never flagged.""" + + def test_private_field_allowed(self, tmp_path: Path) -> None: + """Test private field allowed.""" + code = "class C {\nprivate:\n int _count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_protected_field_allowed(self, tmp_path: Path) -> None: + """Test protected field allowed.""" + code = "class C {\nprotected:\n int _count;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_public_field_without_underscore_allowed(self, tmp_path: Path) -> None: + """Test public field without underscore allowed.""" + code = "class C {\npublic:\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.""" + code = ( + "#define DECLARE_FIXTURE(name) \\\n" + " class name { \\\n" + " public: \\\n" + " static int _test_info; \\\n" + " };\n" + "DECLARE_FIXTURE(Foo)\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestMethodsAllowed: + """Private/protected methods and language-fixed names are never flagged.""" + + def test_private_method_allowed(self, tmp_path: Path) -> None: + """Test private method allowed.""" + code = "class C {\nprivate:\n void _compute();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_protected_method_allowed(self, tmp_path: Path) -> None: + """Test protected method allowed.""" + code = "class C {\nprotected:\n void _compute();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_public_method_without_underscore_allowed(self, tmp_path: Path) -> None: + """Test public method without underscore allowed.""" + code = "class C {\npublic:\n void compute();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_constructor_allowed(self, tmp_path: Path) -> None: + """Test constructor allowed.""" + code = "class C {\npublic:\n C();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_destructor_allowed(self, tmp_path: Path) -> None: + """Test destructor allowed.""" + code = "class C {\npublic:\n ~C();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_operator_overload_allowed(self, tmp_path: Path) -> None: + """Test operator overload allowed.""" + code = ( + "class C {\npublic:\n" + " void operator()();\n" + " bool operator==(const C&) const;\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + def test_conversion_operator_allowed(self, tmp_path: Path) -> None: + """Test conversion operator allowed.""" + code = "class C {\npublic:\n operator int() const;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_overriding_method_allowed(self, tmp_path: Path) -> None: + """Only the base declaration is flagged; the override itself is not.""" + code = ( + "class Base {\npublic:\n" + " virtual void _doThing();\n" + " virtual ~Base() = default;\n" + "};\n" + "class C : public Base {\npublic:\n" + " void _doThing() override;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "_doThing" in diags[0] + + def test_free_function_allowed(self, tmp_path: Path) -> None: + """Test free function allowed.""" + code = "void _compute();\n" + assert _diags(code, tmp_path) == [] + + def test_macro_synthesized_method_allowed(self, tmp_path: Path) -> None: + """A method declared entirely inside a macro body is not flagged.""" + code = ( + "#define DECLARE_FIXTURE(name) \\\n" + " class name { \\\n" + " public: \\\n" + " void _testBody(); \\\n" + " };\n" + "DECLARE_FIXTURE(Foo)\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestMixedAccessSpecifiers: + """Only members/methods under a public section are flagged.""" + + def test_only_public_members_flagged(self, tmp_path: Path) -> None: + """Test only public 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) == 1 + assert "_pub" in diags[0] + + def test_only_public_methods_flagged(self, tmp_path: Path) -> None: + """Test only public methods flagged.""" + code = ( + "class C {\n" + "public:\n" + " void _pub();\n" + "private:\n" + " void _priv();\n" + "protected:\n" + " void _prot();\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "_pub" in diags[0] + + +class TestEnforceNoPublicLeadingUnderscoreCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert EnforceNoPublicLeadingUnderscore().id == "noPublicLeadingUnderscore" + + def test_clean_file_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = "class C {\npublic:\n int count;\n void compute();\n};\n" + assert _diags(code, tmp_path) == []