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

I tried to do automated testing based on guidance from #3194 (comment)

Manual testing results are posted in #3194 (comment)

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.17%. Comparing base (4cde128) to head (473c627).
⚠️ Report is 17 commits behind head on devel.

Additional details and impacted files

@@            Coverage Diff             @@
##            devel    #3194      +/-   ##
==========================================
- Coverage   91.25%   91.17%   -0.08%     
==========================================
  Files         204      206       +2     
  Lines        8965     9031      +66     
==========================================
+ Hits         8181     8234      +53     
- Misses        784      797      +13     

🚀 New features to boost your workflow:

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

[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.

[BaLiKfromUA]

@horenmar I pushed my trial to implement your draft but I am not sure that did everything correctly with cmake setup and python script scope so bear with me please :)

Few comments:

• I am not sure how to trigger Github Actions, I assume it requires your approval for workflow, right?
• Existing clang-tidy version in CI is too old to test my implementation (I put some comments why). For now, I decided to have a separate CI step with newer version of clang-tidy but maybe you want to bump it for everything. But IMO it would be better to make it in a separate PR and then rebase/refactor this one.

Thank you for your review and recommendation!

[BaLiKfromUA]

One potential issue that I spotted during self-review is exception throwing from this expression.

We might want to rewrite this line with something like:

  bool catchInternalAssertionResult = false; 
  CATCH_TRY { 
     catchInternalAssertionResult = static_cast<bool>( __VA_ARGS__ ); 
  } CATCH_CATCH_ALL { 
     // do nothing?
  } 

Otherwise, we introduce a false-negative for this strange case:

TEST_CASE( "CHECK with exception inside does not hide warning" ) {
    std::optional<int> opt;
    CHECK((opt ? true :  throw std::runtime_error("empty"))); 
    CHECK(*opt == 42); // expect-warning: bugprone-unchecked-optional-access
}

I think this case is strange because:

• Usually, an exception comes via other function/method call e.g CHECK(vector.at(123) == 42) so Dataflow analysis would not catch it IIRC.
• I don't think that people would see this frequently if they don't write throw directly inside assertion :)

I decided to flag it anyway: I don't mind to modify my implementation but I don't know if it adds enough benefit for additional complexity.

Because if we go this route, we might need to adjust handling of REQUIRE_FALSE as well, but maybe I am overthinking...