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

[Jan Voung via cfe-commits]

jvoung wrote:

> Sorry for the delay.
> 
> I think, that we can probably all agree that the best solution would be to 
> have the model support the macros, e.g., by recognizing the patterns that are 
> behind these macros. However, that is currently not something that people can 
> find time to do (as was communicated by @ymand w.r.t. current priorities), 
> and not something that seems to have caused significant enough irritation for 
> someone to step in themselves.
> 
> As discussed, disabling the check inside the test folder only works if the 
> project structure supports it (which is IMO the majority, but that is just a 
> feeling), but that is also something users have to know they can do. We 
> should open an issue with the help wanted label, and mention explicitly that 
> this is not simple in the description. That way, someone may be inclined to 
> start working on this.
> 

Thanks, that is a good summary =)

(and sorry for not replying earlier about the test folder alternative, but as 
@ymand mentioned, our codebase unfortunately doesn't use test folders)

> I think the GTest matchers don't work here, because you'd want to mark the 
> optional as having a value or not having a value, and you would need access 
> to the environment at that time, which is not available at this stage. But 
> the GTest macros could be used for a crude implementation inside the model, 
> right?

Yes, the analysis would want to mark the optional as having a value or not in 
the environment. So modeling would be a combination of the GTest matchers to 
dispatch to some transfer function, and then the transfer functions will modify 
the environment appropriately.  Similarly for catch2 matchers + transfer 
functions, etc. 

There was also the suggestion from Gabor of adding `analyzer_noreturn` 
attributes. But it would also take some time to modify the appropriate 
libraries and wait for roll out, etc. Or perhaps similarly, if the libraries 
could be modified to provide super-simple versions of the macro behind ifdef 
__clang_analyzer__ (e.g., for googletest, skip the `Converter` ctor + operator 
bool function crossings), but again take time to see if the libraries are 
willing, modify, wait for rollout, etc.

> I'm not really that opposed to this solution, but the other solutions sound 
> better to me, though with their own caveats (time, work, GTest macros are 
> only for GTest). I'm fine with this, if all other options are not feasible to 
> implement in the near to mid term, but a second non-blocking review from 
> another clang-tidy dev would be best.
> 
> Added two more, but there are some other review comments still open, probably 
> because of the discussion.

Ah sorry, yes, I hadn't addressed the earlier review comments since it seemed 
like bigger picture discussion was still open. Update the patch.


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

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

[Jan Voung 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).

jvoung wrote:

Good point -- added a note about the option in release notes (e.g., when 
preferred, use the .clang-tidy instead, if test files are actually separated 
into folders)

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

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

[Jan Voung 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).
+  bool ignore_test_tus_ = false;
+
+  // Records whether the current TU includes the test-specific headers (e.g.,
+  // googletest), in which case we assume it is a test TU of some sort.
+  // This along with the setting `ignore_test_tus_` allows us to disable
+  // checking for all test TUs.
+  bool is_test_tu_ = false;

jvoung wrote:

Oops, done!

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

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

[Jan Voung via cfe-commits]

jvoung wrote:

Done (there was a false-negative example in `unchecked_value_access` but also 
added the ASSERT_FALSE)

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

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

[Jan Voung via cfe-commits]

@@ -40,11 +40,30 @@ 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 the option ignore_test_tus_ is set.
+  if ((ignore_test_tus_ && is_test_tu_) ||
+  Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())

jvoung wrote:

I don't think we'd have access to `Result.Context->Idents` yet, at 
onStartOfTranslationUnit, to compute IsTestTu ?

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

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

[Jan Voung via cfe-commits]

@@ -40,11 +40,30 @@ 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 the option ignore_test_tus_ is set.
+  if ((ignore_test_tus_ && 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()) {

jvoung wrote:

Okay, added some light detection of catch2 macros similar to this

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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/6] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/5] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Julian Schmidt via cfe-commits]

@@ -40,11 +40,30 @@ 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 the option ignore_test_tus_ is set.
+  if ((ignore_test_tus_ && is_test_tu_) ||
+  Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())

5chmidti wrote:

Couldn't this be moved to onStartOfTranslationUnit?

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

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

[Julian Schmidt via cfe-commits]

@@ -40,11 +40,30 @@ 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 the option ignore_test_tus_ is set.
+  if ((ignore_test_tus_ && 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()) {

5chmidti wrote:

While you are adding the gtest macros, could you also add REQUIRE as well? That 
way Catch2 is also detected. (I don't know do test, but there probably is also 
a simple macro you could check for)

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

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti commented:

Sorry for the delay.

I think, that we can probably all agree that the best solution would be to have 
the model support the macros, e.g., by recognizing the patterns that are behind 
these macros. However, that is currently not something that people can find 
time to do (as was communicated by @ymand w.r.t. current priorities), and not 
something that seems to have caused significant enough irritation for someone 
to step in themselves.

As discussed, disabling the check inside the test folder only works if the 
project structure supports it (which is IMO the majority, but that is just a 
feeling), but that is also something users have to know they can do. We should 
open an issue with the help wanted label, and mention explicitly that this is 
not simple in the description. That way, someone may be inclined to start 
working on this.

I think the GTest matchers don't work here, because you'd want to mark the 
optional as having a value or not having a value, and you would need access to 
the environment at that time, which is not available at this stage. But the 
GTest macros could be used for a crude implementation inside the model, right?

I'm not really that opposed to this solution, but the other solutions sound 
better to me, though with their own caveats (time, work, GTest macros are only 
for GTest). I'm fine with this, if all other options are not feasible to 
implement in the near to mid term, but a second non-blocking review from 
another clang-tidy dev would be best.

---

Added two more, but there are some other review comments still open, probably 
because of the discussion.

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

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti edited 
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

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

[Gábor Horváth via cfe-commits]

https://github.com/Xazax-hun approved this pull request.


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

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

[Gábor Horváth via cfe-commits]

Xazax-hun wrote:

I think the ideal solution would be something like using the 
[analyzer_noreturn](https://clang.llvm.org/docs/AttributeReference.html#analyzer-noreturn)
 attribute in GTest to mark branches that should not be taken by static 
analyzers because some invariants failed and every further diagnostics are 
false positive on that path. This is the solution that the Clang Static 
Analyzer recommends to users (adding annotations to custom asserts). That being 
said, there is a precedent of the analyzer recognizing custom assert functions 
and treating them as noreturn even if they are not annotated: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp

If GTest had a similar function for the assert failed paths, a similar approach 
could be taken for the dataflow framework as well. If there is no such function 
that we could match on in the expanded code, I don't think we could do 
anything, we might need to do changes to GTest.

All that being said, I think we want to provide a good experience for people 
using this popular testing library out of the box, even if they use an older 
version that does not have any annotations. I am OK with suppressing noisy 
static analysis results for test files for the time being. It looks like the 
current options are either leave it broken on tests or turn it off for tests as 
modeling non-test code is prioritized. As a users I would definitely prefer the 
latter. 

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

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

[Yitzhak Mandelbaum via cfe-commits]

ymand wrote:

Gentle ping, since it's been a week since my last post.

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

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

[Yitzhak Mandelbaum via cfe-commits]

ymand wrote:

This review has been going on for longer than a month so, as one of the 
reviewers and the original author of this check, I wanted to jump in and see if 
we can bring it to an agreeable conclusion. I’d like to start with some 
background:

* This check was introduced in 2020 with the current limitations of poor 
understanding of tests and was raised as an [issue in May 
2023](https://github.com/llvm/llvm-project/issues/62600). Our codebase does not 
distinguish test folders for the most part, so we relied on regexp matching of 
file names, which never really worked (the patterns match both too much and too 
little).
* This check is built on the Clang Dataflow Framework, which takes primary 
responsibility for modeling code. The check simply wraps the `optional` model 
from that framework. In that project (of which I am a lead and active 
contributor), we have not prioritized modeling test code, because modeling the 
large variety of test macros correctly is quite difficult and we see it as a 
lower value than improving the framework and models for normal code. We would 
love to have better coverage, but given our limited resources, this is our 
prioritization.
* Despite this limitation being present in this checker since 2020, no one in 
the broader community has ever expressed an interest in picking up this work. 
We take that as a (weak) signal of lack of user prioritization for this feature 
as well. For codebases with dedicated test directories, that makes a lot of 
sense.
* We (jvoung and I) cannot commit to the level of work required to fully 
support the variety of googletest matchers — not least ASSERT_THAT, for which 
we've seen significant use with optional values. We certainly can’t commit to 
support beyond googletest, which is a real issue, as 
[5chmidti](https://github.com/5chmidti)'s comment about `catch2` and `REQUIRE` 
highlights.

With all this, we view it as necessary, for codebases without dedicated test 
directories, to offer this feature in the checker. Not as a short-term fix, but 
as a long term feature. As we progress (hopefully) in coverage of test macros, 
the users of this feature will be able to gradually switch off of it — starting 
with those users that only rely on the relatively simple ASSERT_TRUE/FALSE and 
continuing from there.

Given how long this issue has remained open, I hope we can resolve and reach a 
conclusion soon.

Thanks!
Yitzhak

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

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

[Yitzhak Mandelbaum via cfe-commits]

ymand wrote:

> I am still confused why your team did not just disable this rule for test 
> folder? It is normal cases that source code and test code have different 
> quality metrics.

Our codebase doesn't use test folders. We have no foolproof way of 
distinguishing code from test based on the file system.

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

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

[Carlos Galvez via cfe-commits]

carlosgalvezp wrote:

> It is normal cases that source code and test code have different quality 
> metrics.

+1. That's responsibility of the build system. "What is a test" is something 
that can vary from project to project.

>  is expected to be removed again

>From experience, removing flags it's quite hard, due to backwards 
>compatibility reasons (we break other people that rely on them). It usually 
>takes about 2 releases (1 year) to pull it off. 

So I would be strongly against adding such a flag now if we already see that it 
will be removed - we should rather take the time to implement this right from 
the start.

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

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

[Congcong Cai via cfe-commits]

https://github.com/HerrCai0907 requested changes to this pull request.

It would be better to use `GtestCmp` if you really want clang-tidy support 
gtest in this check.
Here is an unfinished example which can match
```int v = 10;
TEST(a, b) {
  std::optional a;
  if (v) {
a = 1;
  }
  ASSERT_NE(a, std::nullopt);
  a.value();
}
```
```c++
  using namespace ast_matchers;
  auto HasOptionalCallDescendant = hasDescendant(callExpr(callee(cxxMethodDecl(
  ofClass(UncheckedOptionalAccessModel::optionalClassDecl());
  Finder->addMatcher(
  decl(anyOf(functionDecl(
 unless(isExpansionInSystemHeader()),
 // FIXME: Remove the filter below when lambdas are
 // well supported by the check.
 unless(hasDeclContext(cxxRecordDecl(isLambda(,
 hasBody(allOf(
 HasOptionalCallDescendant,
 unless(hasDescendant(gtestAssert(
 GtestCmp::Ne,
 expr(hasType(qualType(hasUnqualifiedDesugaredType(
 recordType(hasDeclaration(
 UncheckedOptionalAccessModel::
 optionalClassDecl())),
 anything())),
 cxxConstructorDecl(hasAnyConstructorInitializer(
 withInitializer(HasOptionalCallDescendant)
  .bind(FuncID),
  this);
```

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

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

[Congcong Cai via cfe-commits]

https://github.com/HerrCai0907 commented:

I am still confused why your team did not just disable this rule for test 
folder?
It is normal cases that source code and test code have different quality 
metrics.

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

[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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti edited 
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

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti edited 
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

[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).
+  bool ignore_test_tus_ = false;
+
+  // Records whether the current TU includes the test-specific headers (e.g.,
+  // googletest), in which case we assume it is a test TU of some sort.
+  // This along with the setting `ignore_test_tus_` allows us to disable
+  // checking for all test TUs.
+  bool is_test_tu_ = false;

5chmidti wrote:

nit: var casing should be `CamelCase`

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

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

[Julian Schmidt via cfe-commits]

5chmidti wrote:

Please also add an `ASSERT_FALSE` (or just `ASSERT_TRUE(!opt.has_value())`) 
with a dereference after, to show that there are false-negatives

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

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti commented:

> option that is off by default. For all other users the behavior is unchanged, 
> but we can opt in on our side.
> Can we add the option to use in the meantime, while I work on the macro 
> handling in parallel?

That sounds okay to me, even though the option is expected to be removed again 
because it won't be needed then. Others may have stronger opinions on this, so 
lets see.

A less fine-grained solution to skip these FPs would be to create a 
`.clang-tidy` inside test directories with `InheritParentConfig` enabled, and 
exclude the check there. Though that would mean that, e.g., test utils would be 
skipped that do not contain the macro.

---

While you're at it, maybe add the catch2 and doctest `REQUIRE` macro as well? 
This could of course mean that the option may not be removed later if only the 
GTest macros are handled, which could be complex enough without implementing 
the `REQUIRE` macro as well.

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

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

[Jan Voung via cfe-commits]

https://github.com/jvoung edited 
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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/5] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Jan Voung via cfe-commits]

jvoung wrote:

Thanks! Ok that does that look useful (and I think the our other reviewer 
ymand@ wrote it =) https://reviews.llvm.org/D74840).

- I'll work on using those in parallel
- It will need to be extended a little to handle a few more matches (right now 
it's the binary comparison eq/ne, etc. not unary EXPECT_TRUE that is more 
common for optional)
- Handle enough of the cases EXPECT_TRUE, EXPECT_EQ, EXPECT_NE, EXPECT_THAT...
- Do some analysis on our false positive data to see what else remains. We have 
some data already of other issues besides macros, like state carried over from 
constructors. There may be cases still where we just can't reduce the FP 
(developers making shortcuts not doing the check, assuming that running the 
test will catch the issue)

In the meantime, I updated the patch to make an option that is off by default. 
For all other users the behavior is unchanged, but we can opt in on our side.

Can we add the option to use in the meantime, while I work on the macro 
handling in parallel?

Thanks!


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

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

[via cfe-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 3b29a8a00809e868e3df7e687695670ff5077fbd 
ee10510d15e75da7fc75420fa33d00e97d6477a0 --extensions h,cpp -- 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
 clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp 
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
``





View the diff from clang-format here.


``diff
diff --git 
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h 
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 3e20fd1a1e..ec87e23fa7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -11,10 +11,10 @@
 
 #include "../ClangTidyCheck.h"
 #include "../ClangTidyOptions.h"
-#include "clang/Basic/LLVM.h"
-#include "clang/Basic/LangOptions.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 {
 
@@ -40,8 +40,7 @@ public:
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
 Options.store(Opts, "IgnoreSmartPointerDereference",
   ModelOptions.IgnoreSmartPointerDereference);
-Options.store(Opts, "IgnoreTestTUs",
-  ignore_test_tus_);
+Options.store(Opts, "IgnoreTestTUs", ignore_test_tus_);
   }
 
 private:

``




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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/4] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Jan Voung via cfe-commits]

jvoung wrote:

Hi @5chmidti and @HerrCai0907 any thoughts on the "Would it help if this was 
instead a checker option?"? Given the volume of false positives right now, I 
think skip by default, but can turn on if needed.

I updated the description to include the point of "we are currently seeing many 
false positives in test files, and it becomes harder for developers to spot 
such true positives, given the FP noise to filter through".

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

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

[Jan Voung via cfe-commits]

https://github.com/jvoung edited 
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

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

[Jan Voung via cfe-commits]

https://github.com/jvoung edited 
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

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

[Congcong Cai via cfe-commits]

HerrCai0907 wrote:

Could clang/lib/ASTMatchers/GtestMatchers.cpp help you to have a better 
solution?

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

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

[Jan Voung via cfe-commits]

https://github.com/jvoung edited 
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

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

[Jan Voung via cfe-commits]

jvoung wrote:

> I don't think we can ignore a TU simply because it has the Google Test header 
> included, which this will do. This would ignore real problems, such as

That is true (and we've considered that -- FWIW, ymand and our team have been 
maintaining this checker).
But as is, we are currently seeing many false positives in test files, and it 
becomes harder for developers to spot such true positives, given the FP noise 
to filter through.

Would it help if this was instead a checker option?

> The issue with the Google Test macros seems to be, that the dataflow check 
> does not understand using a helper for the cast to `bool` like this:
> 
> ```c++
> ...
> if (const auto wrapped_check = Converter{v})
> ;
> else
> return;
> auto val = *v;
> }
> ```

Thanks, that's definitely part of it. If we handle `EXPECT_TRUE` as well (and 
other `EXPECT_...` forms), EXPECT [does not](https://godbolt.org/z/6Tbxae9jq) 
have a `return` so would need special handling (otherwise looks like both 
branches go to the `*v`).


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

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

[Congcong Cai via cfe-commits]

https://github.com/HerrCai0907 edited 
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

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

[Congcong Cai via cfe-commits]

https://github.com/HerrCai0907 requested changes to this pull request.

I don't think it is a good way to suppress all warning when we find gtest macro.
1. it will confuse user who really want to check even in test file.
2. the user can disable this rules in test folder or even use nolint the test 
area.

the potential solution is treat assert_true as assert.

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

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

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti requested changes to this pull request.

I don't think we can ignore a TU simply because it has the Google Test header 
included, which this will do.
This would ignore real problems, such as

```c++
#include 
#include 
void issue(const absl::optional &opt) {
  if (!opt.has_value())
  *opt;
}
```

---

The issue with the Google Test macros seems to be, that the dataflow check does 
not understand using a helper for the cast to `bool` like this:

```c++
#include 

struct Converter {
template 
Converter(T&& val) : success{static_cast(std::forward(val))} {}
operator bool() const { return success; }
bool success;
};

void bar(std::optional v) {
switch (0)
case 0:
default:
if (const auto wrapped_check = Converter{v})
;
else
return;
auto val = *v;
}
```

https://godbolt.org/z/7EdsGYhaW

(found the FP by expanding the `ASSERT_TRUE` macro and reducing manually)

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

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

[Carlos Galvez via cfe-commits]

carlosgalvezp wrote:

Apologies for the late response, I don't have much time to perform in-depth 
reviews. Tagging additional reviewers  @5chmidti @HerrCai0907 

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

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

[Jan Voung via cfe-commits]

jvoung wrote:

Any other concerns? I think I've elaborated on why the ignore/exclude 
alternative is not better.

Otherwise, eventually, it would be great to be able to understand the various 
macros and how they could serve as checks for later accesses, but I think this 
is an improvement on the status quo (we've seen lots of such googletest false 
positives).

Thanks!

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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/3] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Jan Voung via cfe-commits]

jvoung wrote:

> > > we're not (fully) understanding the content
> > 
> > 
> > My thinking was that we don't even need to understand the content, we 
> > simply exclude code that is contained within any of the problematic public 
> > macros. This sounds like it should be possible to do? Unfortunately I don't 
> > know the details on how this could be implemented, hopefully other 
> > reviewers know better?
> > Otherwise ChatGPT seems to give useful ideas on how to skip a matched 
> > result contained within an `ASSERT` macro (obviously untested):
> > ```
> >   if (Lexer::getImmediateMacroName(Loc, SM, Result.Context->getLangOpts()) 
> > == "ASSERT") {
> > // The call is within ASSERT, no diagnostic needed.
> > return;
> >   }
> > ```
> 
> That doesn't handle some cases like:
> 
> ```
> auto opt = DoSomeSetup(...)
> ASSERT_TRUE(opt.has_value())
> T x = DoMoreSetup(*opt)  // warn right here, since we didn't interpret the 
> above ASSERT_TRUE (or other ways to check)
> 
> EXPECT_EQ(FunctionToTest(x), ...);
> ```
> 
> Sometimes the `*opt` may be within a macro, but not always.

- In non-test code, it is even more likely that the `*opt` is not wrapped in a 
macro, while the `ASSERT(opt.has_value())` is.
- If in non-test scenarios, the `ASSERT` macro implementation is actually 
simple, and the analysis already understood `ASSERT(opt.has_value())` makes a 
following `*opt` safe, then ignoring the `ASSERT` is actually worse and causes 
false positives.
- We wouldn't want to accidentally ignore the wrong macros (especially in 
non-test contexts)
- Coming up with a list of exactly the right macros to ignore for gtest, would 
be a bigger list of public macro names than the current change.
- And it still doesn't solve the "sometimes the `*opt` may be within a macro, 
but not always."

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

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

[Jan Voung via cfe-commits]

jvoung wrote:

> > but these are not internal implementation details - these are key elements 
> > of the public API
> 
> In the unit test, you have copied internal code from here: 
> https://github.com/google/googletest/blob/d144031940543e15423a25ae5a8a74141044862f/googletest/include/gtest/internal/gtest-internal.h#L1477
> 
> This is not OK to do, since it may change at any time and make the test no 
> longer useful. The test may also work for some versions of googletest but not 
> others.
> 
> The code that has been copied is also Copyrighted and has certain License 
> obligations, which we may not be allowed to bring into the LLVM repository. 
> Pinging @tstellar for clarification.

Simplified the mock, since we're only matching on the top-level macro names.

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

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

[Jan Voung via cfe-commits]

jvoung wrote:

> > we're not (fully) understanding the content
> 
> My thinking was that we don't even need to understand the content, we simply 
> exclude code that is contained within any of the problematic public macros. 
> This sounds like it should be possible to do? Unfortunately I don't know the 
> details on how this could be implemented, hopefully other reviewers know 
> better?
> 
> Otherwise ChatGPT seems to give useful ideas on how to skip a matched result 
> contained within an `ASSERT` macro (obviously untested):
> 
> ```
>   if (Lexer::getImmediateMacroName(Loc, SM, Result.Context->getLangOpts()) == 
> "ASSERT") {
> // The call is within ASSERT, no diagnostic needed.
> return;
>   }
> ```

That doesn't handle some cases like:

```
auto opt = DoSomeSetup(...)
ASSERT_TRUE(opt.has_value())
T x = DoMoreSetup(*opt)  // warn right here, since we didn't interpret the 
above ASSERT_TRUE (or other ways to check)

EXPECT_EQ(FunctionToTest(x), ...);
```

Sometimes the `*opt` may be within a macro, but not always.

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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/3] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Carlos Galvez via cfe-commits]

carlosgalvezp wrote:

> we're not (fully) understanding the content

My thinking was that we don't even need to understand the content, we simply 
exclude code that is contained within any of the problematic public macros. 
This sounds like it should be possible to do? Unfortunately I don't know the 
details on how this could be implemented, hopefully other reviewers know better?

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

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

[Carlos Galvez via cfe-commits]

carlosgalvezp wrote:

> but these are not internal implementation details - these are key elements of 
> the public API 

In the unit test, you have copied internal code from here:
https://github.com/google/googletest/blob/d144031940543e15423a25ae5a8a74141044862f/googletest/include/gtest/internal/gtest-internal.h#L1477

This is not OK to do, since it may change at any time and make the test no 
longer useful. The test may also work for some versions of googletest but not 
others.

The code that has been copied is also Copyrighted and has certain License 
obligations, which we may not be allowed to bring into the LLVM repository.

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

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

[Jan Voung via cfe-commits]

jvoung wrote:

> Relying on Google-test internal implementation details that are outside of 
> our control and may change at any point in time does not feel good.
> 
> The patch should instead fix the root cause of the problem.

I agree that it's not good to rely on implementation details, but these are not 
internal implementation details - these are key elements of the *public API* , 
which trip up the analysis. It's not just any macros that we have a problem 
with, it's specifically the googletest macros, which we don't properly 
understand, resulting in false positives.


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

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

[Yitzhak Mandelbaum via cfe-commits]

https://github.com/ymand approved this pull request.


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

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

[Jan Voung via cfe-commits]

jvoung wrote:

> A quick middle-point solution is to add an option to allow ignoring code that 
> is executed from within macros, or even allow the user to specify which 
> macros to ignore.

Unfortunately, the problem is exactly that we're not (fully) understanding the 
content of `ASSERT_TRUE` and related macros (`ASSERT_FALSE`, `ASSERT_THAT(..., 
IsTrue(opt.has_value()))`, etc.), which then affect code outside and following 
the macros. So, ignoring them would not solve anything. Until we can understand 
the googletest macros, there is little value in check any tests that use them. 
That is what this patch accomplishes.


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

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

[Carlos Galvez via cfe-commits]

carlosgalvezp wrote:

A quick middle-point solution is to add an option to allow ignoring code that 
is executed from within macros, or even 
allow the user to specify which macros to ignore.

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

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

[Carlos Galvez via cfe-commits]

https://github.com/carlosgalvezp requested changes to this pull request.

Relying on Google-test internal implementation details that are outside of our 
control and may change at any point in time does not feel good. 

The patch should instead fix the root cause of the problem.

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

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

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang-tidy

Author: Jan Voung (jvoung)


Changes

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(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 
 
 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 00..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_BL

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

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang-tools-extra

Author: Jan Voung (jvoung)


Changes

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(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 
 
 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 00..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_

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

[Jan Voung via cfe-commits]

https://github.com/jvoung ready_for_review 
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

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

[Jan Voung via cfe-commits]

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

>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/2] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {   
\
+  public:  

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

[Jan Voung via cfe-commits]

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

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.


>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung 
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH] [clang-tidy] Filter out googletest TUs in
 bugprone-unchecked-optional-access

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.
---
 .../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
 .../bugprone/UncheckedOptionalAccessCheck.h   |  8 ++-
 .../unchecked-optional-access/gtest/gtest.h   | 24 +++
 ...cked-optional-access-ignore-googletest.cpp | 24 +++
 4 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
 create mode 100644 
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp

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(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 
 
 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 00..8a9eeac94dbb77
--- /dev/null
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#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(test_suite_name, test_name) 
\
+  class