#3194
Pull Requests/catchorg/Catch2/#3194

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

OpenBaLiKfromUAwants to merge intofrom
6 commits15 files changed+554-4

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 https://github.com/catchorg/Catch2/pull/3194#issuecomment-5400964184

Manual testing results are posted in https://github.com/catchorg/Catch2/pull/3194#issuecomment-5349279602

GitHub Issues

Partially address https://github.com/catchorg/Catch2/issues/3170

Some discussed follow-ups have not been implemented yet:

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

Activity

14
B
Model `REQUIRE_THROWS_AS/CHECK_THROWS_AS` for static analyzer
283a06f
B
Model `REQUIRE_THROWS/CHECK_THROWS` for static analyzer
fe280a1
B
Model `REQUIRE_NOTHROW/CHECK_NOTHROW` for static analyzer
1eb2633
B
Model `CHECKED_IF/CHECKED_ELSE` for static analyzer
7b4a4b2
B
Model `REQUIRE/CHECK` termination for static analyzer
874e864
BaLiKfromUA2mo ago

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[bot]2mo ago

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 91.18%. Comparing base (0809213) to head (21ba6fd).

Additional details and impacted files 
@@           Coverage Diff           @@
##            devel    #3194   +/-   ##
=======================================
  Coverage   91.18%   91.18%           
=======================================
  Files         205      205           
  Lines        9101     9101           
=======================================
  Hits         8298     8298           
  Misses        803      803
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
horenmarreviewed2mo ago
horenmar2mo ago

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.

B
Test static analysis support with clang-tidy
21ba6fd
BaLiKfromUA1mo ago

@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!

BaLiKfromUAreviewed1mo ago
BaLiKfromUA1mo ago

@horenmar friendly ping to not lose the PR

Thank you for your time and review :)