[clang-tidy] Make readability-container-data-pointer more robust

Fixes PR#52245. I've also added a few test cases beyond PR#52245 that would also fail with the current implementation, which is quite brittle in many respects (e.g. it uses the `hasDescendant()` matcher to find the container that is being accessed, which is very easy to trick, as in the example in PR#52245).

I have not been able to reproduce the second issue mentioned in PR#52245 (namely that using the `data()` member function is suggested even for containers that don't have it), but I've added a test case for it to be sure.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D113863

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

@@ -16,6 +16,13 @@ using namespace clang::ast_matchers;
 namespace clang {
 namespace tidy {
 namespace readability {
+
+constexpr llvm::StringLiteral ContainerExprName = "container-expr";
+constexpr llvm::StringLiteral DerefContainerExprName = "deref-container-expr";
+constexpr llvm::StringLiteral AddrOfContainerExprName =
+    "addr-of-container-expr";
+constexpr llvm::StringLiteral AddressOfName = "address-of";
+
 ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {}
@@ -38,69 +45,63 @@ void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto Container =
       qualType(anyOf(NonTemplateContainerType, TemplateContainerType));
 
+  const auto ContainerExpr = anyOf(
+      unaryOperator(
+          hasOperatorName("*"),
+          hasUnaryOperand(
+              expr(hasType(pointsTo(Container))).bind(DerefContainerExprName)))
+          .bind(ContainerExprName),
+      unaryOperator(hasOperatorName("&"),
+                    hasUnaryOperand(expr(anyOf(hasType(Container),
+                                               hasType(references(Container))))
+                                        .bind(AddrOfContainerExprName)))
+          .bind(ContainerExprName),
+      expr(anyOf(hasType(Container), hasType(pointsTo(Container)),
+                 hasType(references(Container))))
+          .bind(ContainerExprName));
+
+  const auto Zero = integerLiteral(equals(0));
+
+  const auto SubscriptOperator = callee(cxxMethodDecl(hasName("operator[]")));
+
   Finder->addMatcher(
       unaryOperator(
           unless(isExpansionInSystemHeader()), hasOperatorName("&"),
-          hasUnaryOperand(anyOf(
+          hasUnaryOperand(expr(
-              ignoringParenImpCasts(
+              anyOf(cxxOperatorCallExpr(SubscriptOperator, argumentCountIs(2),
-                  cxxOperatorCallExpr(
+                                        hasArgument(0, ContainerExpr),
-                      callee(cxxMethodDecl(hasName("operator[]"))
+                                        hasArgument(1, Zero)),
-                                 .bind("operator[]")),
+                    cxxMemberCallExpr(SubscriptOperator, on(ContainerExpr),
-                      argumentCountIs(2),
+                                      argumentCountIs(1), hasArgument(0, Zero)),
-                      hasArgument(
+                    arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero))))))
-                          0,
+          .bind(AddressOfName),
-                          anyOf(ignoringParenImpCasts(
-                                    declRefExpr(
-                                        to(varDecl(anyOf(
-                                            hasType(Container),
-                                            hasType(references(Container))))))
-                                        .bind("var")),
-                                ignoringParenImpCasts(hasDescendant(
-                                    declRefExpr(
-                                        to(varDecl(anyOf(
-                                            hasType(Container),
-                                            hasType(pointsTo(Container)),
-                                            hasType(references(Container))))))
-                                        .bind("var"))))),
-                      hasArgument(1,
-                                  ignoringParenImpCasts(
-                                      integerLiteral(equals(0)).bind("zero"))))
-                      .bind("operator-call")),
-              ignoringParenImpCasts(
-                  cxxMemberCallExpr(
-                      hasDescendant(
-                          declRefExpr(to(varDecl(anyOf(
-                                          hasType(Container),
-                                          hasType(references(Container))))))
-                              .bind("var")),
-                      argumentCountIs(1),
-                      hasArgument(0,
-                                  ignoringParenImpCasts(
-                                      integerLiteral(equals(0)).bind("zero"))))
-                      .bind("member-call")),
-              ignoringParenImpCasts(
-                  arraySubscriptExpr(
-                      hasLHS(ignoringParenImpCasts(
-                          declRefExpr(to(varDecl(anyOf(
-                                          hasType(Container),
-                                          hasType(references(Container))))))
-                              .bind("var"))),
-                      hasRHS(ignoringParenImpCasts(
-                          integerLiteral(equals(0)).bind("zero"))))
-                      .bind("array-subscript")))))
-          .bind("address-of"),
       this);
 }
 
 void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>("address-of");
+  const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>(AddressOfName);
-  const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("var");
+  const auto *CE = Result.Nodes.getNodeAs<Expr>(ContainerExprName);
+  const auto *DCE = Result.Nodes.getNodeAs<Expr>(DerefContainerExprName);
+  const auto *ACE = Result.Nodes.getNodeAs<Expr>(AddrOfContainerExprName);
 
-  std::string ReplacementText;
+  if (!UO || !CE)
-  ReplacementText = std::string(Lexer::getSourceText(
+    return;
-      CharSourceRange::getTokenRange(DRE->getSourceRange()),
+
-      *Result.SourceManager, getLangOpts()));
+  if (DCE && !CE->getType()->isPointerType())
-  if (DRE->getType()->isPointerType())
+    CE = DCE;
+  else if (ACE)
+    CE = ACE;
+
+  SourceRange SrcRange = CE->getSourceRange();
+
+  std::string ReplacementText{
+      Lexer::getSourceText(CharSourceRange::getTokenRange(SrcRange),
+                           *Result.SourceManager, getLangOpts())};
+
+  if (!isa<DeclRefExpr, ArraySubscriptExpr, CXXOperatorCallExpr, CallExpr>(CE))
+    ReplacementText = "(" + ReplacementText + ")";
+
+  if (CE->getType()->isPointerType())
     ReplacementText += "->data()";
   else
     ReplacementText += ".data()";

clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

@@ -109,3 +109,38 @@ void m(const std::vector<T> &v) {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
   // CHECK-FIXES: {{^  }}return v.data();{{$}}
 }
+
+template <typename T>
+struct container_without_data {
+  using size_type = size_t;
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template <typename T>
+const T *n(const container_without_data<T> &c) {
+  // c has no "data" member function, so there should not be a warning here:
+  return &c[0];
+}
+
+const int *o(const std::vector<std::vector<std::vector<int>>> &v, const size_t idx1, const size_t idx2) {
+  return &v[idx1][idx2][0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}return v[idx1][idx2].data();{{$}}
+}
+
+std::vector<int> &select(std::vector<int> &u, std::vector<int> &v) {
+  return v;
+}
+
+int *p(std::vector<int> &v1, std::vector<int> &v2) {
+  return &select(*&v1, v2)[0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}return select(*&v1, v2).data();{{$}}
+}
+
+int *q(std::vector<int> ***v) {
+  return &(***v)[0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}return (**v)->data();{{$}}
+}