[PATCH] D132654: [clang-tidy] Fix false positive on `ArrayInitIndexExpr` inside `ProBoundsConstantArrayIndexCheck`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs created this revision.
isuckatcs added reviewers: aaron.ballman, LegalizeAdulthood, njames93.
Herald added subscribers: carlosgalvezp, arphaman, kbarton, xazax.hun, nemanjai.
Herald added a project: All.
isuckatcs requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

There are 3 different cases when an `ArrayInitLoopExpr` is used to initialize an array.

- in the implicit copy/move constructor for a class with an array member
- when a lambda-expression captures an array by value
- when a decomposition declaration decomposes an array

The AST of such expression (usually) looks like this:

  |-ArrayInitLoopExpr 'int[3]'
  | | |-OpaqueValueExpr 'int[3]' lvalue
  | | | `-DeclRefExpr 'int[3]' lvalue Var 'arr' 'int[3]'
  | | `-ImplicitCastExpr 'int' <LValueToRValue>
  | |   `-ArraySubscriptExpr 'int' lvalue
  | |     |-ImplicitCastExpr 'int *' <ArrayToPointerDecay>
  | |     | `-OpaqueValueExpr 'int[3]' lvalue
  | |     |   `-DeclRefExpr 'int[3]' lvalue Var 'arr' 'int[3]'
  | |     `-ArrayInitIndexExpr <<invalid sloc>> 'unsigned long'

We basically always have an `ArraySubscriptExpr`, where the index is an `ArrayInitIndexExpr`.
`ArrayInitIndexExpr` is not a constant, so the checker mentioned in the title reports a warning.
This false positive warning only happens in case of decomposition declaration and lambda capture.

Some example without this patch:

  void foo() {
      int arr[3];
  
      auto [a, b, c] = arr;
  }
  ---------------------------------------------------------------
  warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
  auto [a, b, c] = arr;
                   ^



  void foo() {
      int arr[3];
  
      [arr]() {};
  }
  ---------------------------------------------------------------
  warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
  [arr]() {};
   ^


https://reviews.llvm.org/D132654

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index.cpp
@@ -1,4 +1,5 @@
 // RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-constant-array-index %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-constant-array-index -extra-arg=-std=c++17 %t
 
 typedef __SIZE_TYPE__ size_t;
 
@@ -75,13 +76,34 @@
   s[i] = 3; // OK, custom operator
 }
 
+namespace ArrayInitIndexExpr {
 struct A {
   // The compiler-generated copy constructor uses an ArraySubscriptExpr. Don't warn.
   int x[3];
 };
 
-void use_A() {
+void implicitCopyMoveCtor() {
   // Force the compiler to generate a copy constructor.
   A a;
   A a2(a);
+
+  // Force the compiler to generate a move constructor.
+  A a3 = (A&&) a;
+}
+
+void lambdaCapture() {
+  int arr[3];
+
+  // Capturing an array by value uses an ArraySubscriptExpr. Don't warn. 
+  [arr](){};
+}
+
+#if __cplusplus >= 201703L
+void structuredBindings() {
+  int arr[3];
+
+  // Creating structured bindings by value uses an ArraySubscriptExpr. Don't warn.
+  auto [a,b,c] = arr;
 }
+#endif
+} // namespace ArrayInitIndexExpr
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
@@ -61,6 +61,12 @@
   const auto *Matched = Result.Nodes.getNodeAs<Expr>("expr");
   const auto *IndexExpr = Result.Nodes.getNodeAs<Expr>("index");
 
+  // This expression can only appear inside ArrayInitLoopExpr, which
+  // is always implicitly generated. ArrayInitIndexExpr is not a
+  // constant, but we shouldn't report a warning for it.
+  if (isa<ArrayInitIndexExpr>(IndexExpr))
+    return;
+
   if (IndexExpr->isValueDependent())
     return; // We check in the specialization.
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D132654.455562.patch
Type: text/x-patch
Size: 2217 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220825/ad944c48/attachment-0001.bin>