catchorg / catchorg/Catch2

[clang-tidy] Static analyzers can't see that a failed `REQUIRE` doesn't fall through, causing false positives (e.g. `bugprone-unchecked-optional-access`)

Open
#3,170 2 comments 1 reaction 0 assignees View on GitHub
Warnings issue
Dominant language
C++
Stars
21.5k
Forks
3.5k
Avg merge
3d 16h
Merged PRs (30d)
2

Description

**Describe the bug**

Consider the code below:
```cpp
TEST_CASE("demo_with_optional_safe") {
std::optional opt;
REQUIRE(opt);
CHECK(*opt == 42); // safe: false-positive
}

TEST_CASE("demo_with_optional_unsafe") {
std::optional opt;
CHECK(opt);
CHECK(*opt == 42); // unsafe
}
```

If we run the clang-tidy [bugprone-unchecked-optional-access](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unchecked-optional-access.html) check, which warns when an optional is accessed without a preceding check that it holds a value, we get 2 warnings:
```
:28:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
28 | CHECK(*opt == 42); // safe: false-positive
| ^
:35:10: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
35 | CHECK(*opt == 42); // unsafe
| ^
2 warnings generated.
Suppressed 2 warnings (2 NOLINT).
```

**Expected behavior**

The optional access in `"demo_with_optional_safe"` should not be flagged, since `REQUIRE` terminates the test on failure, guaranteeing `opt.has_value()` at the point of access.

The optional access in `"demo_with_optional_unsafe"` should be flagged, since `CHECK` doesn't terminate the test on failure, so the access is reachable with an empty optional.

**Reproduction steps**

[Same example in Compiler Explorer](https://godbolt.org/z/zaPzK6s6E)

**Additional context**

I'm addressing high-priority `bugprone-unchecked-optional-access` false positives at Bloomberg. I recently fixed a related false positive for code using GTest ([llvm/llvm-project#181737](https://github.com/llvm/llvm-project/issues/181737)), and ran into an identical issue with Catch2.

Because of how `REQUIRE` expands internally, clang-tidy's dataflow analysis can't tell that a failed assertion terminates the test, so it can't rule out the "unchecked" path.

I initially considered adding Catch2-specific AST matchers to clang-tidy to model `Catch::AssertionHandler`'s control flow directly, similar to the GTest fix. After discussing it internally, though, it looks like the fix belongs on the Catch2 side instead, since there is already a precedent for this kind of thing ([docs](https://github.com/catchorg/Catch2/blob/devel/docs/configuration.md#static-analysis-support)).

I wanted to raise this here before taking it to the clang-tidy maintainers. I believe the patch on Catch2 side will be much simpler and less error-prone.

During my experiments with CE, I put together a rough proof-of-concept that resolves the false positive. I am not proposing it as the final implementation, but sharing it as a starting point for discussion:
```cpp
#if defined(__clang_analyzer__)

#undef REQUIRE
#define REQUIRE(...) \
do { if (!(__VA_ARGS__)) Catch::throw_test_failure_exception(); } while(false)

#undef CHECK
#define CHECK(...) \
do { (void)(__VA_ARGS__); } while(false)

#endif
```

I'm happy to put together a proper patch if we agree this should be fixed inside Catch2. Keen to discuss the right approach here first.

Contributor guide

Open the contributing guide

Research direction

Start with the static-analysis support guidance in docs/configuration.md, then inspect how the REQUIRE and CHECK macros connect to Catch::AssertionHandler. Reproduce the example with clang-tidy's bugprone-unchecked-optional-access check and determine how the analyzer can distinguish terminating REQUIRE failures from non-terminating CHECK failures. Done means the safe access is not warned about while the unsafe access remains warned about.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.