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

[Jan Voung via cfe-commits]

jvoung wrote:

> 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();
> }
> ```
> 
> ```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);
> ```

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.


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