Model REQUIRE/REQUIRE_FALSE and REQUIRE_THROWS/NOTHROW/THROWS_AS early termination for Static Analysis mode

[BaLiKfromUA]

Description

This patch adds separate implementations of several assertion macros under CATCH_CONFIG_EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT, so that single-TU static analysis, such as clang-tidy's bugprone-unchecked-optional-access, can reason about Catch2's assertions.

The goal is to remove a class of false positives (in particular for flow-sensitive analysers) that users currently get in every test that guards with REQUIRE.

The main idea is to model early termination in case of REQUIRE macro by using Catch::Detail::Unreachable().

Testing

The mode's binaries are not runnable, so there is nothing for ctest to assert on; compilation is the only thing that can be checked automatically. I added X08-StaticAnalysisSupport.cpp, a compile-only test that instantiates every affected macro in this mode and fails the build if one of them stops compiling.

That test does not check that the analysis actually improved, so I verified that part by hand (see below).

GitHub Issues

Partially address #3170

Some discussed follow-ups have not been implemented yet:

• Support of REQUIRE_THAT
• Support of REQUIRE_THROWS_MATCHES
• Support of REQUIRE_THROWS_WITH

[BaLiKfromUA]

Manual tests

REQUIRE/REQUIRE_FALSE

Code example:

#include <catch2/catch_test_macros.hpp>
#include <optional>

TEST_CASE("demo_with_optional_safe") {
  std::optional<int> opt;
  REQUIRE(opt);
  CHECK(*opt == 42);
}

TEST_CASE("demo_with_optional_unsafe1") {
  std::optional<int> opt;
  REQUIRE_FALSE(opt);
  CHECK(*opt == 42); // true positive expected here
}

TEST_CASE("demo_with_optional_unsafe2") {
  std::optional<int> opt;
  CHECK(opt);
  CHECK(*opt == 42); // true positive expected here

  CHECK_FALSE(!opt.has_value());
  CHECK(*opt == 42); // true positive expected here
}

clang-tidy output:

3 warnings generated.
/home/balik/Desktop/Catch2/reproduction.cpp:13:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   13 |   CHECK(*opt == 42); // true positive expected here
      |          ^
/home/balik/Desktop/Catch2/reproduction.cpp:19:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   19 |   CHECK(*opt == 42); // true positive expected here
      |          ^
/home/balik/Desktop/Catch2/reproduction.cpp:22:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   22 |   CHECK(*opt == 42); // true positive expected here

CHECKED_IF / CHECKED_ELSE

Code example:

#include <catch2/catch_test_macros.hpp>
#include <optional>

TEST_CASE("checked_if_safe") {
  std::optional<int> opt;
  CHECKED_IF( opt ) {
    CHECK(*opt == 42); 
  }
}

TEST_CASE("checked_else_safe") {
  std::optional<int> opt;
  CHECKED_ELSE( opt ) {

  } else {
    CHECK(*opt == 42); 
  }
}

TEST_CASE("checked_if_unsafe") {
  std::optional<int> opt;
  CHECKED_IF( opt.has_value() ) {
    
  }
  CHECK(*opt == 42); // true positive expected here
}

clang-tidy output:

1 warning generated.
/home/balik/Desktop/Catch2/reproduction1.cpp:25:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   25 |   CHECK(*opt == 42); // true positive expected here

REQUIRE_NOTHROW

Code example:

#include <catch2/catch_test_macros.hpp>
#include <optional>
#include <stdexcept>

std::optional<int> getOpt();

TEST_CASE("nothrow_optional_safe") {
    std::optional<int> opt = getOpt();
    REQUIRE_NOTHROW( opt ? 0 : throw std::runtime_error("empty") );
    CHECK(*opt == 42);
}

TEST_CASE("nothrow_optional_unsafe") {
    std::optional<int> opt = getOpt();
    REQUIRE_NOTHROW( opt ? 0 : -1 );
    CHECK(*opt == 42); // true positive expected here
}

clang-tidy output:

1 warning generated.
/home/balik/Desktop/Catch2/reproduction2.cpp:16:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   16 |     CHECK(*opt == 42); // true positive expected here

REQUIRE_THROWS

Code example:

#include <catch2/catch_test_macros.hpp>
#include <optional>
#include <stdexcept>

std::optional<int> getOpt();

TEST_CASE("throws_optional_safe") {
    std::optional<int> opt = getOpt();
    REQUIRE_THROWS( opt ? throw std::runtime_error("has value") : 0 );
    CHECK(*opt == 42);
}


TEST_CASE("throws_optional_unsafe") {
    std::optional<int> opt = getOpt();
    CHECK_THROWS( opt ? throw std::runtime_error("has value") : 0 );
    CHECK(*opt == 42); // true positive expected here
}

clang-tidy output:

1 warning generated.
/home/balik/Desktop/Catch2/reproduction3.cpp:17:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   17 |     CHECK(*opt == 42); // true positive expected here

REQUIRE_THROWS_AS

Code example:

#include <catch2/catch_test_macros.hpp>
#include <optional>
#include <stdexcept>

struct MyExc : std::exception {};
std::optional<int> getOpt();

TEST_CASE("throws_as_optional_safe") {
    std::optional<int> opt = getOpt();
    REQUIRE_THROWS_AS( opt ? throw MyExc{} : 0, MyExc );
    CHECK(*opt == 42); 
}


TEST_CASE("throws_as_optional_unsafe") {
    std::optional<int> opt = getOpt();
    CHECK_THROWS_AS( opt ? throw MyExc{} : 0, MyExc );
    CHECK(*opt == 42); // true positive
}

clang-tidy output:

1 warning generated.
/home/balik/Desktop/Catch2/reproduction4.cpp:18:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
   18 |     CHECK(*opt == 42); // true positive

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.24%. Comparing base (4cde128) to head (9833b6c).
⚠️ Report is 4 commits behind head on devel.

Additional details and impacted files

@@            Coverage Diff             @@
##            devel    #3194      +/-   ##
==========================================
- Coverage   91.25%   91.24%   -0.01%     
==========================================
  Files         204      204              
  Lines        8965     8973       +8     
==========================================
+ Hits         8181     8187       +6     
- Misses        784      786       +2     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[horenmar]

You can drop all this, CI already runs clang-tidy.

[horenmar]

At a glance the changes look correct. The tests however, need to be done differently. As a rough draft:

• You should add the examples you manually tested into different .cpp files.
• You should then run clang-tidy over these files.
• The output from clang-tidy should be checked for the presence of the warnings -> the REQUIRE example should not have warning, the CHECK examples should have warning.

We already have some scripts that do their own builds of Catch2 in tests/TestScripts, e.g. in the DiscoverTests subfolder. I would probably add a tests/TestProjects/StaticAnalysis/ subfolder with new CMakeLists.txt, that adds the relevant source files as their own binaries excluded from full build. Then add a new script that invokes their build and captures stdout. By calling this script in the CI job with clang-tidy, the full build should be properly configured to give you clang-tidy's output instead of compiler's.