diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b5eb09..0b85dca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file. ## Next Release +### Features + +#### CPP Rules + +- Add `noNewInGtestSetup` AST check flagging `new` expressions anywhere inside a member function named `SetUp`, `SetUpTestSuite`, or `SetUpTestCase` — the fixture-setup hooks GTest calls by name. A raw pointer allocated there needs a matching manual release whose timing depends on a separate teardown hook firing correctly later; prefer an RAII/smart-pointer owner instead. Only a literal `new` lexically inside the setup function's own body is flagged, not calls into other functions. Takes no configuration +- 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 diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index 6805936..5670f36 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -93,6 +93,14 @@ Available checks * - :doc:`noThrowParen ` - ``throw(...)`` wrapping the whole thrown expression. - None + * - :doc:`noDeleteInGtestTeardown ` + - ``delete``/``delete[]`` inside a GTest ``TearDown``/``TearDownTestSuite``/ + ``TearDownTestCase`` function. + - None + * - :doc:`noNewInGtestSetup ` + - ``new`` inside a GTest ``SetUp``/``SetUpTestSuite``/``SetUpTestCase`` + function. + - None .. toctree:: :maxdepth: 1 @@ -107,3 +115,5 @@ Available checks no_global_using no_global_using_enum no_throw_paren + no_delete_in_gtest_teardown + no_new_in_gtest_setup 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/docs/source/sections/ast_checks/no_new_in_gtest_setup.rst b/docs/source/sections/ast_checks/no_new_in_gtest_setup.rst new file mode 100644 index 0000000..d7a5607 --- /dev/null +++ b/docs/source/sections/ast_checks/no_new_in_gtest_setup.rst @@ -0,0 +1,48 @@ +noNewInGtestSetup +==================== + +Flags any ``new`` expression appearing anywhere inside a member function +named ``SetUp``, ``SetUpTestSuite``, or ``SetUpTestCase`` — the +fixture-setup hooks GTest calls by name. A raw pointer allocated there needs +a matching manual release, and whether that release runs depends on a +separate teardown hook firing later — see also +:doc:`noDeleteInGtestTeardown `. + +.. code-block:: cpp + + class FooTest : public ::testing::Test { + protected: + void SetUp() override { + ptr_ = new Foo(); // Bad — flagged. + } + + Foo* ptr_; + }; + + class BarTest : public ::testing::Test { + protected: + void SetUp() override { + ptr_ = std::make_unique(); // Good — RAII, not flagged. + } + + std::unique_ptr ptr_; + }; + +Only a literal ``new`` expression lexically inside the setup function's own +body is flagged — a call to some other function that itself allocates is +not traced into. A declaration with no body (e.g. a pure-virtual +``SetUp() = 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 = ["noNewInGtestSetup"] 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/checks/no_new_in_gtest_setup.py b/src/devops/cpp/ast/checks/no_new_in_gtest_setup.py new file mode 100644 index 0000000..7622e98 --- /dev/null +++ b/src/devops/cpp/ast/checks/no_new_in_gtest_setup.py @@ -0,0 +1,111 @@ +"""Flag `new` expressions inside a GTest setup function. + +A member function named ``SetUp``, ``SetUpTestSuite``, or ``SetUpTestCase`` +— the fixture-setup hooks GTest calls by name (``SetUp()`` before every +test, the other two once per test suite) — is reported for any ``new`` +expression appearing anywhere in its body. A raw pointer allocated there +needs a matching manual release, and whether that release actually runs +depends on a separate teardown hook firing later (see also +:doc:`noDeleteInGtestTeardown `) +— if `SetUp()` itself throws partway through, or a later `SetUp()` call in +the same fixture leaks the previous allocation, the memory is never freed. +Prefer ``std::unique_ptr``/``std::shared_ptr`` (or another RAII owner) so +the allocation's lifetime is tied to the fixture object itself rather than +to a hook running at the right time. + +Only a literal ``new`` expression lexically inside the setup function's own +body is flagged — a call to some other function that itself allocates is +not traced into. + +To disable this check for a project set:: + + [cpp] + ast_check_disabled_ids = ["noNewInGtestSetup"] +""" + +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. +_SETUP_NAMES = frozenset( + ( + "SetUp", + "SetUpTestSuite", + "SetUpTestCase", + ) +) + + +class NoNewInGtestSetup(Check): + """Flag `new` expressions inside a GTest setup function.""" + + id = "noNewInGtestSetup" + + def visit(self, cursor: clang.Cursor, filename: str) -> list[Diagnostic]: + """Flag any new-expression inside a GTest setup 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 `new` expression found inside `cursor`'s + body, if `cursor` is a GTest setup function; otherwise an empty + list. + + """ + if cursor.kind != clang.CursorKind.CXX_METHOD: + return [] + if cursor.spelling not in _SETUP_NAMES: + return [] + if not cursor.is_definition(): + return [] + + return [ + self._make_diagnostic(new_expr, cursor.spelling, filename) + for new_expr in cursor.walk_preorder() + if new_expr.kind == clang.CursorKind.CXX_NEW_EXPR + ] + + def _make_diagnostic( + self, new_expr: clang.Cursor, function_name: str, filename: str + ) -> Diagnostic: + """Build the diagnostic for one flagged new-expression. + + Parameters + ---------- + new_expr: clang.Cursor + The `CXX_NEW_EXPR` cursor to report. + function_name: str + Name of the enclosing setup function, for the message. + filename: str + Path of the file being checked. + + Returns + ------- + Diagnostic + The diagnostic for `new_expr`. + + """ + loc = new_expr.location + return Diagnostic( + file=filename, + line=loc.line, + column=loc.column, + message=( + f"do not use 'new' inside GTest setup function " + f"'{function_name}()' — prefer an RAII/smart-pointer owner " + "whose lifetime doesn't depend on a later teardown hook" + ), + check_id=self.id, + severity="style", + ) diff --git a/src/devops/cpp/ast/registry.py b/src/devops/cpp/ast/registry.py index 2b0cfe4..4e2f73e 100644 --- a/src/devops/cpp/ast/registry.py +++ b/src/devops/cpp/ast/registry.py @@ -21,8 +21,10 @@ ) 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_new_in_gtest_setup import NoNewInGtestSetup from devops.cpp.ast.checks.no_throw_paren import NoThrowParen from devops.logger import cpp_check_logger @@ -40,8 +42,10 @@ EnforceNoPublicLeadingUnderscore(), EnforceParamNameForType(), MacroReplacement(), + NoDeleteInGtestTeardown(), NoGlobalUsing(), NoGlobalUsingEnum(), + NoNewInGtestSetup(), 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" diff --git a/tests/cpp/ast/test_no_new_in_gtest_setup.py b/tests/cpp/ast/test_no_new_in_gtest_setup.py new file mode 100644 index 0000000..d5b38df --- /dev/null +++ b/tests/cpp/ast/test_no_new_in_gtest_setup.py @@ -0,0 +1,139 @@ +"""Tests for the NoNewInGtestSetup AST check.""" + +from __future__ import annotations + +import typing + +import pytest + +from devops.cpp.ast.checks.no_new_in_gtest_setup import NoNewInGtestSetup +from devops.cpp.ast.engine import run_ast_checks + +if typing.TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("clang.cindex") + +_CHECK = [NoNewInGtestSetup()] +_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 TestNewInSetupFlagged: + """`new` in a recognised setup hook is flagged.""" + + def test_new_in_setup_flagged(self, tmp_path: Path) -> None: + """Test new in SetUp flagged.""" + code = "struct FooTest {\n void SetUp() {\n int* p = new int(1);\n }\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "'new'" in diags[0] + assert "SetUp()" in diags[0] + + def test_new_array_in_setup_flagged(self, tmp_path: Path) -> None: + """Test array new in SetUp flagged.""" + code = "struct FooTest {\n void SetUp() {\n int* p = new int[3];\n }\n};\n" + diags = _diags(code, tmp_path) + assert len(diags) == 1 + + def test_new_in_setup_test_suite_flagged(self, tmp_path: Path) -> None: + """Test new in SetUpTestSuite flagged.""" + code = ( + "struct FooTest {\n" + " static void SetUpTestSuite() {\n" + " shared_ = new int(1);\n" + " }\n" + " static int* shared_;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "SetUpTestSuite()" in diags[0] + + def test_new_in_setup_test_case_flagged(self, tmp_path: Path) -> None: + """Test new in SetUpTestCase flagged.""" + code = ( + "struct FooTest {\n" + " static void SetUpTestCase() {\n" + " shared_ = new int(1);\n" + " }\n" + " static int* shared_;\n" + "};\n" + ) + diags = _diags(code, tmp_path) + assert len(diags) == 1 + assert "SetUpTestCase()" in diags[0] + + def test_multiple_news_all_flagged(self, tmp_path: Path) -> None: + """Test multiple news all flagged.""" + code = ( + "struct FooTest {\n" + " void SetUp() {\n" + " int* a = new int(1);\n" + " int* b = new int(2);\n" + " }\n" + "};\n" + ) + assert len(_diags(code, tmp_path)) == 2 + + def test_new_in_nested_lambda_flagged(self, tmp_path: Path) -> None: + """Test new inside a lambda defined in SetUp is still flagged.""" + code = ( + "struct FooTest {\n" + " void SetUp() {\n" + " auto f = []() { return new int(1); };\n" + " f();\n" + " }\n" + "};\n" + ) + assert len(_diags(code, tmp_path)) == 1 + + +class TestNewElsewhereAllowed: + """`new` outside a recognised setup hook is not flagged.""" + + def test_new_in_tear_down_allowed(self, tmp_path: Path) -> None: + """Test new in TearDown allowed.""" + code = ( + "struct FooTest {\n void TearDown() {\n int* p = new int(1);\n }\n};\n" + ) + assert _diags(code, tmp_path) == [] + + def test_new_in_unrelated_method_allowed(self, tmp_path: Path) -> None: + """Test new in an unrelated method allowed.""" + code = "struct S {\n void Init() {\n int* p = new int(1);\n }\n};\n" + assert _diags(code, tmp_path) == [] + + def test_new_in_free_function_named_setup_allowed(self, tmp_path: Path) -> None: + """Test new in a free function (not a method) named SetUp allowed.""" + code = "void SetUp() {\n int* p = new int(1);\n}\n" + assert _diags(code, tmp_path) == [] + + def test_setup_declaration_without_body_allowed(self, tmp_path: Path) -> None: + """Test a SetUp declaration with no body is not flagged.""" + code = "struct Base {\n virtual void SetUp() = 0;\n};\n" + assert _diags(code, tmp_path) == [] + + def test_clean_setup_produces_no_diagnostics(self, tmp_path: Path) -> None: + """Test clean file produces no diagnostics.""" + code = ( + "struct FooTest {\n" + " void SetUp() {\n" + " ptr_ = std::make_unique(1);\n" + " }\n" + "};\n" + ) + assert _diags(code, tmp_path) == [] + + +class TestNoNewInGtestSetupCheckId: + """The check id is correct and the check is selectable.""" + + def test_check_id(self) -> None: + """Test check id.""" + assert NoNewInGtestSetup().id == "noNewInGtestSetup"