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