From b33a6f4ec8dd5df5f783f237595d65afec223d18 Mon Sep 17 00:00:00 2001 From: Jakob Gamper <97gamjak@gmail.com> Date: Wed, 30 Sep 2026 21:28:03 +0200 Subject: [PATCH] Disallow using new in Setup of GTEST Fixes #147 --- CHANGELOG.md | 1 + docs/source/sections/ast_checks/index.rst | 5 + .../ast_checks/no_new_in_gtest_setup.rst | 48 ++++++ .../cpp/ast/checks/no_new_in_gtest_setup.py | 111 ++++++++++++++ src/devops/cpp/ast/registry.py | 2 + tests/cpp/ast/test_no_new_in_gtest_setup.py | 139 ++++++++++++++++++ 6 files changed, 306 insertions(+) create mode 100644 docs/source/sections/ast_checks/no_new_in_gtest_setup.rst create mode 100644 src/devops/cpp/ast/checks/no_new_in_gtest_setup.py create mode 100644 tests/cpp/ast/test_no_new_in_gtest_setup.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cafb026..0b85dca 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 `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 diff --git a/docs/source/sections/ast_checks/index.rst b/docs/source/sections/ast_checks/index.rst index 39bf524..5670f36 100644 --- a/docs/source/sections/ast_checks/index.rst +++ b/docs/source/sections/ast_checks/index.rst @@ -97,6 +97,10 @@ Available checks - ``delete``/``delete[]`` inside a GTest ``TearDown``/``TearDownTestSuite``/ ``TearDownTestCase`` function. - None + * - :doc:`noNewInGtestSetup ` + - ``new`` inside a GTest ``SetUp``/``SetUpTestSuite``/``SetUpTestCase`` + function. + - None .. toctree:: :maxdepth: 1 @@ -112,3 +116,4 @@ Available checks 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_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_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 22b47cc..4e2f73e 100644 --- a/src/devops/cpp/ast/registry.py +++ b/src/devops/cpp/ast/registry.py @@ -24,6 +24,7 @@ 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 @@ -44,6 +45,7 @@ NoDeleteInGtestTeardown(), NoGlobalUsing(), NoGlobalUsingEnum(), + NoNewInGtestSetup(), NoThrowParen(), ] 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"