[clang-tidy] Add new check cppcoreguidelines-pro-bounds-array-to-pointer-decay

[mgehre]

Summary

This check flags all array to pointer decays.

Pointers should not be used as arrays. array_view is a bounds-checked,
safe alternative to using pointers to access arrays.

This rule is part of the "Bounds safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds3-no-array-to-pointer-decay

[sbenza]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

29 ↗

(On Diff #37069)

__range is an implementation detail and should not be used here.
Check that this is the range init of a cxxForRangeStmt parent node or something like that.

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

12 ↗

(On Diff #37069)

this is not implicit. Is the 'implicit' optional?

[mgehre]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

29 ↗

(On Diff #37069)

That was my first attempt, but I couldn't quite figure it out.

I tried

unless(hasAncestor(cxxForRangeStmt(hasRangeInit(hasDescendant(equalsBoundNode("cast"))))))

but that does not compile.

Any ideas?

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

12 ↗

(On Diff #37069)

Currently, the diag is

diag(MatchedCast->getExprLoc(), "do not (implicitly) convert an array to a pointer");

should I make the parenthesis conditional?

[klimek]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

40 ↗

(On Diff #37069)

Can't we provide a fixit?

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

29 ↗

(On Diff #37069)

I'm not certain that you'll be able to do that. Looking at the AST dump:

TranslationUnitDecl 0xa40168 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0xa40458 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'char *'
`-FunctionDecl 0xa404c0 <E:\Desktop\test1.cpp:1:1, line:5:1> line:1:6 f 'void (void)'
  `-CompoundStmt 0xa40b88 <col:10, line:5:1>
    |-DeclStmt 0xa40600 <line:2:3, col:11>
    | `-VarDecl 0xa405c8 <col:3, col:10> col:7 used a 'int [5]'
    `-CXXForRangeStmt 0xa40b50 <line:3:3, line:4:5>
      |-DeclStmt 0xa40828 <line:3:16>
      | `-VarDecl 0xa406b8 <col:16> col:16 implicit used __range 'int (&&)[5]' cinit
      |   `-DeclRefExpr 0xa40610 <col:16> 'int [5]' lvalue Var 0xa405c8 'a' 'int [5]'
      |-DeclStmt 0xa40a00 <col:14>
      | |-VarDecl 0xa40870 <col:14> col:14 implicit used __begin 'int *':'int *' cinit
      | | `-ImplicitCastExpr 0xa40960 <col:14> 'int *' <ArrayToPointerDecay>
      | |   `-DeclRefExpr 0xa40838 <col:14> 'int [5]' lvalue Var 0xa406b8 '__range' 'int (&&)[5]'
      | `-VarDecl 0xa408b0 <col:14, col:16> col:14 implicit used __end 'int *':'int *' cinit
      |   `-BinaryOperator 0xa409a0 <col:14, col:16> 'int *' '+'
      |     |-ImplicitCastExpr 0xa40990 <col:14> 'int *' <ArrayToPointerDecay>
      |     | `-DeclRefExpr 0xa40850 <col:14> 'int [5]' lvalue Var 0xa406b8 '__range' 'int (&&)[5]'
      |     `-IntegerLiteral 0xa40970 <col:16> 'int' 5
      |-BinaryOperator 0xa40a60 <col:14> '_Bool' '!='
      | |-ImplicitCastExpr 0xa40a40 <col:14> 'int *':'int *' <LValueToRValue>
      | | `-DeclRefExpr 0xa40a10 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      | `-ImplicitCastExpr 0xa40a50 <col:14> 'int *':'int *' <LValueToRValue>
      |   `-DeclRefExpr 0xa40a28 <col:14> 'int *':'int *' lvalue Var 0xa408b0 '__end' 'int *':'int *'
      |-UnaryOperator 0xa40a90 <col:14> 'int *':'int *' lvalue prefix '++'
      | `-DeclRefExpr 0xa40a78 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      |-DeclStmt 0xa40680 <col:7, col:18>
      | `-VarDecl 0xa40648 <col:7, col:14> col:12 e 'int':'int' cinit
      |   `-ImplicitCastExpr 0xa40b40 <col:14> 'int' <LValueToRValue>
      |     `-UnaryOperator 0xa40ad0 <col:14> 'int' lvalue prefix '*'
      |       `-ImplicitCastExpr 0xa40ac0 <col:14> 'int *':'int *' <LValueToRValue>
      |         `-DeclRefExpr 0xa40aa8 <col:14> 'int *':'int *' lvalue Var 0xa40870 '__begin' 'int *':'int *'
      `-NullStmt 0xa40b78 <line:4:5>

There is no range init in the AST. I'm not certain if that is an issue we want to rectify in the AST or not.

40 ↗

(On Diff #37069)

I think the fixit that the C++ Core Guidelines wants is to use array_view, which isn't available to everyone and can't be applied locally (for instance, it may require changing a function parameter instead of the argument passed to the function).

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

26 ↗

(On Diff #37069)

I would like to see a test case demonstrating that this does not trigger a diagnostic when creating an array_view object. We don't need to pull in all of the array_view machinery from GSL, but it would be good to have a skeleton of the implementation to ensure we aren't diagnosing there.

[klimek]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

40 ↗

(On Diff #37069)

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds3-no-array-to-pointer-decay
says that &a[0] is an OK work around (makes the thing visible)

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

40 ↗

(On Diff #37069)

Hmm, I missed that part, but you're right. I wonder if they intended that to be used as a workaround or not, however. It's not an array decay because it's an explicit subscript of the array, and a unary op. It's certainly a functional workaround, though. I would worry about more complex cases:

size_t g();
void h(int *);

void f() {
  int a[5];
  h(a + g() - 10); // Convert to &a[g() - 10];
}

[mgehre]

Test more complex array arithmetic, test gsl::array_view works, add "use array_view instead" to diagnostic

[mgehre]

Matcher does not rely on __range anymore

[mgehre]

Updated commit to fix comments

[aaron.ballman]

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

13 ↗

(On Diff #37417)

Currently, the diag is

diag(MatchedCast->getExprLoc(), "do not (implicitly) convert an array to a pointer");

should I make the parenthesis conditional?

The core guideline only says to diagnose for implicit decay, so I think we should not diagnose in this case.

Perhaps the wording could be, "do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit decay instead"?

18 ↗

(On Diff #37417)

Formatting

41 ↗

(On Diff #37417)

Formatting (may want to just run clang-format over the file).

[mgehre]

test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

13 ↗

(On Diff #37417)

I posted the question here https://github.com/isocpp/CppCoreGuidelines/issues/352
From my understanding, explicit casts should also be forbidden for the same reasons that implicit decay was:
"Pointers are not arrays". If you need a pointer to the first element of an array, one can still use &a[0].

[mgehre]

Do not flag explicit casts

[sbenza]

Just one formatting comment.

clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

52 ↗

(On Diff #38353)

reflow to 80 cols

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
+  ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
 
   LINK_LIBS

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/AssignOperatorSignatureCheck.h"
+#include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
+        "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.h

+//===--- ProBoundsArrayToPointerDecayCheck.h - clang-tidy--------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_ARRAY_TO_POINTER_DECAY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_ARRAY_TO_POINTER_DECAY_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// This check flags all array to pointer decays
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-bounds-array-to-pointer-decay.html
+class ProBoundsArrayToPointerDecayCheck : public ClangTidyCheck {
+public:
+  ProBoundsArrayToPointerDecayCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_ARRAY_TO_POINTER_DECAY_H
+

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

+//===--- ProBoundsArrayToPointerDecayCheck.cpp - clang-tidy----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ProBoundsArrayToPointerDecayCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+AST_MATCHER_P(CXXForRangeStmt, hasRangeBeginEndStmt,
+              ast_matchers::internal::Matcher<DeclStmt>, InnerMatcher) {
+  const DeclStmt *const Stmt = Node.getBeginEndStmt();
+  return (Stmt != nullptr && InnerMatcher.matches(*Stmt, Finder, Builder));
+}
+
+AST_MATCHER(Stmt, isInsideOfRangeBeginEndStmt) {
+  return stmt(hasAncestor(cxxForRangeStmt(
+                  hasRangeBeginEndStmt(hasDescendant(equalsNode(&Node))))))
+      .matches(Node, Finder, Builder);
+}
+
+void ProBoundsArrayToPointerDecayCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // The only allowed array to pointer decay
+  // 1) just before array subscription
+  // 2) inside a range-for over an array
+  // 3) if it converts a string literal to a pointer
+  Finder->addMatcher(
+      implicitCastExpr(unless(hasParent(arraySubscriptExpr())),
+                       unless(hasParent(explicitCastExpr())),
+                       unless(isInsideOfRangeBeginEndStmt()),
+                       unless(hasSourceExpression(stringLiteral())))
+          .bind("cast"),
+      this);
+}
+
+void ProBoundsArrayToPointerDecayCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
+  if (MatchedCast->getCastKind() != CK_ArrayToPointerDecay)
+    return;
+
+  diag(MatchedCast->getExprLoc(), "do not implicitly decay an array into a "
+                                  "pointer; consider using gsl::array_view or "
+                                  "an explicit cast instead");
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-array-to-pointer-decay.rst

+cppcoreguidelines-pro-bounds-array-to-pointer-decay
+===================================================
+
+This check flags all array to pointer decays.
+
+Pointers should not be used as arrays. array_view is a bounds-checked, safe alternative to using pointers to access arrays.
+
+This rule is part of the "Bounds safety" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds3-no-array-to-pointer-decay

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 List of clang-tidy Checks
 =========================
 
 .. toctree::
    cert-setlongjmp
    cert-variadic-function-def
+   cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    google-build-explicit-make-pair
    google-build-namespaces

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-array-to-pointer-decay %t
+#include <stddef.h>
+
+namespace gsl {
+template <class T>
+class array_view {
+public:
+  template <class U, size_t N>
+  array_view(U (&arr)[N]);
+};
+}
+
+void pointerfun(int *p);
+void arrayfun(int p[]);
+void arrayviewfun(gsl::array_view<int> &p);
+size_t s();
+
+void f() {
+  int a[5];
+  pointerfun(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+  pointerfun((int *)a); // OK, explicit cast
+  arrayfun(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not implicitly decay an array into a pointer
+
+  pointerfun(a + s() - 10); // Convert to &a[g() - 10];
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not implicitly decay an array into a pointer
+
+  gsl::array_view<int> av(a);
+  arrayviewfun(av); // OK
+
+  int i = a[0];      // OK
+  pointerfun(&a[0]); // OK
+
+  for (auto &e : a) // OK, iteration internally decays array to pointer
+    e = 1;
+}
+
+const char *g() {
+  return "clang"; // OK, decay string literal to pointer
+}