[clang-tools-extra] [clang-tidy] Fix `readability-container-data-pointer` check (PR #165636)

[via cfe-commits]

================
@@ -71,20 +71,20 @@ void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
 
   const auto Zero = integerLiteral(equals(0));
 
-  const auto SubscriptOperator = callee(cxxMethodDecl(hasName("operator[]")));
-
-  Finder->addMatcher(
+  const auto AddressOfMatcher =
       unaryOperator(
           unless(isExpansionInSystemHeader()), hasOperatorName("&"),
-          hasUnaryOperand(expr(
-              anyOf(cxxOperatorCallExpr(SubscriptOperator, argumentCountIs(2),
-                                        hasArgument(0, ContainerExpr),
-                                        hasArgument(1, Zero)),
-                    cxxMemberCallExpr(SubscriptOperator, on(ContainerExpr),
-                                      argumentCountIs(1), hasArgument(0, Zero)),
-                    arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero))))))
-          .bind(AddressOfName),
-      this);
+          hasUnaryOperand(ignoringParenImpCasts(expr(anyOf(
+              cxxOperatorCallExpr(
+                  hasOverloadedOperatorName("[]"), argumentCountIs(2),
+                  hasArgument(0, ContainerExpr), hasArgument(1, Zero)),
+              cxxMemberCallExpr(callee(cxxMethodDecl(hasName("operator[]"))),
+                                on(ContainerExpr), argumentCountIs(1),
+                                hasArgument(0, Zero)),
+              arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero)))))))
+          .bind(AddressOfName);
+
+  Finder->addMatcher(traverse(TK_AsIs, AddressOfMatcher), this);
----------------
zeyi2 wrote:

Hi, I tried to implement the new check without `TK_AsIs` and I encountered some difficulties:

The former template case `m` is a simple "dependent type + subscript" inside a template body. The old matcher can recognize containers by inspecting the primary template declaration. The new test case `u` is an "overloaded operator* followed by operator[]", it requires knowing that the dependent result type `T` actually has a `data()` method to safely emit `(*p).data()`.

With `TK_IgnoreUnlessSpelledInSource`, many nodes remain dependent in uninstantiated template bodies, which makes predicates fail to check whether the type really has `.data()`.

To fix this issue, I switched to a syntax‑driven approach that doesn’t rely on type constraints in the matcher (only matches address-of subscript zero) and do all container detection in `check()`. But still, resolving "type has a public data()" on a dependent specialization is not reliably possible, so now the fix for  `m (return v.data();)` cannot be emitted conservatively.

I think it will be hard to proceed without `TK_AsIs`, I may need to reimplement a dependency-aware walk from a dependent specialization back to its primary template in `check()`, which may introduce more problems.

Would you be open to a compromise where we enable `TK_AsIs` only for this check? IMHO it is a more practical and low-risk option.

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