diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index ffa984f..793987b 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -15,7 +15,7 @@ jobs: strategy: fail-fast: false matrix: - python-version: [3.12, 3.13] + python-version: [3.13, 3.14] steps: - uses: actions/checkout@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index d1fdb94..1cb31d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file. ## Next Release +### Python Requirement + +- remove 3.12 dependeny +- add support for 3.14 + +### Features + +#### CPP Rules + +- Add `memberFunctionLeadingUnderscore` AST check flagging private and protected member functions (including static and template ones) whose name doesn't start with `_`, e.g. `void compute();` under `private:`/`protected:` should be `void _compute();`. Public methods are never checked, and 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. As with `memberLeadingUnderscore`, methods synthesized entirely by a macro invoked on the same source line are excluded too. Takes no configuration + +### Fixes + +#### Documentation + +- Fix `sphinx-build` failing under `-W` when the optional `ast` extra (`libclang`) isn't installed — as in the docs CI job, which only installs the `docs` extra. `memberFunctionLeadingUnderscore` bound a libclang ctypes function signature (`ctypes.POINTER(clang.Cursor)`) at module import time, which raised `TypeError: must be a ctypes type` against Sphinx's mocked `clang` module and broke autosummary for `devops.cpp` and everything that imports it (`add_license_header`, `cpp_checks`, `cpp_files`). The binding is now deferred to first use, when a real `clang.Cursor` is guaranteed to be available + ## [0.6.0](https://github.com/repo/owner/releases/tag/0.6.0) - 2026-09-27 diff --git a/docs/source/sections/ast_checks/enforce_member_function_leading_underscore.rst b/docs/source/sections/ast_checks/enforce_member_function_leading_underscore.rst new file mode 100644 index 0000000..4887947 --- /dev/null +++ b/docs/source/sections/ast_checks/enforce_member_function_leading_underscore.rst @@ -0,0 +1,51 @@ +memberFunctionLeadingUnderscore +================================= + +Flags any ordinary member function (including static and template member +functions) declared under a ``private:`` or ``protected:`` access specifier +whose name doesn't start with ``_``. Public member functions are never +flagged. + +.. code-block:: cpp + + class Widget { + public: + void resize(); // Good — public methods are not checked. + + private: + void compute(); // Bad — flagged, missing leading underscore. + void _compute(); // Good. + + protected: + void hook(); // Bad — flagged, missing leading underscore. + void _hook(); // Good. + }; + +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. + +As with :doc:`memberLeadingUnderscore `, +a member function 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 = ["memberFunctionLeadingUnderscore"] diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index aa40396..3a6c334 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -67,6 +67,9 @@ Available checks * - :doc:`memberLeadingUnderscore ` - A private or protected member variable not starting with ``_``. - None + * - :doc:`memberFunctionLeadingUnderscore ` + - A private or protected member function not starting with ``_``. + - None * - :doc:`paramNameForType ` - A parameter of a configured type not using its canonical name. - Required (``type_to_name``) @@ -88,6 +91,7 @@ Available checks :hidden: enforce_member_leading_underscore + enforce_member_function_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 888d373..95dcfae 100644 --- a/docs/source/sections/configuration.rst +++ b/docs/source/sections/configuration.rst @@ -217,8 +217,9 @@ Controls the checks run by :ref:`cpp_checks `. - ``[]`` - If non-empty, only AST checks whose ``id`` is in this list run (allowlist). Available check ids: ``memberLeadingUnderscore``, - ``paramNameForType``, ``macroReplacement``, ``noGlobalUsing``, - ``noGlobalUsingEnum``, ``noThrowParen`` (each documented below). + ``memberFunctionLeadingUnderscore``, ``paramNameForType``, + ``macroReplacement``, ``noGlobalUsing``, ``noGlobalUsingEnum``, + ``noThrowParen`` (each documented below). * - ``ast_check_disabled_ids`` - list of strings - ``[]`` @@ -268,8 +269,9 @@ 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 (``memberLeadingUnderscore``, -``paramNameForType``, ``macroReplacement``, ``noGlobalUsing``, -``noGlobalUsingEnum``, ``noThrowParen``). +``memberFunctionLeadingUnderscore``, ``paramNameForType``, +``macroReplacement``, ``noGlobalUsing``, ``noGlobalUsingEnum``, +``noThrowParen``). ``[file]`` ^^^^^^^^^^ diff --git a/pyproject.toml b/pyproject.toml index 3824748..356dbaf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ dynamic = ["version"] authors = [{ name = "Jakob Gamper", email = "97gamjak@gmail.com" }] description = "This package is a collection of DevOps related tools and scripts." readme = "README.md" -requires-python = ">=3.12" +requires-python = ">=3.13" dependencies = ["typer>=0.20.0"] [project.optional-dependencies] diff --git a/src/devops/cpp/ast/checks/enforce_member_function_leading_underscore.py b/src/devops/cpp/ast/checks/enforce_member_function_leading_underscore.py new file mode 100644 index 0000000..adb4b00 --- /dev/null +++ b/src/devops/cpp/ast/checks/enforce_member_function_leading_underscore.py @@ -0,0 +1,240 @@ +"""Enforce a leading underscore on private and protected member functions. + +Any ordinary member function (including static and template member +functions) declared under a ``private:`` or ``protected:`` access specifier +must have a name starting with ``_`` (e.g. ``_compute``, ``_isReady``). +Public member functions are never flagged, since they are part of the +class's external interface and not the concern of this check. + +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. + +As with `memberLeadingUnderscore`, a member function synthesized by a macro +invoked on the same source line is not flagged either. + +To disable this check for a project set:: + + [cpp] + ast_check_disabled_ids = ["memberFunctionLeadingUnderscore"] +""" + +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 member functions 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, + ) +) + +# 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, + ) +) + +_RESTRICTED_ACCESS = frozenset( + (clang.AccessSpecifier.PRIVATE, clang.AccessSpecifier.PROTECTED) +) + + +def _bind_overridden_cursors_ctypes() -> None: + """Bind the ctypes signatures for the two libclang functions used below. + + ``clang_getOverriddenCursors()``/``clang_disposeOverriddenCursors()`` + have no high-level wrapper in this libclang Python binding, so they're + bound here directly via ctypes (a stable part of the public libclang C + API since LLVM 3.x). 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 _is_restricted_member_function(cursor: clang.Cursor) -> bool: + """Return whether `cursor` is a private/protected member function candidate. + + Parameters + ---------- + cursor: clang.Cursor + The AST node to check. + + Returns + ------- + bool + True if `cursor` is a member function this check should inspect for + a leading underscore. + + """ + if cursor.kind not in _MEMBER_FUNCTION_KINDS: + return False + if cursor.access_specifier not in _RESTRICTED_ACCESS: + 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) + + +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 + + +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 EnforceMemberFunctionLeadingUnderscore(Check): + """Flag private/protected member functions without a leading underscore. + + Reporting is deferred to `finalize()` for the same reason as in + `EnforceMemberLeadingUnderscore`: whether a candidate was synthesized by + a macro can only be known once every ``MACRO_INSTANTIATION`` cursor in + the file has been seen. + """ + + id = "memberFunctionLeadingUnderscore" + + 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 methods 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 not _is_restricted_member_function(cursor): + return [] + + name = cursor.spelling + 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 methods not on a macro-instantiation line. + + Parameters + ---------- + filename: str + Path of the file being checked. + + Returns + ------- + list[Diagnostic] + One diagnostic per offending member function 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 function '{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 c557816..b77dc99 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_function_leading_underscore import ( + EnforceMemberFunctionLeadingUnderscore, +) from devops.cpp.ast.checks.enforce_member_leading_underscore import ( EnforceMemberLeadingUnderscore, ) @@ -27,6 +30,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] = [ + EnforceMemberFunctionLeadingUnderscore(), EnforceMemberLeadingUnderscore(), EnforceParamNameForType(), MacroReplacement(), diff --git a/src/devops/files/files.py b/src/devops/files/files.py index 39eb2e6..f3290bd 100644 --- a/src/devops/files/files.py +++ b/src/devops/files/files.py @@ -300,9 +300,7 @@ def file_exist( @contextmanager -def open_file( - file: str | Path, mode: str = "r" -) -> typing.Generator[typing.IO[str], None, None]: +def open_file(file: str | Path, mode: str = "r") -> typing.Generator[typing.IO[str]]: """Read the content of a file. Parameters diff --git a/tests/cpp/ast/test_enforce_member_function_leading_underscore.py b/tests/cpp/ast/test_enforce_member_function_leading_underscore.py new file mode 100644 index 0000000..18d5fb4 --- /dev/null +++ b/tests/cpp/ast/test_enforce_member_function_leading_underscore.py @@ -0,0 +1,189 @@ +"""Tests for the EnforceMemberFunctionLeadingUnderscore AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.enforce_member_function_leading_underscore import ( + EnforceMemberFunctionLeadingUnderscore, +) +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [EnforceMemberFunctionLeadingUnderscore()] +_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 TestPrivateMethodsFlagged: + """Private member functions without a leading underscore are flagged.""" + + def test_private_method_flagged(self, tmp_path: Path) -> None: + """Test private method flagged.""" + code = "class C {\nprivate:\n void compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "private member function 'compute'" in diags[0] + assert "'_compute'" in diags[0] + + def test_private_static_method_flagged(self, tmp_path: Path) -> None: + """Test private static method flagged.""" + code = "class C {\nprivate:\n static void compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + def test_private_template_method_flagged(self, tmp_path: Path) -> None: + """Test private template method flagged.""" + code = "class C {\nprivate:\n template void compute(T);\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "compute" in diags[0] + + def test_private_struct_method_flagged(self, tmp_path: Path) -> None: + """Test private struct method flagged.""" + code = "struct S {\nprivate:\n void compute();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + +class TestProtectedMethodsFlagged: + """Protected member functions without a leading underscore are flagged.""" + + def test_protected_method_flagged(self, tmp_path: Path) -> None: + """Test protected method flagged.""" + code = "class C {\nprotected:\n void hook();\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "protected member function 'hook'" in diags[0] + + +class TestMethodsAllowed: + """Public methods, prefixed methods, and language-fixed names are allowed.""" + + def test_public_method_allowed(self, tmp_path: Path) -> None: + """Test public method allowed.""" + code = "class C {\npublic:\n void compute();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_default_public_struct_method_allowed(self, tmp_path: Path) -> None: + """Test default public struct method allowed.""" + code = "struct S {\n void compute();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_private_method_with_underscore_allowed(self, tmp_path: Path) -> None: + """Test private method with underscore allowed.""" + code = "class C {\nprivate:\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 {\nprivate:\n C();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_destructor_allowed(self, tmp_path: Path) -> None: + """Test destructor allowed.""" + code = "class C {\nprivate:\n ~C();\n};\n" + assert _diags(code, tmp_path) == [] + + def test_copy_constructor_and_assignment_allowed(self, tmp_path: Path) -> None: + """Test copy constructor and assignment allowed.""" + code = ( + "class C {\nprivate:\n C(const C&);\n C& operator=(const 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 {\nprivate:\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 {\nprivate:\n operator int() const;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_overriding_method_allowed(self, tmp_path: Path) -> None: + """Test overriding method allowed.""" + code = ( + "class Base {\npublic:\n" + " virtual void doThing();\n" + " virtual ~Base() = default;\n" + "};\n" + "class C : public Base {\nprivate:\n" + " void doThing() override;\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + 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" + " private: \\\n" + " void testBody(); \\\n" + " };\n" + "DECLARE_FIXTURE(Foo)\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestMixedAccessSpecifiers: + """Only methods under a private/protected section are flagged.""" + + def test_only_restricted_methods_flagged(self, tmp_path: Path) -> None: + """Test only restricted 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) == 2 + assert any("priv" in d for d in diags) + assert any("prot" in d for d in diags) + + +class TestEnforceMemberFunctionLeadingUnderscoreCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert ( + EnforceMemberFunctionLeadingUnderscore().id + == "memberFunctionLeadingUnderscore" + ) + + def test_clean_file_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = ( + "class C {\nprivate:\n void _compute();\nprotected:\n" + " void _hook();\n};\n" + ) + assert _diags(code, tmp_path) == []