[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 11 19:13:20 PST 2024
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 <jvoung at gmail.com>
Date: Tue, 5 Nov 2024 19:20:36 +0000
Subject: [PATCH 1/3] [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 <jvoung at gmail.com>
Date: Tue, 5 Nov 2024 19:29:28 +0000
Subject: [PATCH 2/3] 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 <jvoung at gmail.com>
Date: Wed, 6 Nov 2024 22:10:28 +0000
Subject: [PATCH 3/3] 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_
More information about the cfe-commits
mailing list