llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
