[clang-tidy] Access checks not done classes derived of std::array

[sousajo]

Summary

Index accessing checks are not performed for derived classes of
of std::array, as only std::array itself and its aliases
seems to be checked.

This patch aims to extend it for derived classes such as:

template<class T, size_t N>
class DerivedArray : public std::array<T, N> {};

[PiotrZSL]

Good, just verify that documentation for this check is still proper, and add entry in release notes about this change.

[PiotrZSL]

LGTM

[sousajo]

hi @PiotrZSL, thanks for the review :) can you please land it for me?

Jorge Pinto Sousa <[e-mail]>

[carlosgalvezp]

What about the use case of privately inheriting std::array, and overriding the operator[] there with bounds check? I believe there shouldn't be warnings there.

Would it be possible to check only _public_ inheritance?

[sousajo]

In D156624#4545107, @PiotrZSL wrote:

Good, just verify that documentation for this check is still proper, and add entry in release notes about this change.

I missed those, I will sure ^^

In D156624#4545117, @carlosgalvezp wrote:

What about the use case of privately inheriting std::array, and overriding the operator[] there with bounds check? I believe there shouldn't be warnings there.

Would it be possible to check only _public_ inheritance?

Good point, I cannot think of an _easy_ way to check it.

[PiotrZSL]

In D156624#4545170, @sousajo wrote:

Good point, I cannot think of an _easy_ way to check it.

I think you can do something like this:

Finder->addMatcher(
    cxxOperatorCallExpr(
        hasOverloadedOperatorName("[]"),
        calle(cxxMethodDecl(ofClass(cxxRecordDecl(hasName("::std::array")).bind("type"))))
        hasArgument(1, expr().bind("index")))
        .bind("expr"),
    this);

In such case instead of checking argument passed to call, we check what operator we used and who provided it.

[sousajo]

@PiotrZSL @carlosgalvezp can you please recheck?

[PiotrZSL]

LGTM

[sousajo]

thanks for the review. can you please land it for me pls? Jorge Pinto Sousa <[e-mail]>

clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

Finder->addMatcher(arraySubscriptExpr(hasBase(ignoringImpCasts(hasType(
                                         hasIndex(expr().bind("index")),
                                         unless(hasAncestor(decl(isImplicit()))))
                          .bind("expr"),
                      this);
 
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasOverloadedOperatorName("[]"),
-          hasArgument(
-              0, hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
-                     cxxRecordDecl(hasName("::std::array")).bind("type")))))),
+          callee(cxxMethodDecl(
+              ofClass(cxxRecordDecl(hasName("::std::array")).bind("type")))),
           hasArgument(1, expr().bind("index")))
           .bind("expr"),
       this);
 }
 
 void ProBoundsConstantArrayIndexCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Matched = Result.Nodes.getNodeAs<Expr>("expr");

clang-tools-extra/docs/ReleaseNotes.rst

 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   ignore delegate constructors.
 
 - Improved :doc:`cppcoreguidelines-pro-bounds-array-to-pointer-decay
   <clang-tidy/checks/cppcoreguidelines/pro-bounds-array-to-pointer-decay>` check
   to ignore predefined expression (e.g., ``__func__``, ...).
 
+- Improved :doc:`cppcoreguidelines-pro-bounds-constant-array-index
+  <clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index>` check
+  to perform checks on derived classes of  ``std::array``.
+
 - Improved :doc:`cppcoreguidelines-pro-type-member-init
   <clang-tidy/checks/cppcoreguidelines/pro-type-member-init>` check to ignore
   dependent delegate constructors.
 
 - Improved :doc:`cppcoreguidelines-pro-type-vararg
   <clang-tidy/checks/cppcoreguidelines/pro-type-vararg>` check to ignore
   false-positives in unevaluated context (e.g., ``decltype``, ``sizeof``, ...).
 

clang-tools-extra/test/clang-tidy/checkers/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;
 }
 
+template<class T, size_t N>
+class DerivedArray : public std::array<T, N> {};
+
 void f(std::array<int, 10> a, int pos) {
   a [ pos / 2 /*comment*/] = 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
   int j = a[pos - 1];
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression
 
   a.at(pos-1) = 2; // OK, at() instead of []
   gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
void f(std::array<int, 10> a, int pos) {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
 
   m[0] = 3; // OK, constant index and inside bounds
   m[1] = 3; // OK, constant index and inside bounds
   m[9] = 3; // OK, constant index and inside bounds
   m[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
+template<class T, size_t N>
+class PrivateDerivedArray : std::array<T, N> {
+public:
+  T& operator[](size_t n){
+    return std::array<T, N>::operator[](static_cast<int>(n));
+  };
+  T& at(size_t n) {
+    return std::array<T, N>::at(static_cast<int>(n));
+  };
+};
+
+void f_derived(DerivedArray<int, 10> a, int pos) {
+  a [ pos / 2 /*comment*/] = 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
+  int j = a[pos - 1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression
+
+  a.at(pos-1) = 2; // OK, at() instead of []
+  gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
+
+  a[-1] = 3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
+  a[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;
+  // 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
+
+  using MyArray = DerivedArray<int, 10>;
+  MyArray m{};
+  m [ pos / 2 /*comment*/] = 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
+  int jj = m[pos - 1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use array subscript when the index is not an integer constant expression
+
+  m.at(pos-1) = 2; // OK, at() instead of []
+  gsl::at(m, pos-1) = 2; // OK, gsl::at() instead of []
+  m[-1] = 3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
+  m[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]
+
+  m[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)
+
+  m[0] = 3; // OK, constant index and inside bounds
+  m[1] = 3; // OK, constant index and inside bounds
+  m[9] = 3; // OK, constant index and inside bounds
+  m[const_index(6)] = 3; // OK, constant index and inside bounds
+
+  using MyPrivateArray = PrivateDerivedArray<int, 10>;
+  MyPrivateArray pm{};
+  pm [ pos / 2 /*comment*/] = 1;
+  int jjj = pm[pos - 1];
+
+  pm.at(pos-1) = 2; // OK, at() instead of []
+  pm[-1] = 3;
+  pm[10] = 4;
+
+  pm[const_index(7)] = 3;
+
+  pm[0] = 3; // OK, constant index and inside bounds
+  pm[1] = 3; // OK, constant index and inside bounds
+  pm[9] = 3; // OK, constant index and inside bounds
+  pm[const_index(6)] = 3; // OK, constant index and inside bounds
+}
+
+
+
+
 void g() {
   int a[10];
   for (int i = 0; i < 10; ++i) {
     a[i] = i;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression
     // CHECK-FIXES: gsl::at(a, i) = i;
     gsl::at(a, i) = i; // OK, gsl::at() instead of []
   }