[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tidy

Author: Jan Voung (jvoung)

<details>
<summary>Changes</summary>

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.


---
Full diff: https://github.com/llvm/llvm-project/pull/115051.diff


4 Files Affected:

- (modified) clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp (+18-1) 
- (modified) clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h (+7-1) 
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h (+27) 
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp (+24) 


``````````diff
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..ba865dfc5a966f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,27 @@
+#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_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
+
+#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();
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/115051