Repository navigation
Model REQUIRE/REQUIRE_FALSE and REQUIRE_THROWS/NOTHROW/THROWS_AS early termination for Static Analysis mode
#3194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
BaLiKfromUA
wants to merge
6
commits into
catchorg:devel
Choose a base branch
from
BaLiKfromUA:issues/3170
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+554
−4
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
874e864
Model `REQUIRE/CHECK` termination for static analyzer
BaLiKfromUA 7b4a4b2
Model `CHECKED_IF/CHECKED_ELSE` for static analyzer
BaLiKfromUA 1eb2633
Model `REQUIRE_NOTHROW/CHECK_NOTHROW` for static analyzer
BaLiKfromUA fe280a1
Model `REQUIRE_THROWS/CHECK_THROWS` for static analyzer
BaLiKfromUA 283a06f
Model `REQUIRE_THROWS_AS/CHECK_THROWS_AS` for static analyzer
BaLiKfromUA 21ba6fd
Test static analysis support with clang-tidy
BaLiKfromUA File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| # This configuration deliberately does not inherit the repository-wide one. | ||
| # | ||
| InheritParentConfig: false | ||
| Checks: '-*,bugprone-unchecked-optional-access' | ||
| WarningsAsErrors: '' | ||
| HeaderFilterRegex: '' | ||
| FormatStyle: none | ||
| CheckOptions: [] | ||
| ... |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| cmake_minimum_required(VERSION 3.16) | ||
| project(static-analysis-tests LANGUAGES CXX) | ||
|
|
||
| # std::optional, which is what bugprone-unchecked-optional-access reasons about | ||
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| add_subdirectory(${CATCH2_PATH} catch2-build) | ||
|
|
||
| set(SA_TEST_SOURCES | ||
| RequireTerminates.cpp | ||
| CheckedIf.cpp | ||
| NoThrowTerminates.cpp | ||
| ThrowsTerminates.cpp | ||
| ThrowsAsTerminates.cpp | ||
| ) | ||
|
|
||
| # These TUs compile but never link. | ||
| # Per-target clang-tidy keeps Catch2 itself out of the analysis. | ||
| foreach(sourceFile ${SA_TEST_SOURCES}) | ||
| get_filename_component(targetName "${sourceFile}" NAME_WE) | ||
| add_library(${targetName} OBJECT "${sourceFile}") | ||
| target_link_libraries(${targetName} PRIVATE Catch2::Catch2) | ||
| set_target_properties(${targetName} | ||
| PROPERTIES | ||
| CXX_CLANG_TIDY "${STATIC_ANALYSIS_CLANG_TIDY}" | ||
| ) | ||
| endforeach() |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
|
|
||
| // Copyright Catch2 Authors | ||
| // Distributed under the Boost Software License, Version 1.0. | ||
| // (See accompanying file LICENSE.txt or copy at | ||
| // https://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| // SPDX-License-Identifier: BSL-1.0 | ||
|
|
||
| #include <catch2/catch_test_macros.hpp> | ||
|
|
||
| #include <optional> | ||
|
|
||
| // Deliberately not defined, so that the analysis cannot see through it. | ||
| std::optional<int> maybe_value(); | ||
|
|
||
| TEST_CASE( "CHECKED_IF branches on the condition" ) { | ||
| std::optional<int> opt = maybe_value(); | ||
| CHECKED_IF( opt.has_value() ) { CHECK( *opt == 42 ); } | ||
| } | ||
|
|
||
| TEST_CASE( "CHECKED_ELSE branches on the negated condition" ) { | ||
| std::optional<int> opt = maybe_value(); | ||
| CHECKED_ELSE( opt.has_value() ) { CHECK( *opt == 42 ); } // expect-warning: bugprone-unchecked-optional-access | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
|
|
||
| // Copyright Catch2 Authors | ||
| // Distributed under the Boost Software License, Version 1.0. | ||
| // (See accompanying file LICENSE.txt or copy at | ||
| // https://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| // SPDX-License-Identifier: BSL-1.0 | ||
|
|
||
| #include <catch2/catch_test_macros.hpp> | ||
|
|
||
| #include <optional> | ||
| #include <stdexcept> | ||
|
|
||
| TEST_CASE( "REQUIRE_NOTHROW terminates the test case when the expression throws" ) { | ||
| std::optional<int> opt; | ||
| REQUIRE_NOTHROW( opt ? 0 : throw std::runtime_error("empty") ); | ||
| CHECK( *opt == 42 ); | ||
| } | ||
|
|
||
| TEST_CASE( "CHECK_NOTHROW keeps going when the expression throws" ) { | ||
| std::optional<int> opt; | ||
| CHECK_NOTHROW( opt ? 0 : throw std::runtime_error("empty") ); | ||
| CHECK( *opt == 42 ); // expect-warning: bugprone-unchecked-optional-access | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
|
|
||
| // Copyright Catch2 Authors | ||
| // Distributed under the Boost Software License, Version 1.0. | ||
| // (See accompanying file LICENSE.txt or copy at | ||
| // https://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| // SPDX-License-Identifier: BSL-1.0 | ||
|
|
||
| #include <catch2/catch_test_macros.hpp> | ||
|
|
||
| #include <optional> | ||
|
|
||
| // Deliberately not defined, so that the analysis cannot see through it. | ||
| std::optional<int> maybe_value(); | ||
|
|
||
| TEST_CASE( "REQUIRE terminates the test case on failure" ) { | ||
| std::optional<int> opt = maybe_value(); | ||
| REQUIRE( opt.has_value() ); | ||
| CHECK( *opt == 42 ); | ||
| } | ||
|
|
||
| TEST_CASE( "REQUIRE_FALSE terminates the test case on failure" ) { | ||
| std::optional<int> opt = maybe_value(); | ||
| REQUIRE_FALSE( !opt.has_value() ); | ||
| CHECK( *opt == 42 ); | ||
| } | ||
|
|
||
| TEST_CASE( "CHECK does not terminate the test case on failure" ) { | ||
| std::optional<int> opt = maybe_value(); | ||
| CHECK( opt.has_value() ); | ||
| CHECK( *opt == 42 ); // expect-warning: bugprone-unchecked-optional-access | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Otherwise, we introduce a false-negative for this strange case:
I think this case is strange because:
CHECK(vector.at(123) == 42)so Dataflow analysis would not catch it IIRC.throwdirectly 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_FALSEas well, but maybe I am overthinking...