[clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access #115051
Conversation
…-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.
|
@llvm/pr-subscribers-clang-tidy Author: Jan Voung (jvoung) ChangesWe currently do not handle ASSERT_TRUE(opt.has_value()) macros from Full diff: https://github.com/llvm/llvm-project/pull/115051.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+ is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
if (FuncDecl->isTemplated())
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include <optional>
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)} {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00000000000000..ba865dfc5a966f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,27 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) \
+ test_suite_name##_##test_name##_Test
+
+#define GTEST_TEST(test_suite_name, test_name) \
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) { \
+ public: \
+ GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default; \
+ };
+
+#define GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ switch (0) \
+ case 0: \
+ default: // NOLINT
+
+#define ASSERT_TRUE(condition) \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (condition) \
+ ; \
+ else \
+ return; // should fail...
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
new file mode 100644
index 00000000000000..2f213434d0ad61
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I %S/Inputs/unchecked-optional-access
+
+#include "absl/types/optional.h"
+#include "gtest/gtest.h"
+
+// All of the warnings are suppressed once we detect that we are in a test TU
+// even the unchecked_value_access case.
+
+// CHECK-MESSAGE: 1 warning generated
+// CHECK-MESSAGES: Suppressed 1 warnings
+
+void unchecked_value_access(const absl::optional<int> &opt) {
+ opt.value();
+}
+
+void assert_true_check_operator_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ *opt;
+}
+
+void assert_true_check_value_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ opt.value();
+}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Jan Voung (jvoung) ChangesWe currently do not handle ASSERT_TRUE(opt.has_value()) macros from Full diff: https://github.com/llvm/llvm-project/pull/115051.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+ is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
if (FuncDecl->isTemplated())
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include <optional>
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)} {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00000000000000..ba865dfc5a966f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,27 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) \
+ test_suite_name##_##test_name##_Test
+
+#define GTEST_TEST(test_suite_name, test_name) \
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) { \
+ public: \
+ GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default; \
+ };
+
+#define GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ switch (0) \
+ case 0: \
+ default: // NOLINT
+
+#define ASSERT_TRUE(condition) \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (condition) \
+ ; \
+ else \
+ return; // should fail...
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
new file mode 100644
index 00000000000000..2f213434d0ad61
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I %S/Inputs/unchecked-optional-access
+
+#include "absl/types/optional.h"
+#include "gtest/gtest.h"
+
+// All of the warnings are suppressed once we detect that we are in a test TU
+// even the unchecked_value_access case.
+
+// CHECK-MESSAGE: 1 warning generated
+// CHECK-MESSAGES: Suppressed 1 warnings
+
+void unchecked_value_access(const absl::optional<int> &opt) {
+ opt.value();
+}
+
+void assert_true_check_operator_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ *opt;
+}
+
+void assert_true_check_value_access(const absl::optional<int> &opt) {
+ ASSERT_TRUE(opt.has_value());
+ opt.value();
+}
|
|
A quick middle-point solution is to add an option to allow ignoring code that is executed from within macros, or even |
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. |
Unfortunately, the problem is exactly that we're not (fully) understanding the content of |
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
In the unit test, you have copied internal code from here: 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. |
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 |
The implementation doesn't matter as much for testing, since the currently skipping is keying on some top-level macros.
That doesn't handle some cases like: Sometimes the |
Simplified the mock, since we're only matching on the top-level macro names. |
|
|
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! |
|
Apologies for the late response, I don't have much time to perform in-depth reviews. Tagging additional reviewers @5chmidti @HerrCai0907 |
There was a problem hiding this comment.
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
#include <gtest/gtest.h>
#include <optional>
void issue(const absl::optional<int> &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:
#include <optional>
struct Converter {
template <typename T>
Converter(T&& val) : success{static_cast<bool>(std::forward<T>(val))} {}
operator bool() const { return success; }
bool success;
};
void bar(std::optional<int> 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)
There was a problem hiding this comment.
I don't think it is a good way to suppress all warning when we find gtest macro.
- it will confuse user who really want to check even in test file.
- 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 in dataflow analysis
That is true (and we've considered that -- FWIW, ymand and our team have been maintaining this checker). Would it help if this was instead a checker option?
I don't think nolint is a viable option, given the volume of false positives. It would just add lots of noise to the source code.
Thanks, that's definitely part of it. If we handle
Right, that is what we'd like in the longer term, as I mentioned "eventually, it would be great to be able to understand the various macros and how they could serve as checks for later accesses". |
|
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". |
|
Could clang/lib/ASTMatchers/GtestMatchers.cpp help you to have a better solution? |
Also fix a CHECK-MESSAGE -> CHECK-MESSAGES
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks! Ok that does that look useful (and I think the our other reviewer ymand@ wrote it =) https://reviews.llvm.org/D74840).
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! |
There was a problem hiding this comment.
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 removedfor those test frameworks if only the GTest macros are handled, which could be complex enough without implementing the REQUIRE macro as well.
There was a problem hiding this comment.
Please also add an ASSERT_FALSE (or just ASSERT_TRUE(!opt.has_value())) with a dereference after, to show that there are false-negatives
There was a problem hiding this comment.
Done (there was a false-negative example in unchecked_value_access but also added the ASSERT_FALSE)
| 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; |
There was a problem hiding this comment.
nit: var casing should be CamelCase
| // 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). |
There was a problem hiding this comment.
Please add a release note, and add the option to the check documentation with an explanation similar to this, but targeted towards the user.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
TEST(a, b) {
std::optional<int> a;
if (v) {
a = 1;
}
ASSERT_NE(a, std::nullopt);
a.value();
}
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);
+1. That's responsibility of the build system. "What is a test" is something that can vary from project to project. There's many other clang-tidy checks that don't make sense in tests (for example,
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. |
Our codebase doesn't use test folders. We have no foolproof way of distinguishing code from test based on the file system. |
|
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:
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! |
|
Gentle ping, since it's been a week since my last post. |
|
I think the ideal solution would be something like using the 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 user I would definitely prefer the latter. |
There was a problem hiding this comment.
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.
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
Outdated
Show resolved
Hide resolved
- lightweight detection of catch2 - style etc. - add release note about option
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)
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
Ah sorry, yes, I hadn't addressed the earlier review comments since it seemed like bigger picture discussion was still open. Update the patch. |
Just coming back to this, would you mind clarifying "but a second non-blocking review from another clang-tidy dev would be best"? Does ymand and Xazax-hun count? |
Sorry I missed this comment earlier! I think doing "unless( ... gtestAssert... GtestCmp...)" for filtering this way would be more complex for not much gain, over the current filtering. Is the concern accuracy? regarding complexity: there are many forms to check if the optional has a value, so doing filters this way (with "unless") we would need to enumerate all of them (gtestExpect, gtestExpectThat, gtestAssertThat, a to-be-added gtestExpectTrue, gtestAssertTrue, might need GtestCmp::Eq as well as Ne, like ASSERT_EQ(a.has_value(), true)). Similarly if we want to filter out catch2. At that point we may as well handle the matcher + transfer functions to "see through" the macros in the checker itself rather than filtering here in a simple way (but then see prioritization concern). regarding accuracy: I've run a large scale experiment across our monorepo, and the current method seems accurate w.r.t. filtering test files, test utilities, test base classes, fakes, mock classes, etc. Only 0.4% of the diffs didn't have "test/Test/TEST/..." etc in the path for file name (as a rough check). Among that 0.4% some really were test-only libraries that were less obviously test-only from the file name (golden_generator, sampler, fake, mock, benchmark, etc.), and some were timeout diffs. |
|
I would love to see this PR merged; false positives from googletest prevent my project from enabling this powerful check (https://issues.pigweed.dev/issues/405164174). What's the next step? @5chmidti if another clang-tidy dev should review this, could you recommend one? |
Behind an option (default is off / current behavior)
We currently do not handle:
ASSERT_TRUE(opt.has_value())macros followed by*opt(inside or outside macros) from googletestASSERT_THAT(opt, Not(Eq(std::nullopt))))So would see lots of false positives in test TUs. Some test TUs have 100s to 1000 warnings, which are too many to look through to find "true" signals, to make it worth warning in tests.