[clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access

clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp

@@ -40,11 +40,39 @@ void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
  this);
 }
 
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ IsTestTu = 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 the option ignore_test_tus_ is set.
+ if ((IgnoreTestTus && IsTestTu) ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
  return;
 
+ // 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;
+ }
+
  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
  if (FuncDecl->isTemplated())
  return;

clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h

@@ -10,8 +10,11 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNCHECKEDOPTIONALACCESSCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../ClangTidyOptions.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 {
 
@@ -25,19 +28,43 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
 public:
  UncheckedOptionalAccessCheck(StringRef Name, ClangTidyContext *Context)
  : ClangTidyCheck(Name, Context),
- ModelOptions{Options.get("IgnoreSmartPointerDereference", false)} {}
+ ModelOptions{Options.get("IgnoreSmartPointerDereference", 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;
  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", IgnoreTestTus);
  }
 
 private:
  dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // 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.
+ bool IgnoreTestTus = false;
+
+ // Records whether the current TU includes the test-specific headers (e.g.,
+ // 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

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

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_

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h

@@ -0,0 +1,33 @@
+#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.
+
+// Normally this declares a class, but it isn't relevant for testing.
+#define GTEST_TEST(test_suite_name, test_name)
+
+// 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 (BoolWrapper(condition)) \
+ ; \
+ else \
+ return;
+
+#define ASSERT_FALSE(condition) \
+ if (BoolWrapper(!condition)) \
+ ; \
+ else \
+ return;
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_

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

clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

@@ -0,0 +1,48 @@
+// 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"
+
+// When IgnoreTestTUs is set, all of the warnings in a test TU are suppressed,
+// even the `unchecked_value_access` and `assert_true_incorrect` cases.
+
+// CHECK-MESSAGES-IGNORE-TESTS: 2 warnings generated
+// CHECK-MESSAGES-DEFAULT: 6 warnings generated
+
+// 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
+}
+
+// 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
+}
+
+// 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 2 warnings
+// CHECK-MESSAGES-DEFAULT: Suppressed 2 warnings