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

[fwolff]

Summary

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.

[whisperity]

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

20–23

Perhaps we could use constexpr llvm::StringLiteral?

[fwolff]

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

20–23

Fixed.

[compnerd]

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

22

Would you mind using addr-of-container-expr and renaming this to AddrOfContainerExprName?

77

Nice, I do like this, it is quite a bit more succinct. One thing that is a bit less clear to me is that the previous expression was a bit more restrictive in the parameter types to certain expressions, is there a reason that you don't expect that to matter much in practice? If a malformed input is provided, we could now match certain things that we didn't previously, or am I not matching up the conditions carefully enough?

84

The diff shows up odd here, is there a hard tab or is that just the rendering?

[fwolff]

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

77

I have indeed changed things slightly to make everything more consistent. For instance, calling &vp->operator[](0) currently does not yield a warning (Godbolt link), but &v.operator[](0) does, because the old check allowed a pointer to a container for a cxxOperatorCallExpr() (line 62 on the left) but not for cxxMemberCallExpr(). I think this was simply an oversight because the check was written in such a redundant way; my version covers both.

So yes, I have made some checks a bit less restrictive, but I think that's an improvement. Or which cases were you thinking of exactly?

84

I think this is Phabricator's way of saying that the only thing that was changed in this line is the indentation. I haven't used any tabs.

[aaron.ballman]

Mostly just nits from me. At a high level, I think this is good, but if someone else wanted to double-check the changes to the matcher statements themselves, that'd be good.

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

81–105

Might as well use the constructor rather than assignment operator here.

86–87

94

Drop top-level const and spell out the type explicitly.

[fwolff]

Fixed the nits. Thanks for the review @aaron.ballman!

[aaron.ballman]

LGTM!

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

 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
 
 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) {}
 
 void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto Record =
       cxxRecordDecl(
           isSameOrDerivedFrom(
               namedDecl(
                   has(cxxMethodDecl(isPublic(), hasName("data")).bind("data")))
                   .bind("container")))
           .bind("record");
 
   const auto NonTemplateContainerType =
       qualType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(Record))));
   const auto TemplateContainerType =
       qualType(hasUnqualifiedDesugaredType(templateSpecializationType(
           hasDeclaration(classTemplateDecl(has(Record))))));
 
   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(
-              ignoringParenImpCasts(
-                  cxxOperatorCallExpr(
-                      callee(cxxMethodDecl(hasName("operator[]"))
-                                 .bind("operator[]")),
-                      argumentCountIs(2),
-                      hasArgument(
-                          0,
-                          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"),
+          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);
 }
 
 void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>("address-of");
-  const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("var");
+  const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>(AddressOfName);
+  const auto *CE = Result.Nodes.getNodeAs<Expr>(ContainerExprName);
+  const auto *DCE = Result.Nodes.getNodeAs<Expr>(DerefContainerExprName);
+  const auto *ACE = Result.Nodes.getNodeAs<Expr>(AddrOfContainerExprName);
+
+  if (!UO || !CE)
+    return;
+
+  if (DCE && !CE->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 + ")";
 
-  std::string ReplacementText;
-  ReplacementText = std::string(Lexer::getSourceText(
-      CharSourceRange::getTokenRange(DRE->getSourceRange()),
-      *Result.SourceManager, getLangOpts()));
-  if (DRE->getType()->isPointerType())
+  if (CE->getType()->isPointerType())
     ReplacementText += "->data()";
   else
     ReplacementText += ".data()";
 
   FixItHint Hint =
       FixItHint::CreateReplacement(UO->getSourceRange(), ReplacementText);
   diag(UO->getBeginLoc(),
        "'data' should be used for accessing the data pointer instead of taking "
        "the address of the 0-th element")
       << Hint;
 }
 } // namespace readability
 } // namespace tidy
 } // namespace clang

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

 }
 
 template <typename T>
 void m(const std::vector<T> &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();{{$}}
 }
+
+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();{{$}}
+}