[clang-tidy] Fix incorrect array name generation in cppcoreguidelines-pro-bounds-constant-array-index

[Quolyk]

Summary

This patch fixes incorrect array name generation for a cppcoreguidelines-pro-bounds-constant-array-index warning.
Motivation: https://bugs.llvm.org/show_bug.cgi?id=38510.

[Quolyk]

For now I just updated tests. The problem is in BaseRange definition, as it holds EndLoc and BeginLoc pointing to the beginning of ArrayExpression base https://github.com/llvm-mirror/clang-tools-extra/blob/e0441f6939da38f26bea9c1d75bb33024daa4e40/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp#L78. I'm investigating this.

[Quolyk]

Patch is not yet fixed.

[JonasToth]

Thank you for working on this!

Could you please run the check over LLVM or any other significant codebase once you have the fix implemented and report if the transformation still breaks code?

[Quolyk]

Update. Solution is not elegant, because DeclRef->BaseRange somehow has zero length.

[JonasToth]

Sorry for the long delay!

I have one question about a slighty different code constellation.

std::optional<std::array<int, 4>> my_array;

for(int i = 0; i < 42; ++i)
    (*my_array)[i] = 42;

Will this be fixed properly?

for(int i = 0; i < 42; ++i)
    gsl::at(*my_array, i) = 42;

If the braces in (*my_array) would be moved into the function that is ok as well.

This is just of general interest and should not block this patch in my opinion, as its a different bug.

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

 void ProBoundsConstantArrayIndexCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   // Note: if a struct contains an array member, the compiler-generated
   // constructor has an arraySubscriptExpr.
   Finder->addMatcher(
       arraySubscriptExpr(
-          hasBase(ignoringImpCasts(hasType(constantArrayType().bind("type")))),
+          hasBase(ignoringImpCasts(declRefExpr(
+              hasType(constantArrayType().bind("type"))).bind("declRef"))),
           hasIndex(expr().bind("index")), unless(hasAncestor(isImplicit())))
           .bind("expr"),
       this);
 
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasOverloadedOperatorName("[]"),
           hasArgument(
-              0, hasType(cxxRecordDecl(hasName("::std::array")).bind("type"))),
+            0, declRefExpr(hasType(cxxRecordDecl(
+                hasName("::std::array")).bind("type"))).bind("declRef")),
           hasArgument(1, expr().bind("index")))
           .bind("expr"),
       this);
 }
 
 void ProBoundsConstantArrayIndexCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Matched = Result.Nodes.getNodeAs<Expr>("expr");
   const auto *IndexExpr = Result.Nodes.getNodeAs<Expr>("index");
+  const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef");
 
   if (IndexExpr->isValueDependent())
     return; // We check in the specialization.
 
   llvm::APSInt Index;
   if (!IndexExpr->isIntegerConstantExpr(Index, *Result.Context, nullptr,
                                         /*isEvaluated=*/true)) {
-    SourceRange BaseRange;
-    if (const auto *ArraySubscriptE = dyn_cast<ArraySubscriptExpr>(Matched))
-      BaseRange = ArraySubscriptE->getBase()->getSourceRange();
-    else
-      BaseRange =
-          dyn_cast<CXXOperatorCallExpr>(Matched)->getArg(0)->getSourceRange();
+    SourceRange BaseRange = DeclRef->getSourceRange();
     SourceRange IndexRange = IndexExpr->getSourceRange();
+    StringRef DeclRefName = DeclRef->getFoundDecl()->getName();
 
     auto Diag = diag(Matched->getExprLoc(),
                      "do not use array subscript when the index is "
                      "not an integer constant expression; use gsl::at() "
                      "instead");
     if (!GslHeader.empty()) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
            << FixItHint::CreateReplacement(
-                  SourceRange(BaseRange.getEnd().getLocWithOffset(1),
+                  SourceRange(BaseRange.getBegin(),
                               IndexRange.getBegin().getLocWithOffset(-1)),
-                  ", ")
+                  (DeclRefName + ", ").str())
            << FixItHint::CreateReplacement(Matched->getEndLoc(), ")");
 
       Optional<FixItHint> Insertion = Inserter->CreateIncludeInsertion(
           Result.SourceManager->getMainFileID(), GslHeader,
           /*IsAngled=*/false);
       if (Insertion)
         Diag << Insertion.getValue();
     }

test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp

namespace gsl {
   template<class T, size_t N>
   T& at( std::array<T, N> &a, size_t index );
 }
 
 constexpr int const_index(int base) {
   return base + 3;
 }
 
-void f(std::array<int, 10> a, int pos) {
-  a [ pos / 2 /*comment*/] = 1;
+void f(std::array<int, 10> list, int pos) {
+  list [ pos / 2 /*comment*/] = 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
-  // CHECK-FIXES: gsl::at(a,  pos / 2 /*comment*/) = 1;
-  int j = a[pos - 1];
+  // CHECK-FIXES: gsl::at(list,  pos / 2 /*comment*/) = 1;
+  int j = list[pos - 1];
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
-  // CHECK-FIXES: int j = gsl::at(a, pos - 1);
+  // CHECK-FIXES: int j = gsl::at(list, pos - 1);
 
-  a.at(pos-1) = 2; // OK, at() instead of []
-  gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
+  list.at(pos-1) = 2; // OK, at() instead of []
+  gsl::at(list, pos-1) = 2; // OK, gsl::at() instead of []
 
-  a[-1] = 3;
+  list[-1] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
-  a[10] = 4;
+  list[10] = 4;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements) [cppcoreguidelines-pro-bounds-constant-array-index]
 
-  a[const_index(7)] = 3;
+  list[const_index(7)] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
 
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, constant index and inside bounds
+  list[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 void g() {
-  int a[10];
+  int list[10];
   for (int i = 0; i < 10; ++i) {
-    a[i] = i;
+    list[i] = i;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
-    // CHECK-FIXES: gsl::at(a, i) = i;
-    gsl::at(a, i) = i; // OK, gsl::at() instead of []
+    // CHECK-FIXES: gsl::at(list, i) = i;
+    gsl::at(list, i) = i; // OK, gsl::at() instead of []
   }
 
-  a[-1] = 3; // flagged by clang-diagnostic-array-bounds
-  a[10] = 4; // flagged by clang-diagnostic-array-bounds
-  a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
-
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[-1] = 3; // flagged by clang-diagnostic-array-bounds
+  list[10] = 4; // flagged by clang-diagnostic-array-bounds
+  list[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
+
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, constant index and inside bounds
+  list[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 struct S {
   int& operator[](int i);
 };
 
 void customOperator() {
   S s;
   int i = 0;
   s[i] = 3; // OK, custom operator
 }

test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp

namespace gsl {
   template<class T, size_t N>
   T& at( std::array<T, N> &a, size_t index );
 }
 
 constexpr int const_index(int base) {
   return base + 3;
 }
 
-void f(std::array<int, 10> a, int pos) {
-  a [ pos / 2 /*comment*/] = 1;
+void f(std::array<int, 10> list, int pos) {
+  list [ pos / 2 /*comment*/] = 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
-  int j = a[pos - 1];
+  int j = list[pos - 1];
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
 
-  a.at(pos-1) = 2; // OK, at() instead of []
-  gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
+  list.at(pos-1) = 2; // OK, at() instead of []
+  gsl::at(list, pos-1) = 2; // OK, gsl::at() instead of []
 
-  a[-1] = 3;
+  list[-1] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
-  a[10] = 4;
+  list[10] = 4;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements) [cppcoreguidelines-pro-bounds-constant-array-index]
 
-  a[const_index(7)] = 3;
+  list[const_index(7)] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
 
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, constant index and inside bounds
+  list[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 void g() {
-  int a[10];
+  int list[10];
   for (int i = 0; i < 10; ++i) {
-    a[i] = i;
+    list[i] = i;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
-    // CHECK-FIXES: gsl::at(a, i) = i;
-    gsl::at(a, i) = i; // OK, gsl::at() instead of []
+    // CHECK-FIXES: gsl::at(list, i) = i;
+    gsl::at(list, i) = i; // OK, gsl::at() instead of []
   }
 
-  a[-1] = 3; // flagged by clang-diagnostic-array-bounds
-  a[10] = 4; // flagged by clang-diagnostic-array-bounds
-  a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
-
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[-1] = 3; // flagged by clang-diagnostic-array-bounds
+  list[10] = 4; // flagged by clang-diagnostic-array-bounds
+  list[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
+
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, constant index and inside bounds
+  list[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 struct S {
   int& operator[](int i);
 };
 
 void customOperator() {
   S s;