From cdcbdc3a22096fa1ced7786ec93a3291d65247b1 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Wed, 30 Sep 2026 12:04:25 +0200 Subject: [PATCH 1/2] Disallow using delete in GTest teardown functions via ast cpp checker Fixes #145 --- CHANGELOG.md | 1 + docs/source/sections/ast_checks/index.rst | 5 + .../no_delete_in_gtest_teardown.rst | 49 ++++++ .../ast/checks/no_delete_in_gtest_teardown.py | 113 +++++++++++++ src/devops/cpp/ast/registry.py | 2 + .../ast/test_no_delete_in_gtest_teardown.py | 150 ++++++++++++++++++ 6 files changed, 320 insertions(+) create mode 100644 docs/source/sections/ast_checks/no_delete_in_gtest_teardown.rst create mode 100644 src/devops/cpp/ast/checks/no_delete_in_gtest_teardown.py create mode 100644 tests/cpp/ast/test_no_delete_in_gtest_teardown.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 30571c3..35d3cb2 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 `noDeleteInGtestTeardown` AST check flagging `delete`/`delete[]` expressions anywhere inside a member function named `TearDown`, `TearDownTestSuite`, or `TearDownTestCase` — the fixture-teardown hooks GTest calls by name. Manually deleting a raw pointer there is fragile (a `SetUp()` that throws or returns early skips the matching `delete`, and a test body that already freed the pointer causes a double-free); prefer an RAII/smart-pointer owner instead. Only a literal `delete`/`delete[]` lexically inside the teardown function's own body is flagged, not calls into other functions. Takes no configuration - 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/index.rst b/docs/source/sections/ast_checks/index.rst index 6805936..39bf524 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -93,6 +93,10 @@ Available checks * - :doc:`noThrowParen ` - ``throw(...)`` wrapping the whole thrown expression. - None + * - :doc:`noDeleteInGtestTeardown ` + - ``delete``/``delete[]`` inside a GTest ``TearDown``/``TearDownTestSuite``/ + ``TearDownTestCase`` function. + - None .. toctree:: :maxdepth: 1 @@ -107,3 +111,4 @@ Available checks no_global_using no_global_using_enum no_throw_paren + no_delete_in_gtest_teardown diff --git a/docs/source/sections/ast_checks/no_delete_in_gtest_teardown.rst b/docs/source/sections/ast_checks/no_delete_in_gtest_teardown.rst new file mode 100644 index 0000000..2320a91 --- /dev/null +++ b/docs/source/sections/ast_checks/no_delete_in_gtest_teardown.rst @@ -0,0 +1,49 @@ +noDeleteInGtestTeardown +========================= + +Flags any ``delete``/``delete[]`` expression appearing anywhere inside a +member function named ``TearDown``, ``TearDownTestSuite``, or +``TearDownTestCase`` — the fixture-teardown hooks GTest calls by name. +Manually deleting a raw pointer there is fragile: if ``SetUp()`` throws or +returns early, the matching ``delete`` in ``TearDown()`` never runs, and if +a test body already frees the pointer, the ``TearDown()`` delete +double-frees it. + +.. code-block:: cpp + + class FooTest : public ::testing::Test { + protected: + void TearDown() override { + delete ptr_; // Bad — flagged. + } + + Foo* ptr_; + }; + + class BarTest : public ::testing::Test { + protected: + void TearDown() override { + ptr_.reset(); // Good — RAII, not flagged. + } + + std::unique_ptr ptr_; + }; + +Only a literal ``delete``/``delete[]`` lexically inside the teardown +function's own body is flagged — a call to some other function that itself +deletes something is not traced into. A declaration with no body (e.g. a +pure-virtual ``TearDown() = 0``) is never flagged, and neither is a free +function of the same name that isn't a member function. + +Configuration +-------------- + +This check takes no configuration. + +Disabling +---------- + +.. code-block:: toml + + [cpp] + ast_check_disabled_ids = ["noDeleteInGtestTeardown"] diff --git a/src/devops/cpp/ast/checks/no_delete_in_gtest_teardown.py b/src/devops/cpp/ast/checks/no_delete_in_gtest_teardown.py new file mode 100644 index 0000000..f7acc2f --- /dev/null +++ b/src/devops/cpp/ast/checks/no_delete_in_gtest_teardown.py @@ -0,0 +1,113 @@ +"""Flag `delete`/`delete[]` expressions inside a GTest teardown function. + +A member function named ``TearDown``, ``TearDownTestSuite``, or +``TearDownTestCase`` — the fixture-teardown hooks GTest calls by name +(``TearDown()`` after every test, the other two once per test suite) — is +reported for any ``delete``/``delete[]`` expression appearing anywhere in +its body. Manually deleting a raw pointer there is exactly the kind of +lifetime bookkeeping RAII/smart pointers exist to make unnecessary, and it's +especially fragile in a teardown hook: if ``SetUp()`` throws or returns +early, the matching ``delete`` in ``TearDown()`` never runs, and if a test +body already frees the pointer, the ``TearDown()`` delete double-frees it. +Prefer ``std::unique_ptr``/``std::shared_ptr`` (or another RAII owner) for +anything the fixture needs to release, so its lifetime doesn't depend on a +teardown hook running at all. + +Only a literal ``delete``/``delete[]`` lexically inside the teardown +function's own body is flagged — a call to some other function that itself +deletes something is not traced into. + +To disable this check for a project set:: + + [cpp] + ast_check_disabled_ids = ["noDeleteInGtestTeardown"] +""" + +from __future__ import annotations + +import clang.cindex as clang + +from devops.cpp.ast.base import Check, Diagnostic + +# GTest calls these fixture methods by name; no base-class relationship is +# required by the framework itself, so none is checked here either. +_TEARDOWN_NAMES = frozenset( + ( + "TearDown", + "TearDownTestSuite", + "TearDownTestCase", + ) +) + + +class NoDeleteInGtestTeardown(Check): + """Flag `delete`/`delete[]` expressions inside a GTest teardown function.""" + + id = "noDeleteInGtestTeardown" + + def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: + """Flag any delete-expression inside a GTest teardown function body. + + Parameters + ---------- + cursor: clang.Cursor + The AST node currently being visited. + filename: str + Path of the file being checked. + + Returns + ------- + list[Diagnostic] + One diagnostic per `delete`/`delete[]` expression found inside + `cursor`'s body, if `cursor` is a GTest teardown function; + otherwise an empty list. + + """ + if cursor.kind != clang.CursorKind.CXX_METHOD: + return [] + if cursor.spelling not in _TEARDOWN_NAMES: + return [] + if not cursor.is_definition(): + return [] + + return [ + self._make_diagnostic(delete_expr, cursor.spelling, filename) + for delete_expr in cursor.walk_preorder() + if delete_expr.kind == clang.CursorKind.CXX_DELETE_EXPR + ] + + def _make_diagnostic( + self, delete_expr: clang.Cursor, function_name: str, filename: str + ) -> Diagnostic: + """Build the diagnostic for one flagged delete-expression. + + Parameters + ---------- + delete_expr: clang.Cursor + The `CXX_DELETE_EXPR` cursor to report. + function_name: str + Name of the enclosing teardown function, for the message. + filename: str + Path of the file being checked. + + Returns + ------- + Diagnostic + The diagnostic for `delete_expr`. + + """ + tokens = [t.spelling for t in delete_expr.get_tokens()] + spelling = "delete[]" if tokens[1:2] == ["["] else "delete" + loc = delete_expr.location + return Diagnostic( + file=filename, + line=loc.line, + column=loc.column, + message=( + f"do not use '{spelling}' inside GTest teardown function " + f"'{function_name}()' — prefer an RAII/smart-pointer owner " + "whose lifetime doesn't depend on teardown running" + ), + check_id=self.id, + severity="style", + ) diff --git a/src/devops/cpp/ast/registry.py b/src/devops/cpp/ast/registry.py index 2b0cfe4..22b47cc 100644 --- a/src/devops/cpp/ast/registry.py +++ b/src/devops/cpp/ast/registry.py @@ -21,6 +21,7 @@ ) 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_delete_in_gtest_teardown import NoDeleteInGtestTeardown from devops.cpp.ast.checks.no_global_using import NoGlobalUsing from devops.cpp.ast.checks.no_global_using_enum import NoGlobalUsingEnum from devops.cpp.ast.checks.no_throw_paren import NoThrowParen @@ -40,6 +41,7 @@ EnforceNoPublicLeadingUnderscore(), EnforceParamNameForType(), MacroReplacement(), + NoDeleteInGtestTeardown(), NoGlobalUsing(), NoGlobalUsingEnum(), NoThrowParen(), diff --git a/tests/cpp/ast/test_no_delete_in_gtest_teardown.py b/tests/cpp/ast/test_no_delete_in_gtest_teardown.py new file mode 100644 index 0000000..e878e02 --- /dev/null +++ b/tests/cpp/ast/test_no_delete_in_gtest_teardown.py @@ -0,0 +1,150 @@ +"""Tests for the NoDeleteInGtestTeardown AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.no_delete_in_gtest_teardown import NoDeleteInGtestTeardown +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [NoDeleteInGtestTeardown()] +_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 TestDeleteInTeardownFlagged: + """`delete`/`delete[]` in a recognised teardown hook is flagged.""" + + def test_delete_in_teardown_flagged(self, tmp_path: Path) -> None: + """Test delete in TearDown flagged.""" + code = ( + "struct FooTest {\n" + " void TearDown() {\n" + " int* p = new int(1);\n" + " delete p;\n" + " }\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "'delete'" in diags[0] + assert "TearDown()" in diags[0] + + def test_delete_array_in_teardown_flagged(self, tmp_path: Path) -> None: + """Test delete[] in TearDown flagged.""" + code = ( + "struct FooTest {\n" + " void TearDown() {\n" + " int* p = new int[3];\n" + " delete[] p;\n" + " }\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "'delete[]'" in diags[0] + + def test_delete_in_teardown_test_suite_flagged(self, tmp_path: Path) -> None: + """Test delete in TearDownTestSuite flagged.""" + code = ( + "struct FooTest {\n" + " static void TearDownTestSuite() {\n" + " delete shared_;\n" + " }\n" + " static int* shared_;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "TearDownTestSuite()" in diags[0] + + def test_delete_in_teardown_test_case_flagged(self, tmp_path: Path) -> None: + """Test delete in TearDownTestCase flagged.""" + code = ( + "struct FooTest {\n" + " static void TearDownTestCase() {\n" + " delete shared_;\n" + " }\n" + " static int* shared_;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "TearDownTestCase()" in diags[0] + + def test_multiple_deletes_all_flagged(self, tmp_path: Path) -> None: + """Test multiple deletes all flagged.""" + code = ( + "struct FooTest {\n" + " void TearDown() {\n" + " int* a = nullptr;\n" + " int* b = nullptr;\n" + " delete a;\n" + " delete b;\n" + " }\n" + "};\n" + ) + assert len(_diags(code, tmp_path)) == 2 + + def test_delete_in_nested_lambda_flagged(self, tmp_path: Path) -> None: + """Test delete inside a lambda defined in TearDown is still flagged.""" + code = ( + "struct FooTest {\n" + " void TearDown() {\n" + " auto f = [](int* p) { delete p; };\n" + " f(nullptr);\n" + " }\n" + "};\n" + ) + assert len(_diags(code, tmp_path)) == 1 + + +class TestDeleteElsewhereAllowed: + """`delete` outside a recognised teardown hook is not flagged.""" + + def test_delete_in_set_up_allowed(self, tmp_path: Path) -> None: + """Test delete in SetUp allowed.""" + code = "struct FooTest {\n void SetUp() {\n delete p;\n }\n};\n" + assert _diags(code, tmp_path) == [] + + def test_delete_in_unrelated_method_allowed(self, tmp_path: Path) -> None: + """Test delete in an unrelated method allowed.""" + code = "struct S {\n void Cleanup() {\n delete p;\n }\n};\n" + assert _diags(code, tmp_path) == [] + + def test_delete_in_free_function_named_teardown_allowed( + self, tmp_path: Path + ) -> None: + """Test delete in a free function (not a method) named TearDown allowed.""" + code = "void TearDown() {\n int* p = nullptr;\n delete p;\n}\n" + assert _diags(code, tmp_path) == [] + + def test_teardown_declaration_without_body_allowed(self, tmp_path: Path) -> None: + """Test a TearDown declaration with no body is not flagged.""" + code = "struct Base {\n virtual void TearDown() = 0;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_clean_teardown_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = "struct FooTest {\n void TearDown() {\n ptr_.reset();\n }\n};\n" + assert _diags(code, tmp_path) == [] + + +class TestNoDeleteInGtestTeardownCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert NoDeleteInGtestTeardown().id == "noDeleteInGtestTeardown" From 63966e401f5b1331e1a465ff77998a1e7ff62ee4 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Wed, 30 Sep 2026 21:11:22 +0200 Subject: [PATCH 2/2] Disallow using delete in GTest teardown functions via ast cpp checker Fixes #145 --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88275f1..cafb026 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. ## Next Release +### Features + +#### CPP Rules + +- Add `noDeleteInGtestTeardown` AST check flagging `delete`/`delete[]` expressions anywhere inside a member function named `TearDown`, `TearDownTestSuite`, or `TearDownTestCase` — the fixture-teardown hooks GTest calls by name. Manually deleting a raw pointer there is fragile (a `SetUp()` that throws or returns early skips the matching `delete`, and a test body that already freed the pointer causes a double-free); prefer an RAII/smart-pointer owner instead. Only a literal `delete`/`delete[]` lexically inside the teardown function's own body is flagged, not calls into other functions. Takes no configuration + ## [0.8.0](https://github.com/repo/owner/releases/tag/0.8.0) - 2026-09-30 @@ -11,7 +17,6 @@ All notable changes to this project will be documented in this file. #### CPP Rules -- Add `noDeleteInGtestTeardown` AST check flagging `delete`/`delete[]` expressions anywhere inside a member function named `TearDown`, `TearDownTestSuite`, or `TearDownTestCase` — the fixture-teardown hooks GTest calls by name. Manually deleting a raw pointer there is fragile (a `SetUp()` that throws or returns early skips the matching `delete`, and a test body that already freed the pointer causes a double-free); prefer an RAII/smart-pointer owner instead. Only a literal `delete`/`delete[]` lexically inside the teardown function's own body is flagged, not calls into other functions. Takes no configuration - 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