diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp index 0a0e212f345ed..eee26604c1d8f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp +++ b/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(FuncID); if (FuncDecl->isTemplated()) return; diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h index e2fcccbfefb26..b9817dc35b07f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h +++ b/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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 94e15639c4a92..755d8ef171150 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -232,6 +232,13 @@ Changes in existing checks ` to support `bsl::optional` and `bdlb::NullableValue` from _. + 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 ` 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 0000000000000..1c15bd8266851 --- /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(__VA_ARGS__)) +#define CATCH_REQUIRE_FALSE( ... ) Wrapper(static_cast(__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(__VA_ARGS__)) +#define REQUIRE_FALSE( ... ) Wrapper(static_cast(__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 new file mode 100644 index 0000000000000..3e17737bedfe3 --- /dev/null +++ b/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 + BoolWrapper(T &&val) : success_(static_cast(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_ 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 0000000000000..fbf3b20d66f73 --- /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 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 new file mode 100644 index 0000000000000..0e74bf9aed177 --- /dev/null +++ b/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 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 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 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 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