https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Tue, 5 Nov 2024 19:20:36 +0000
Subject: [PATCH 1/6] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++++++++++++++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++++++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++++++++++++++++++
...cked-optional-access-ignore-googletest.cpp | 24 +++++++++++++++++++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+ is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include <optional>
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00000000000000..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
\
+ GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default;
\
+ };
+
+#define GTEST_AMBIGUOUS_ELSE_BLOCKER_
\
+ switch (0)
\
+ case 0:
\
+ default: // NOLINT
+
+#define ASSERT_TRUE(condition)
\
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_
\
+ if (condition)
\
+ ;
\
+ else
\
+ return; // should fail...
+
+#endif //
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
new file mode 100644
index 00000000000000..2f213434d0ad61
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I
%S/Inputs/unchecked-optional-access
+
+#include "absl/types/optional.h"
+#include "gtest/gtest.h"
+
+// All of the warnings are suppressed once we detect that we are in a test TU
+// even the unchecked_value_access case.
+
+// CHECK-MESSAGE: 1 warning generated
+// CHECK-MESSAGES: Suppressed 1 warnings
+
+void unchecked_value_access(const absl::optional<int> &opt) {
+ opt.value();
+}
+
+void assert_true_check_operator_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ *opt;
+}
+
+void assert_true_check_value_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ opt.value();
+}
>From 3427fa73c174ccdc68b703c20c0ab40a7418e955 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Tue, 5 Nov 2024 19:29:28 +0000
Subject: [PATCH 2/6] Add one more missing macro
---
.../bugprone/Inputs/unchecked-optional-access/gtest/gtest.h | 3 +++
1 file changed, 3 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
index 8a9eeac94dbb77..ba865dfc5a966f 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -3,6 +3,9 @@
// Mock version of googletest macros.
+#define GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)
\
+ test_suite_name##_##test_name##_Test
+
#define GTEST_TEST(test_suite_name, test_name)
\
class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
public:
\
>From 2611af1a9a7ecb007d0f0f735d0b7b1581a0f1b1 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Wed, 6 Nov 2024 22:10:28 +0000
Subject: [PATCH 3/6] Simplify the mock a bit more
The implementation doesn't matter as much for testing, since the
currently skipping is keying on some top-level macros.
---
.../unchecked-optional-access/gtest/gtest.h | 21 ++++++-------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
index ba865dfc5a966f..e3b545c01d2fa1 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -3,25 +3,16 @@
// Mock version of googletest macros.
-#define GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)
\
- test_suite_name##_##test_name##_Test
-
-#define GTEST_TEST(test_suite_name, test_name)
\
- class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
- public:
\
- GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default;
\
- };
-
-#define GTEST_AMBIGUOUS_ELSE_BLOCKER_
\
- switch (0)
\
- case 0:
\
- default: // NOLINT
+// Normally this declares a class, but it isn't relevant for testing.
+#define GTEST_TEST(test_suite_name, test_name)
+// Normally, this has a relatively complex implementation
+// (wrapping the condition evaluation), more complex failure behavior, etc.,
+// but we keep it simple for testing.
#define ASSERT_TRUE(condition)
\
- GTEST_AMBIGUOUS_ELSE_BLOCKER_
\
if (condition)
\
;
\
else
\
- return; // should fail...
+ return; // normally "fails" rather than just return
#endif //
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
>From ee10510d15e75da7fc75420fa33d00e97d6477a0 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Fri, 6 Dec 2024 19:29:51 +0000
Subject: [PATCH 4/6] Make an option, default off
Also fix a CHECK-MESSAGE -> CHECK-MESSAGES
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 8 +++--
.../bugprone/UncheckedOptionalAccessCheck.h | 32 ++++++++++++++++---
...cked-optional-access-ignore-googletest.cpp | 20 +++++++++---
3 files changed, 48 insertions(+), 12 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 06deea0a446809..71de2fda005bd7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -48,8 +48,9 @@ void UncheckedOptionalAccessCheck::onStartOfTranslationUnit()
{
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
// The googletest assertion macros are not currently recognized, so we have
- // many false positives in tests. So, do not check functions in a test TU.
- if (is_test_tu_ ||
+ // many false positives in tests. So, do not check functions in a test TU
+ // if the option ignore_test_tus_ is set.
+ if ((ignore_test_tus_ && is_test_tu_) ||
Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
return;
@@ -59,7 +60,8 @@ void UncheckedOptionalAccessCheck::check(
if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
is_test_tu_ = true;
- return;
+ if (ignore_test_tus_)
+ return;
}
const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 3bd7ff9867b145..3e20fd1a1ed4eb 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -10,6 +10,9 @@
#define
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNCHECKEDOPTIONALACCESSCHECK_H
#include "../ClangTidyCheck.h"
+#include "../ClangTidyOptions.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
@@ -26,7 +29,8 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
UncheckedOptionalAccessCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
ModelOptions{
- Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
+ Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)},
+ ignore_test_tus_(Options.getLocalOrGlobal("IgnoreTestTUs", false)) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void onStartOfTranslationUnit() override;
@@ -36,15 +40,33 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
Options.store(Opts, "IgnoreSmartPointerDereference",
ModelOptions.IgnoreSmartPointerDereference);
+ Options.store(Opts, "IgnoreTestTUs",
+ ignore_test_tus_);
}
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
- // Records whether the current TU includes the googletest headers, in which
- // case we assume it is a test TU of some sort. The googletest assertion
- // macros are not currently recognized, so we have many false positives in
- // tests. So, we disable checking for all test TUs.
+ // Tracks the Option of whether we should ignore test TUs (e.g., googletest).
+ // Currently we have many false positives in tests, making it difficult to
+ // find true positives and developers end up ignoring the warnings in tests,
+ // reducing the check's effectiveness.
+ // Reasons for false positives (once fixed we could remove this option):
+ // - has_value() checks wrapped in googletest assertion macros are not
handled
+ // (e.g., EXPECT_TRUE() and fall through, or ASSERT_TRUE() and crash,
+ // or more complex ones like ASSERT_THAT(x, Not(Eq(std::nullopt))))
+ // - we don't handle state carried over from test fixture constructors/setup
+ // to test case bodies (constructor may initialize an optional to a value)
+ // - developers may make shortcuts in tests making assumptions and
+ // use the test runs (expecially with sanitizers) to check assumptions.
+ // This is different from production code in that test code should have
+ // near 100% coverage (if not covered by the test itself, it is dead code).
+ bool ignore_test_tus_ = false;
+
+ // Records whether the current TU includes the test-specific headers (e.g.,
+ // googletest), in which case we assume it is a test TU of some sort.
+ // This along with the setting `ignore_test_tus_` allows us to disable
+ // checking for all test TUs.
bool is_test_tu_ = false;
};
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
index 2f213434d0ad61..3effafc041daa4 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
@@ -1,16 +1,25 @@
-// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I
%S/Inputs/unchecked-optional-access
+// RUN: %check_clang_tidy -check-suffix=IGNORE-TESTS %s \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: {bugprone-unchecked-optional-access.IgnoreTestTUs: true}}" \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
+
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
#include "absl/types/optional.h"
#include "gtest/gtest.h"
-// All of the warnings are suppressed once we detect that we are in a test TU
+// When IgnoreTestTUs is set, all of the warnings in a test TU are suppressed,
// even the unchecked_value_access case.
-// CHECK-MESSAGE: 1 warning generated
-// CHECK-MESSAGES: Suppressed 1 warnings
+// CHECK-MESSAGES-IGNORE-TESTS: 1 warning generated
+// CHECK-MESSAGES-DEFAULT: 2 warnings generated
void unchecked_value_access(const absl::optional<int> &opt) {
opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
}
void assert_true_check_operator_access(const absl::optional<int> &opt) {
@@ -22,3 +31,6 @@ void assert_true_check_value_access(const absl::optional<int>
&opt) {
ASSERT_TRUE(opt.has_value());
opt.value();
}
+
+// CHECK-MESSAGES-IGNORE-TESTS: Suppressed 1 warnings
+// CHECK-MESSAGES-DEFAULT: Suppressed 1 warnings
>From d8b311d053cdbccca43420533eeb0570bf62a8d9 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Fri, 6 Dec 2024 19:43:23 +0000
Subject: [PATCH 5/6] fix clang-format
---
.../clang-tidy/bugprone/UncheckedOptionalAccessCheck.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 3e20fd1a1ed4eb..ec87e23fa7d83d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -11,10 +11,10 @@
#include "../ClangTidyCheck.h"
#include "../ClangTidyOptions.h"
-#include "clang/Basic/LLVM.h"
-#include "clang/Basic/LangOptions.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/LangOptions.h"
namespace clang::tidy::bugprone {
@@ -40,8 +40,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
Options.store(Opts, "IgnoreSmartPointerDereference",
ModelOptions.IgnoreSmartPointerDereference);
- Options.store(Opts, "IgnoreTestTUs",
- ignore_test_tus_);
+ Options.store(Opts, "IgnoreTestTUs", ignore_test_tus_);
}
private:
>From ed8e56a59deacfc235f66a582b528298c1ac256b Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Wed, 8 Jan 2025 20:49:44 +0000
Subject: [PATCH 6/6] Address review comments
- lightweight detection of catch2
- style etc.
- add release note about option
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 27 ++++--
.../bugprone/UncheckedOptionalAccessCheck.h | 26 +++---
clang-tools-extra/docs/ReleaseNotes.rst | 7 ++
.../catch2/catch_test_macros.hpp | 32 +++++++
.../unchecked-optional-access/gtest/gtest.h | 27 ++++--
...ecked-optional-access-ignore-catchtest.cpp | 85 +++++++++++++++++++
...cked-optional-access-ignore-googletest.cpp | 30 +++++--
7 files changed, 196 insertions(+), 38 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/catch2/catch_test_macros.hpp
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-catchtest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 71de2fda005bd7..eee26604c1d8fd 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -42,7 +42,7 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
// Reset the flag for each TU.
- is_test_tu_ = false;
+ IsTestTu = false;
}
void UncheckedOptionalAccessCheck::check(
@@ -50,17 +50,26 @@ void UncheckedOptionalAccessCheck::check(
// The googletest assertion macros are not currently recognized, so we have
// many false positives in tests. So, do not check functions in a test TU
// if the option ignore_test_tus_ is set.
- if ((ignore_test_tus_ && is_test_tu_) ||
+ if ((IgnoreTestTus && IsTestTu) ||
Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
return;
- // Look for two (public) googletest macros; if found, we'll mark this TU as a
- // test TU. We look for ASSERT_TRUE because it is a problematic macro that
- // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
- if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
- Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
- is_test_tu_ = true;
- if (ignore_test_tus_)
+ // Look for some public test library macros; if found, we'll mark this TU as
a
+ // test TU. We look for two macros from each library to help disambiguate
+ // (otherwise "ASSERT_TRUE" or "REQUIRE" could be macros for non-test code).
+ IdentifierTable &Idents = Result.Context->Idents;
+ if (
+ // googletest
+ (Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Idents.get("GTEST_TEST").hasMacroDefinition()) ||
+ // catch2 w/out prefix
+ (Idents.get("REQUIRE_FALSE").hasMacroDefinition() &&
+ Idents.get("METHOD_AS_TEST_CASE").hasMacroDefinition()) ||
+ // catch2 w/ prefix
+ (Idents.get("CATCH_REQUIRE_FALSE").hasMacroDefinition() &&
+ Idents.get("CATCH_METHOD_AS_TEST_CASE").hasMacroDefinition())) {
+ IsTestTu = true;
+ if (IgnoreTestTus)
return;
}
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 03640acb570b57..b9817dc35b07fe 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -29,8 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
UncheckedOptionalAccessCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
ModelOptions{Options.get("IgnoreSmartPointerDereference", false)},
- ignore_test_tus_(Options.get("IgnoreTestTUs", false)) {}
-
+ IgnoreTestTus(Options.get("IgnoreTestTUs", false)) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void onStartOfTranslationUnit() override;
@@ -40,33 +39,32 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
Options.store(Opts, "IgnoreSmartPointerDereference",
ModelOptions.IgnoreSmartPointerDereference);
- Options.store(Opts, "IgnoreTestTUs", ignore_test_tus_);
+ Options.store(Opts, "IgnoreTestTUs", IgnoreTestTus);
}
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
- // Tracks the Option of whether we should ignore test TUs (e.g., googletest).
- // Currently we have many false positives in tests, making it difficult to
- // find true positives and developers end up ignoring the warnings in tests,
- // reducing the check's effectiveness.
+ // Tracks the Option of whether we should ignore test TUs (e.g., googletest,
+ // catch2). Currently we have many false positives in tests, making it
+ // difficult to find true positives and developers end up ignoring the
+ // warnings in tests, reducing the check's effectiveness.
// Reasons for false positives (once fixed we could remove this option):
// - has_value() checks wrapped in googletest assertion macros are not
handled
// (e.g., EXPECT_TRUE() and fall through, or ASSERT_TRUE() and crash,
// or more complex ones like ASSERT_THAT(x, Not(Eq(std::nullopt))))
+ // Catch2's REQUIRE, CHECK, etc. General macro issue:
+ // https://github.com/llvm/llvm-project/issues/62600
// - we don't handle state carried over from test fixture constructors/setup
// to test case bodies (constructor may initialize an optional to a value)
// - developers may make shortcuts in tests making assumptions and
// use the test runs (expecially with sanitizers) to check assumptions.
- // This is different from production code in that test code should have
- // near 100% coverage (if not covered by the test itself, it is dead code).
- bool ignore_test_tus_ = false;
+ bool IgnoreTestTus = false;
// Records whether the current TU includes the test-specific headers (e.g.,
- // googletest), in which case we assume it is a test TU of some sort.
- // This along with the setting `ignore_test_tus_` allows us to disable
- // checking for all test TUs.
- bool is_test_tu_ = false;
+ // googletest, catch2), in which case we assume it is a test TU.
+ // This along with `IgnoreTestTus` allows us to disable checking in test TUs.
+ bool IsTestTu = false;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 94e15639c4a92e..755d8ef1711503 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -232,6 +232,13 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
`bsl::optional` and `bdlb::NullableValue` from
<https://github.com/bloomberg/bde>_.
+ Added option `IgnoreTestTUs` to suppress all warnings in Googletest and
Catch2
+ translation units. This is useful (a) if projects don't have separate folders
+ for test code (otherwise, one can just add a .clang-tidy config in test
+ folders to exclude the check) (b) until false positives due to the analysis
+ seeing values of checks propagate through a variety of complex macros and
+ wrapper functions is fixed (general problem
+ https://github.com/llvm/llvm-project/issues/62600).
- Improved :doc:`bugprone-unhandled-self-assignment
<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by fixing smart
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/catch2/catch_test_macros.hpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/catch2/catch_test_macros.hpp
new file mode 100644
index 00000000000000..1c15bd82668510
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/catch2/catch_test_macros.hpp
@@ -0,0 +1,32 @@
+#ifndef
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_CATCH2_CATCH_TEST_MACROS_HPP_
+#define
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_CATCH2_CATCH_TEST_MACROS_HPP_
+
+// Mock version of catch2 test framework macros.
+
+// The macros are normally much more complex. For
+// - REQUIRE and REQUIRE_FALSE: define them to go through some wrapper
+// (that is not at all what catch2 actually does)
+// - TEST_CASE and METHOD_AS_TEST_CASE: define a function
+void Wrapper(bool cond) {
+ if (!cond)
+ __builtin_trap();
+}
+
+#define CONCAT_HELP(X,Y) X##Y // helper macro
+#define CONCAT(X,Y) CONCAT_HELP(X,Y)
+
+// Catch2 can be configured to have a prefix "CATCH" for macro names, or not,
+// so we model this.
+#ifdef CATCH_CONFIG_PREFIX_ALL
+#define CATCH_REQUIRE( ... ) Wrapper(static_cast<bool>(__VA_ARGS__))
+#define CATCH_REQUIRE_FALSE( ... ) Wrapper(static_cast<bool>(__VA_ARGS__))
+#define CATCH_TEST_CASE( ... ) void CONCAT(TEST, __LINE__)()
+#define CATCH_METHOD_AS_TEST_CASE( ... ) void CONCAT(TEST, __LINE__)()
+#else
+#define REQUIRE( ... ) Wrapper(static_cast<bool>(__VA_ARGS__))
+#define REQUIRE_FALSE( ... ) Wrapper(static_cast<bool>(__VA_ARGS__))
+#define TEST_CASE( ... ) void CONCAT(TEST, __LINE__)()
+#define METHOD_AS_TEST_CASE( ... ) void CONCAT(TEST, __LINE__)()
+#endif
+
+#endif //
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_CATCH2_CATCH_TEST_MACROS_HPP_
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
index e3b545c01d2fa1..3e17737bedfe34 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -6,13 +6,28 @@
// Normally this declares a class, but it isn't relevant for testing.
#define GTEST_TEST(test_suite_name, test_name)
-// Normally, this has a relatively complex implementation
-// (wrapping the condition evaluation), more complex failure behavior, etc.,
-// but we keep it simple for testing.
+// Normally googletest creates a class wrapping the condition, which the
+// dataflow analysis framework has difficulty "seeing through"
+// (needs some inter-procedural analysis, or special-case handling).
+// Here is a simplified version.
+class BoolWrapper {
+public:
+ template <typename T>
+ BoolWrapper(T &&val) : success_(static_cast<bool>(val)) {}
+ operator bool() const { return success_; }
+ bool success_;
+};
+
#define ASSERT_TRUE(condition)
\
- if (condition)
\
+ if (BoolWrapper(condition))
\
+ ;
\
+ else
\
+ return;
+
+#define ASSERT_FALSE(condition)
\
+ if (BoolWrapper(!condition))
\
;
\
else
\
- return; // normally "fails" rather than just return
+ return;
-#endif //
LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-catchtest.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-catchtest.cpp
new file mode 100644
index 00000000000000..fbf3b20d66f73b
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-catchtest.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy -check-suffix=IGNORE-TESTS %s \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: {bugprone-unchecked-optional-access.IgnoreTestTUs: true}}" \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
+
+// RUN: %check_clang_tidy -check-suffix=IGNORE-TESTS-CATCH-PREFIX %s \
+// RUN: --extra-arg=-DCATCH_CONFIG_PREFIX_ALL \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: {bugprone-unchecked-optional-access.IgnoreTestTUs: true}}" \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
+
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
+
+// RUN: %check_clang_tidy -check-suffix=DEFAULT-CATCH-PREFIX %s \
+// RUN: --extra-arg=-DCATCH_CONFIG_PREFIX_ALL \
+// RUN: bugprone-unchecked-optional-access %t -- \
+// RUN: -- -I %S/Inputs/unchecked-optional-access
+
+#include "absl/types/optional.h"
+#include "catch2/catch_test_macros.hpp"
+
+// When IgnoreTestTUs is set, all of the warnings in a test TU are suppressed,
+// even the "Unguarded access" and "Guarded incorrectly" cases.
+
+// CHECK-MESSAGES-IGNORE-TESTS: 2 warnings generated
+// CHECK-MESSAGES-IGNORE-TESTS-CATCH-PREFIX: 1 warning generated
+// CHECK-MESSAGES-DEFAULT: 6 warnings generated
+// CHECK-MESSAGES-DEFAULT-CATCH-PREFIX: 3 warnings generated
+
+absl::optional<int> FunctionUnderTest();
+
+#ifndef CATCH_CONFIG_PREFIX_ALL
+
+TEST_CASE("FN -- Unguarded access", "[optional]") {
+ auto opt = FunctionUnderTest();
+ opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
+}
+
+TEST_CASE("FN -- Guarded incorrectly assert false has_value()", "[optional]") {
+ auto opt = FunctionUnderTest();
+ REQUIRE_FALSE(!opt.has_value());
+ opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
+}
+
+TEST_CASE("FP -- Guarded assert true has_value()", "[optional]") {
+ auto opt = FunctionUnderTest();
+ REQUIRE(opt.has_value());
+ *opt;
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:4: warning: unchecked access to
optional value
+}
+
+TEST_CASE("FP -- Guarded assert false not operator bool()", "[optional]") {
+ auto opt = FunctionUnderTest();
+ REQUIRE_FALSE(!opt);
+ opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
+}
+
+#else
+
+CATCH_TEST_CASE("FN -- Unguarded access", "[optional]") {
+ auto opt = FunctionUnderTest();
+ opt.value();
+ // CHECK-MESSAGES-DEFAULT-CATCH-PREFIX: :[[@LINE-1]]:3: warning: unchecked
access to optional value
+}
+
+CATCH_TEST_CASE("FP -- Guarded assert true has_value()", "[optional]") {
+ auto opt = FunctionUnderTest();
+ CATCH_REQUIRE(opt.has_value());
+ *opt;
+ // CHECK-MESSAGES-DEFAULT-CATCH-PREFIX: :[[@LINE-1]]:4: warning: unchecked
access to optional value
+}
+
+#endif
+
+// CHECK-MESSAGES-IGNORE-TESTS: Suppressed 2 warnings
+// CHECK-MESSAGES-IGNORE-TESTS-CATCH-PREFIX: Suppressed 1 warning
+// CHECK-MESSAGES-DEFAULT: Suppressed 2 warnings
+// CHECK-MESSAGES-DEFAULT-CATCH-PREFIX: Suppressed 1 warning
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
index 3effafc041daa4..0e74bf9aed1779 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
@@ -12,25 +12,37 @@
#include "gtest/gtest.h"
// When IgnoreTestTUs is set, all of the warnings in a test TU are suppressed,
-// even the unchecked_value_access case.
+// even the `unchecked_value_access` and `assert_true_incorrect` cases.
-// CHECK-MESSAGES-IGNORE-TESTS: 1 warning generated
-// CHECK-MESSAGES-DEFAULT: 2 warnings generated
+// CHECK-MESSAGES-IGNORE-TESTS: 2 warnings generated
+// CHECK-MESSAGES-DEFAULT: 6 warnings generated
-void unchecked_value_access(const absl::optional<int> &opt) {
+// False negative from suppressing in test TU.
+void unchecked_value_access(const absl::optional<int> opt) {
opt.value();
// CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
}
-void assert_true_check_operator_access(const absl::optional<int> &opt) {
+// False negative from suppressing in test TU.
+void assert_true_incorrect(const absl::optional<int> opt) {
+ ASSERT_TRUE(!opt.has_value());
+ opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
+}
+
+// False positive, unless we suppress in test TU.
+void assert_true_check_has_value(const absl::optional<int> opt) {
ASSERT_TRUE(opt.has_value());
*opt;
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:4: warning: unchecked access to
optional value
}
-void assert_true_check_value_access(const absl::optional<int> &opt) {
- ASSERT_TRUE(opt.has_value());
+// False positive, unless we suppress (one of many other ways to check)
+void assert_true_check_operator_bool_not_false(const absl::optional<int> opt) {
+ ASSERT_FALSE(!opt);
opt.value();
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: unchecked access to
optional value
}
-// CHECK-MESSAGES-IGNORE-TESTS: Suppressed 1 warnings
-// CHECK-MESSAGES-DEFAULT: Suppressed 1 warnings
+// CHECK-MESSAGES-IGNORE-TESTS: Suppressed 2 warnings
+// CHECK-MESSAGES-DEFAULT: Suppressed 2 warnings
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
