Model REQUIRE/REQUIRE_FALSE and REQUIRE_THROWS/NOTHROW/THROWS_AS early termination for Static Analysis mode - #3194
Open
BaLiKfromUA wants to merge 6 commits into
Open
Model REQUIRE/REQUIRE_FALSE and REQUIRE_THROWS/NOTHROW/THROWS_AS early termination for Static Analysis mode#3194BaLiKfromUA wants to merge 6 commits into
REQUIRE/REQUIRE_FALSE and REQUIRE_THROWS/NOTHROW/THROWS_AS early termination for Static Analysis mode#3194BaLiKfromUA wants to merge 6 commits into
Conversation
Under `CATCH_CONFIG_EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT`, `REQUIRE( expr )` now evaluates `expr` directly and marks the failing path with `Catch::Detail::Unreachable()`, instead of routing the expression through `Catch::AssertionHandler`, which single-TU analyzers cannot see through. `CHECK` keeps falling through, so assertions that do not stop the test case keep being reported. `Unreachable()` is used rather than a throw because that is what `FAIL` and `SKIP` already use, and because it also works when exceptions are disabled. Related to catchorg#3170
In static analysis mode both macros expand to a plain `if` over the user's expression, so that the analyzer sees the branch condition directly, instead of `Catch::Detail::lastAssertionPassed()`, whose value it cannot know. Neither macro stops the test case when the expression is false, so there is no `Unreachable()` on either path.
`REQUIRE_NOTHROW` marks its `catch( ... )` path unreachable, so the code after it is only reachable when the expression did not throw.
`REQUIRE_THROWS` is the opposite: the path where the expression did not throw is the unreachable one.
Like `REQUIRE_THROWS`, but only `exceptionType` counts as the expected exception.
The expansions in this mode use symbols that are left undefined on purpose, so the file is compiled but never linked, and compiling it is the test.
Author
Manual tests
|
Codecov Report✅ All modified and coverable lines are covered by tests. 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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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'sbugprone-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
REQUIREmacro by usingCatch::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:
REQUIRE_THATREQUIRE_THROWS_MATCHESREQUIRE_THROWS_WITH