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

[Julian Schmidt via cfe-commits]

================
@@ -27,19 +29,44 @@ 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;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
     Options.store(Opts, "IgnoreSmartPointerDereference",
                   ModelOptions.IgnoreSmartPointerDereference);
+    Options.store(Opts, "IgnoreTestTUs", ignore_test_tus_);
   }
 
 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.
+  // 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).
----------------
5chmidti wrote:

Please add a release note, and add the option to the check documentation with an explanation similar to this, but targeted towards the user.

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