From 6920a108788ab4835cd1679f497870e01ec2ebba Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Mon, 28 Sep 2026 22:45:37 +0200 Subject: [PATCH 1/5] Create ast cpp check for correct ordering of class members and functions Fixes #139 --- CHANGELOG.md | 7 + .../ast_checks/enforce_class_member_order.rst | 77 ++++++ docs/source/sections/ast_checks/index.rst | 6 + .../ast/checks/enforce_class_member_order.py | 172 +++++++++++++ src/devops/cpp/ast/registry.py | 2 + .../ast/test_enforce_class_member_order.py | 230 ++++++++++++++++++ 6 files changed, 494 insertions(+) create mode 100644 docs/source/sections/ast_checks/enforce_class_member_order.rst create mode 100644 src/devops/cpp/ast/checks/enforce_class_member_order.py create mode 100644 tests/cpp/ast/test_enforce_class_member_order.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 70ad1cf..6b0df49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file. ## Next Release + +### Features + +#### CPP Rules + +- 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 no configuration + ## [0.7.0](https://github.com/repo/owner/releases/tag/0.7.0) - 2026-09-28 ### Python Requirement diff --git a/docs/source/sections/ast_checks/enforce_class_member_order.rst b/docs/source/sections/ast_checks/enforce_class_member_order.rst new file mode 100644 index 0000000..af00bd0 --- /dev/null +++ b/docs/source/sections/ast_checks/enforce_class_member_order.rst @@ -0,0 +1,77 @@ +classMemberOrder +================= + +Flags a member variable or member function declared before a section that +must precede it. Within a single class/struct/union body (including +templates), the direct members must appear grouped, in exactly this order: + +1. public member variables +2. protected member variables +3. private member variables +4. public member functions +5. protected member functions +6. private member functions + +.. code-block:: cpp + + class Widget { + public: + int size; + + protected: + int flag; + + private: + int count; + + public: + void resize(); + + protected: + void hook(); + + private: + void compute(); + }; + + class Bad { + private: + int count; // Bad — flagged, a private member variable + + public: + int size; // before a public one. + + void resize(); + + private: + void compute(); + + public: + void hook(); // Bad — flagged, a public member function after + // a private one. + }; + +A section may be skipped entirely (e.g. a class with no protected members +at all), but once a later section has started, an earlier one may not +reappear. + +Only members declared lexically inside the class body count — an +out-of-line member-function definition (``void C::f() { ... }``) never +affects its class's ordering, only the in-class declaration does. Nested +types, ``using`` declarations, enums, friend declarations, and other +declarations not covered by the list above are ignored for ordering +purposes: they neither need to fit anywhere in particular nor reset the +sequence. + +Configuration +-------------- + +This check takes no configuration. + +Disabling +---------- + +.. code-block:: toml + + [cpp] + ast_check_disabled_ids = ["classMemberOrder"] diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index 3a6c334..e5ede54 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -70,6 +70,11 @@ Available checks * - :doc:`memberFunctionLeadingUnderscore ` - A private or protected member function not starting with ``_``. - None + * - :doc:`classMemberOrder ` + - A member variable/function declared before a section that must + precede it (public/protected/private members, then + public/protected/private member functions). + - None * - :doc:`paramNameForType ` - A parameter of a configured type not using its canonical name. - Required (``type_to_name``) @@ -92,6 +97,7 @@ Available checks enforce_member_leading_underscore enforce_member_function_leading_underscore + enforce_class_member_order param_name_for_type macro_replacement no_global_using diff --git a/src/devops/cpp/ast/checks/enforce_class_member_order.py b/src/devops/cpp/ast/checks/enforce_class_member_order.py new file mode 100644 index 0000000..8fe9a5c --- /dev/null +++ b/src/devops/cpp/ast/checks/enforce_class_member_order.py @@ -0,0 +1,172 @@ +"""Enforce a fixed section order for member variables and member functions. + +Within a single class/struct/union body (including templates), the direct +members must appear grouped, in exactly this order: + +1. public member variables +2. protected member variables +3. private member variables +4. public member functions +5. protected member functions +6. private member functions + +Anything not covered by that list — nested types, ``using`` declarations, +enums, friend declarations, ``static_assert``, the access specifiers +themselves — is ignored for ordering purposes: it neither has to fit +anywhere in particular nor resets the sequence. + +Only declarations that lexically appear inside the class body are +considered, so an out-of-line member-function definition +(``void C::f() { ... }``) never affects its class's ordering — only the +in-class declaration does. + +To disable this check for a project set:: + + [cpp] + ast_check_disabled_ids = ["classMemberOrder"] +""" + +from __future__ import annotations + +import clang.cindex as clang + +from devops.cpp.ast.base import Check, Diagnostic + +# Record kinds whose direct children 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 direct VAR_DECL child of a +# record is a static data member (ordinary VAR_DECLs elsewhere aren't +# children of a record cursor at all, so no extra filtering is needed here). +_MEMBER_VARIABLE_KINDS = frozenset( + (clang.CursorKind.FIELD_DECL, clang.CursorKind.VAR_DECL) +) + +# CXX_METHOD covers ordinary (including static and operator) methods, +# CONSTRUCTOR/DESTRUCTOR cover special member functions, CONVERSION_FUNCTION +# covers `operator T() const`-style conversions, and FUNCTION_TEMPLATE covers +# member function templates. +_MEMBER_FUNCTION_KINDS = frozenset( + ( + clang.CursorKind.CXX_METHOD, + clang.CursorKind.CONSTRUCTOR, + clang.CursorKind.DESTRUCTOR, + clang.CursorKind.CONVERSION_FUNCTION, + clang.CursorKind.FUNCTION_TEMPLATE, + ) +) + +_ACCESS_RANK = { + clang.AccessSpecifier.PUBLIC: 0, + clang.AccessSpecifier.PROTECTED: 1, + clang.AccessSpecifier.PRIVATE: 2, +} + +# Indexed by phase (category-rank * 3 + access-rank): the required order is +# public/protected/private member variables, then public/protected/private +# member functions. +_PHASE_LABELS = ( + "a public member variable", + "a protected member variable", + "a private member variable", + "a public member function", + "a protected member function", + "a private member function", +) + + +def _category_rank(kind: clang.CursorKind) -> int | None: + """Classify a direct record-member cursor kind for ordering purposes. + + Parameters + ---------- + kind: clang.CursorKind + The cursor kind of a direct child of a class/struct/union. + + Returns + ------- + int | None + 0 for a member variable, 1 for a member function, or None if `kind` + isn't relevant to this check's ordering (nested types, using + declarations, access specifiers, ...). + + """ + if kind in _MEMBER_VARIABLE_KINDS: + return 0 + if kind in _MEMBER_FUNCTION_KINDS: + return 1 + return None + + +class EnforceClassMemberOrder(Check): + """Flag member variables/functions declared out of the required section order.""" + + id = "classMemberOrder" + + def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: + """Check one record's direct children for out-of-order sections. + + Parameters + ---------- + cursor: clang.Cursor + The AST node currently being visited. + filename: str + Path of the file being checked. + + Returns + ------- + list[Diagnostic] + One diagnostic per member declared before a section that must + precede it (e.g. a member function found before a still-pending + private member variable section). + + """ + if cursor.kind not in _RECORD_KINDS: + return [] + + diagnostics: list[Diagnostic] = [] + highest_phase_seen = -1 + highest_label = "" + + for child in cursor.get_children(): + category_rank = _category_rank(child.kind) + if category_rank is None: + continue + access_rank = _ACCESS_RANK.get(child.access_specifier) + if access_rank is None: + continue + + phase = category_rank * 3 + access_rank + if phase < highest_phase_seen: + name = child.spelling or "" + loc = child.location + diagnostics.append( + Diagnostic( + file=filename, + line=loc.line, + column=loc.column, + message=( + f"{_PHASE_LABELS[phase]} '{name}' is declared " + f"after {highest_label} — a class must declare " + "public, protected, then private member " + "variables, followed by public, protected, then " + "private member functions, in that order" + ), + check_id=self.id, + severity="style", + ) + ) + continue + + highest_phase_seen = phase + highest_label = _PHASE_LABELS[phase] + + return diagnostics diff --git a/src/devops/cpp/ast/registry.py b/src/devops/cpp/ast/registry.py index b77dc99..e415dce 100644 --- a/src/devops/cpp/ast/registry.py +++ b/src/devops/cpp/ast/registry.py @@ -9,6 +9,7 @@ import copy import typing +from devops.cpp.ast.checks.enforce_class_member_order import EnforceClassMemberOrder from devops.cpp.ast.checks.enforce_member_function_leading_underscore import ( EnforceMemberFunctionLeadingUnderscore, ) @@ -30,6 +31,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] = [ + EnforceClassMemberOrder(), EnforceMemberFunctionLeadingUnderscore(), EnforceMemberLeadingUnderscore(), EnforceParamNameForType(), diff --git a/tests/cpp/ast/test_enforce_class_member_order.py b/tests/cpp/ast/test_enforce_class_member_order.py new file mode 100644 index 0000000..3045104 --- /dev/null +++ b/tests/cpp/ast/test_enforce_class_member_order.py @@ -0,0 +1,230 @@ +"""Tests for the EnforceClassMemberOrder AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.enforce_class_member_order import EnforceClassMemberOrder +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [EnforceClassMemberOrder()] +_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 TestCorrectOrderAllowed: + """A class following the required section order is never flagged.""" + + def test_full_correct_order_allowed(self, tmp_path: Path) -> None: + """Test full correct order allowed.""" + code = ( + "class C {\n" + "public:\n" + " int a;\n" + "protected:\n" + " int b;\n" + "private:\n" + " int c;\n" + "public:\n" + " void f();\n" + "protected:\n" + " void g();\n" + "private:\n" + " void h();\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + def test_sections_may_be_skipped(self, tmp_path: Path) -> None: + """Test sections may be skipped.""" + code = ( + "class C {\n" + "public:\n" + " int a;\n" + "private:\n" + " int c;\n" + "public:\n" + " void f();\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + def test_default_public_struct_order_allowed(self, tmp_path: Path) -> None: + """Test default public struct order allowed.""" + code = "struct S {\n int a;\n void f();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_only_variables_allowed(self, tmp_path: Path) -> None: + """Test only variables allowed.""" + code = "class C {\npublic:\n int a;\nprivate:\n int b;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_only_functions_allowed(self, tmp_path: Path) -> None: + """Test only functions allowed.""" + code = "class C {\npublic:\n void f();\nprivate:\n void g();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_ignored_declarations_do_not_break_order(self, tmp_path: Path) -> None: + """Nested types, usings, and friend decls don't affect ordering.""" + code = ( + "class C {\n" + "public:\n" + " using Alias = int;\n" + " enum class E { A, B };\n" + " struct Nested { int x; };\n" + " int a;\n" + "private:\n" + " int b;\n" + "public:\n" + " void f();\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestOutOfOrderFlagged: + """Sections declared before a section that must precede them are flagged.""" + + def test_private_before_public_member_flagged(self, tmp_path: Path) -> None: + """Test private before public member flagged.""" + code = ( + "class C {\n" + "private:\n" + " int b;\n" + "public:\n" + " int a;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable 'a'" in diags[0] + assert "private member variable" in diags[0] + + def test_protected_before_public_member_flagged(self, tmp_path: Path) -> None: + """Test protected before public member flagged.""" + code = ( + "class C {\n" + "protected:\n" + " int b;\n" + "public:\n" + " int a;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable 'a'" in diags[0] + + def test_function_before_member_variable_flagged(self, tmp_path: Path) -> None: + """Member functions declared before member variables are flagged.""" + code = "class C {\npublic:\n void f();\n int a;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable 'a'" in diags[0] + assert "public member function" in diags[0] + + def test_public_function_before_private_member_flagged( + self, tmp_path: Path + ) -> None: + """Test public function before private member flagged.""" + code = ( + "class C {\n" + "public:\n" + " void f();\n" + "private:\n" + " int a;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "private member variable 'a'" in diags[0] + + def test_private_function_before_public_function_flagged( + self, tmp_path: Path + ) -> None: + """Test private function before public function flagged.""" + code = ( + "class C {\n" + "private:\n" + " void g();\n" + "public:\n" + " void f();\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member function 'f'" in diags[0] + + def test_multiple_violations_all_flagged(self, tmp_path: Path) -> None: + """Test multiple violations all flagged.""" + code = ( + "class C {\n" + "private:\n" + " int b;\n" + "public:\n" + " int a;\n" + " void f();\n" + "private:\n" + " void g();\n" + "protected:\n" + " void h();\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 2 + assert any("'a'" in d for d in diags) + assert any("'h'" in d for d in diags) + + +class TestStaticAndSpecialMembers: + """Static data members and constructors/destructors participate in ordering.""" + + def test_static_member_treated_as_variable(self, tmp_path: Path) -> None: + """Test static member treated as variable.""" + code = ( + "class C {\n" + "public:\n" + " void f();\n" + " static int count;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable 'count'" in diags[0] + + def test_constructor_treated_as_function(self, tmp_path: Path) -> None: + """Test constructor treated as function.""" + code = "class C {\npublic:\n C();\n int a;\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "public member variable 'a'" in diags[0] + + +class TestEnforceClassMemberOrderCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert EnforceClassMemberOrder().id == "classMemberOrder" + + def test_clean_file_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = ( + "class C {\n" + "public:\n" + " int a;\n" + " void f();\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] From b7ff07819de0af3d7913ec72899d60069d5aa96d Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Mon, 28 Sep 2026 23:40:04 +0200 Subject: [PATCH 2/5] Create ast cpp check for correct ordering of class members and functions Fixes #139 --- CHANGELOG.md | 2 +- .../ast_checks/enforce_class_member_order.rst | 13 +- docs/source/sections/ast_checks/index.rst | 2 +- .../ast/checks/enforce_class_member_order.py | 167 ++++++++++++++---- .../ast/test_enforce_class_member_order.py | 73 +++++++- 5 files changed, 220 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b0df49..c08f7ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to this project will be documented in this file. #### CPP Rules -- 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 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 ## [0.7.0](https://github.com/repo/owner/releases/tag/0.7.0) - 2026-09-28 diff --git a/docs/source/sections/ast_checks/enforce_class_member_order.rst b/docs/source/sections/ast_checks/enforce_class_member_order.rst index af00bd0..39fa17e 100644 --- a/docs/source/sections/ast_checks/enforce_class_member_order.rst +++ b/docs/source/sections/ast_checks/enforce_class_member_order.rst @@ -66,7 +66,18 @@ sequence. Configuration -------------- -This check takes no configuration. +Optional — exclude specific macros invoked inside a class body from +ordering entirely. Any member (or access-specifier change) synthesized by +a listed macro, matched by the macro's own name at its invocation line, is +skipped: it's neither flagged itself nor counted when checking what came +before or after it. Useful for macros such as Qt's ``Q_OBJECT`` that expand +to boilerplate members and their own access-specifier bookkeeping, whose +position isn't the author's choice. + +.. code-block:: toml + + [cpp.ast_check_config.classMemberOrder] + excluded_macros = ["Q_OBJECT", "MY_DECLARE_PROPERTY"] Disabling ---------- diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index e5ede54..43792a2 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -74,7 +74,7 @@ Available checks - A member variable/function declared before a section that must precede it (public/protected/private members, then public/protected/private member functions). - - None + - Optional (``excluded_macros``) * - :doc:`paramNameForType ` - A parameter of a configured type not using its canonical name. - Required (``type_to_name``) diff --git a/src/devops/cpp/ast/checks/enforce_class_member_order.py b/src/devops/cpp/ast/checks/enforce_class_member_order.py index 8fe9a5c..4951f34 100644 --- a/src/devops/cpp/ast/checks/enforce_class_member_order.py +++ b/src/devops/cpp/ast/checks/enforce_class_member_order.py @@ -20,7 +20,18 @@ (``void C::f() { ... }``) never affects its class's ordering — only the in-class declaration does. -To disable this check for a project set:: +Declarations synthesized by a configured macro (matched by the macro's own +name, at its invocation line) are excluded entirely from ordering: they are +neither flagged themselves nor counted when checking what came before or +after them — exactly like the nested-type/using-declaration exclusions +above. This is for macros such as Qt's ``Q_OBJECT`` that expand to +boilerplate members (and possibly their own access-specifier changes) whose +position isn't the author's choice:: + + [cpp.ast_check_config.classMemberOrder] + excluded_macros = ["Q_OBJECT", "MY_DECLARE_PROPERTY"] + +To disable this check entirely for a project set:: [cpp] ast_check_disabled_ids = ["classMemberOrder"] @@ -28,8 +39,11 @@ from __future__ import annotations +import typing + import clang.cindex as clang +from devops.config.base import ConfigError from devops.cpp.ast.base import Check, Diagnostic # Record kinds whose direct children this check inspects. @@ -106,13 +120,60 @@ def _category_rank(kind: clang.CursorKind) -> int | None: return None +class _PendingMember(typing.NamedTuple): + """A classified record member, buffered until end-of-file macro info is known.""" + + phase: int + name: str + line: int + column: int + + class EnforceClassMemberOrder(Check): - """Flag member variables/functions declared out of the required section order.""" + """Flag member variables/functions declared out of the required section order. + + Reporting is deferred to `finalize()`: a macro invoked inside a class + body (e.g. ``Q_OBJECT``) shows up in libclang's preprocessing record as + a cursor local to the translation unit rather than as a lexical child of + the class, and may be visited before or after the class itself in a + single preorder walk. Whether a given member's line is covered by an + *excluded* macro invocation can therefore only be known once the whole + file has been walked. + """ id = "classMemberOrder" + def __init__(self) -> None: + """Initialise with no macros excluded and empty per-file buffers.""" + self._excluded_macros: frozenset[str] = frozenset() + self._macro_lines: dict[str, set[int]] = {} + self._pending_records: dict[str, list[list[_PendingMember]]] = {} + + def configure(self, config: dict) -> None: + """Load the ``excluded_macros`` list from the check's TOML config block. + + Parameters + ---------- + config: dict + Expected shape: ``{"excluded_macros": ["Q_OBJECT"]}``. + + Raises + ------ + ConfigError + If ``excluded_macros`` is present but is not a list of strings. + + """ + raw = config.get("excluded_macros", []) + if not isinstance(raw, list) or not all(isinstance(n, str) for n in raw): + msg = ( + f"{self.id}: 'excluded_macros' in " + f"[cpp.ast_check_config.{self.id}] must be a list of strings" + ) + raise ConfigError(msg) + self._excluded_macros = frozenset(raw) + def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: - """Check one record's direct children for out-of-order sections. + """Record excluded-macro lines and classify one record's direct children. Parameters ---------- @@ -124,18 +185,22 @@ def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: Returns ------- list[Diagnostic] - One diagnostic per member declared before a section that must - precede it (e.g. a member function found before a still-pending - private member variable section). + Always empty — diagnostics are emitted from `finalize()` once + the whole file (including its macro instantiations) is known. """ - if cursor.kind not in _RECORD_KINDS: + if cursor.kind == clang.CursorKind.MACRO_INSTANTIATION: + if cursor.spelling in self._excluded_macros: + lines = self._macro_lines.setdefault(filename, set()) + lines.update( + range(cursor.extent.start.line, cursor.extent.end.line + 1) + ) return [] - diagnostics: list[Diagnostic] = [] - highest_phase_seen = -1 - highest_label = "" + if cursor.kind not in _RECORD_KINDS: + return [] + members: list[_PendingMember] = [] for child in cursor.get_children(): category_rank = _category_rank(child.kind) if category_rank is None: @@ -144,29 +209,67 @@ def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: if access_rank is None: continue - phase = category_rank * 3 + access_rank - if phase < highest_phase_seen: - name = child.spelling or "" - loc = child.location - diagnostics.append( - Diagnostic( - file=filename, - line=loc.line, - column=loc.column, - message=( - f"{_PHASE_LABELS[phase]} '{name}' is declared " - f"after {highest_label} — a class must declare " - "public, protected, then private member " - "variables, followed by public, protected, then " - "private member functions, in that order" - ), - check_id=self.id, - severity="style", - ) + loc = child.location + members.append( + _PendingMember( + phase=category_rank * 3 + access_rank, + name=child.spelling or "", + line=loc.line, + column=loc.column, ) - continue + ) + + self._pending_records.setdefault(filename, []).append(members) + return [] + + def finalize(self, filename: str) -> list[Diagnostic]: + """Evaluate ordering for every record buffered for `filename`. + + Parameters + ---------- + filename: str + Path of the file being checked. + + Returns + ------- + list[Diagnostic] + One diagnostic per member declared before a section that must + precede it, skipping members whose line is covered by an + excluded macro invocation. + + """ + macro_lines = self._macro_lines.pop(filename, set()) + records = self._pending_records.pop(filename, []) + + diagnostics: list[Diagnostic] = [] + for members in records: + highest_phase_seen = -1 + highest_label = "" + for member in members: + if member.line in macro_lines: + continue + + if member.phase < highest_phase_seen: + diagnostics.append( + Diagnostic( + file=filename, + line=member.line, + column=member.column, + message=( + f"{_PHASE_LABELS[member.phase]} '{member.name}' " + f"is declared after {highest_label} — a class " + "must declare public, protected, then private " + "member variables, followed by public, " + "protected, then private member functions, in " + "that order" + ), + check_id=self.id, + severity="style", + ) + ) + continue - highest_phase_seen = phase - highest_label = _PHASE_LABELS[phase] + highest_phase_seen = member.phase + highest_label = _PHASE_LABELS[member.phase] return diagnostics diff --git a/tests/cpp/ast/test_enforce_class_member_order.py b/tests/cpp/ast/test_enforce_class_member_order.py index 3045104..806f5e8 100644 --- a/tests/cpp/ast/test_enforce_class_member_order.py +++ b/tests/cpp/ast/test_enforce_class_member_order.py @@ -6,6 +6,7 @@ import pytest +from devops.config.base import ConfigError from devops.cpp.ast.checks.enforce_class_member_order import EnforceClassMemberOrder from devops.cpp.ast.engine import run_ast_checks @@ -18,10 +19,12 @@ _ARGS = ["-std=c++17"] -def _diags(code: str, tmp_path: Path) -> list[str]: +def _diags( + code: str, tmp_path: Path, checks: list[EnforceClassMemberOrder] | None = None +) -> 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)] + return [d.message for d in run_ast_checks(p, code, _ARGS, checks=checks or _CHECK)] class TestCorrectOrderAllowed: @@ -211,6 +214,72 @@ def test_constructor_treated_as_function(self, tmp_path: Path) -> None: assert "public member variable 'a'" in diags[0] +class TestExcludedMacros: + """Declarations synthesized by a configured macro are ignored for ordering.""" + + def _check(self, excluded_macros: list[str]) -> list[EnforceClassMemberOrder]: + check = EnforceClassMemberOrder() + check.configure({"excluded_macros": excluded_macros}) + return [check] + + def test_excluded_macro_member_ignored(self, tmp_path: Path) -> None: + """A member synthesized by an excluded macro doesn't break ordering. + + The macro expands to a private field followed by a reset back to + ``public:`` (mirroring how ``Q_OBJECT``-style macros bring their own + access-specifier bookkeeping), which would otherwise make the + public ``a`` that follows look like a public-after-private + violation. + """ + code = ( + "#define Q_OBJECT private: int _qObjectData; public:\n" + "class C {\n" + "public:\n" + " Q_OBJECT\n" + " int a;\n" + "};\n" + ) + assert _diags(code, tmp_path, self._check(["Q_OBJECT"])) == [] + + def test_excluded_macro_member_still_flagged_when_not_configured( + self, tmp_path: Path + ) -> None: + """The same file is flagged when the macro isn't excluded.""" + code = ( + "#define Q_OBJECT private: int _qObjectData; public:\n" + "class C {\n" + "public:\n" + " Q_OBJECT\n" + " int a;\n" + "};\n" + ) + assert _diags(code, tmp_path) != [] + + def test_unrelated_macro_not_excluded(self, tmp_path: Path) -> None: + """Only the configured macro name is excluded, not every macro.""" + code = ( + "#define OTHER_MACRO private: int _otherData; public:\n" + "class C {\n" + "public:\n" + " OTHER_MACRO\n" + " int a;\n" + "};\n" + ) + assert _diags(code, tmp_path, self._check(["Q_OBJECT"])) != [] + + def test_invalid_excluded_macros_type_raises(self) -> None: + """A non-list-of-strings ``excluded_macros`` raises ConfigError.""" + check = EnforceClassMemberOrder() + with pytest.raises(ConfigError): + check.configure({"excluded_macros": "Q_OBJECT"}) + + def test_invalid_excluded_macros_element_type_raises(self) -> None: + """A list containing a non-string element raises ConfigError.""" + check = EnforceClassMemberOrder() + with pytest.raises(ConfigError): + check.configure({"excluded_macros": [123]}) + + class TestEnforceClassMemberOrderCheckId: """The check id is correct and the check is selectable.""" From 9bf62a9f2194b7a52ab740f1b1b4853444abd6d6 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Tue, 29 Sep 2026 21:24:47 +0200 Subject: [PATCH 3/5] Create ast cpp check for correct ordering of class members and functions Fixes #139 --- .../ast/test_enforce_class_member_order.py | 52 +++---------------- 1 file changed, 6 insertions(+), 46 deletions(-) diff --git a/tests/cpp/ast/test_enforce_class_member_order.py b/tests/cpp/ast/test_enforce_class_member_order.py index 806f5e8..f978b0e 100644 --- a/tests/cpp/ast/test_enforce_class_member_order.py +++ b/tests/cpp/ast/test_enforce_class_member_order.py @@ -102,14 +102,7 @@ class TestOutOfOrderFlagged: def test_private_before_public_member_flagged(self, tmp_path: Path) -> None: """Test private before public member flagged.""" - code = ( - "class C {\n" - "private:\n" - " int b;\n" - "public:\n" - " int a;\n" - "};\n" - ) + code = "class C {\nprivate:\n int b;\npublic:\n int a;\n};\n" diags = _diags(code, tmp_path) assert len(diags) == 1 assert "public member variable 'a'" in diags[0] @@ -117,14 +110,7 @@ def test_private_before_public_member_flagged(self, tmp_path: Path) -> None: def test_protected_before_public_member_flagged(self, tmp_path: Path) -> None: """Test protected before public member flagged.""" - code = ( - "class C {\n" - "protected:\n" - " int b;\n" - "public:\n" - " int a;\n" - "};\n" - ) + code = "class C {\nprotected:\n int b;\npublic:\n int a;\n};\n" diags = _diags(code, tmp_path) assert len(diags) == 1 assert "public member variable 'a'" in diags[0] @@ -141,14 +127,7 @@ def test_public_function_before_private_member_flagged( self, tmp_path: Path ) -> None: """Test public function before private member flagged.""" - code = ( - "class C {\n" - "public:\n" - " void f();\n" - "private:\n" - " int a;\n" - "};\n" - ) + code = "class C {\npublic:\n void f();\nprivate:\n int a;\n};\n" diags = _diags(code, tmp_path) assert len(diags) == 1 assert "private member variable 'a'" in diags[0] @@ -157,14 +136,7 @@ def test_private_function_before_public_function_flagged( self, tmp_path: Path ) -> None: """Test private function before public function flagged.""" - code = ( - "class C {\n" - "private:\n" - " void g();\n" - "public:\n" - " void f();\n" - "};\n" - ) + code = "class C {\nprivate:\n void g();\npublic:\n void f();\n};\n" diags = _diags(code, tmp_path) assert len(diags) == 1 assert "public member function 'f'" in diags[0] @@ -195,13 +167,7 @@ class TestStaticAndSpecialMembers: def test_static_member_treated_as_variable(self, tmp_path: Path) -> None: """Test static member treated as variable.""" - code = ( - "class C {\n" - "public:\n" - " void f();\n" - " static int count;\n" - "};\n" - ) + code = "class C {\npublic:\n void f();\n static int count;\n};\n" diags = _diags(code, tmp_path) assert len(diags) == 1 assert "public member variable 'count'" in diags[0] @@ -289,11 +255,5 @@ def test_check_id(self) -> None: def test_clean_file_produces_no_diagnostics(self, tmp_path: Path) -> None: """Test clean file produces no diagnostics.""" - code = ( - "class C {\n" - "public:\n" - " int a;\n" - " void f();\n" - "};\n" - ) + code = "class C {\npublic:\n int a;\n void f();\n};\n" assert _diags(code, tmp_path) == [] From 4aa13f76ab854ef2f9a895dfdaea53b4b1dd4473 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Tue, 29 Sep 2026 21:28:11 +0200 Subject: [PATCH 4/5] Create ast cpp check for correct ordering of class members and functions Fixes #139 --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c08f7ed..46777a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,14 +4,13 @@ All notable changes to this project will be documented in this file. ## Next Release - - ### Features #### CPP Rules - 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 + ## [0.7.0](https://github.com/repo/owner/releases/tag/0.7.0) - 2026-09-28 ### Python Requirement From a10b015ee30e8d96191f468a69e084754a677cd2 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Tue, 29 Sep 2026 21:50:54 +0200 Subject: [PATCH 5/5] Check that public members and functions DONT start with an underscore Fixes #140 --- CHANGELOG.md | 1 + .../enforce_no_public_leading_underscore.rst | 43 +++ docs/source/sections/ast_checks/index.rst | 4 + .../enforce_no_public_leading_underscore.py | 275 ++++++++++++++++++ src/devops/cpp/ast/registry.py | 4 + ...st_enforce_no_public_leading_underscore.py | 241 +++++++++++++++ 6 files changed, 568 insertions(+) create mode 100644 docs/source/sections/ast_checks/enforce_no_public_leading_underscore.rst create mode 100644 src/devops/cpp/ast/checks/enforce_no_public_leading_underscore.py create mode 100644 tests/cpp/ast/test_enforce_no_public_leading_underscore.py 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) == []