[clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

[carlosgalvezp]

Summary

Currently the fix hint is hardcoded to gsl::at(). This poses
a problem for people who, for a number of reasons, don't want
or cannot use the GSL library.

In these situations, the fix hint does more harm than good
as it creates confusion as to what the fix should be. People
can even misinterpret the fix "gsl::at" as e.g. "std::array::at",
which can lead to even more trouble (e.g. when having guidelines
that disallow exceptions).

Therefore, introduce a new option FixHint that allows users
to specify exactly what the fix hint should be for their
use case. The default is kept to gsl::at.

Since the users can write anything in this option, clang-tidy
should not add fix-it hints in this case, as they could
lead to invalid code. For example, a reasonable fix hint
like "MyCustomSafeArrayWrapper::at()" would not be possible
to replace directly in code, yet it would be of great help
pointing to users in the right direction.

[carlosgalvezp]

Update release notes.

[carlosgalvezp]

Fix doc.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

156

Please sort checks in this section alphabetically.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst

30

Please use single back-ticks for gsl::at() (option value).

[carlosgalvezp]

Single backticks for gsl::at

clang-tools-extra/docs/ReleaseNotes.rst

156

Hmm I'm not sure what you mean, should these bullets be ordered alphabetically? If so, why is "Fixed" showing up after "Updated"?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

156

Now cppcoreguidelines-pro-bounds-constant-array-index is before cppcoreguidelines-explicit-virtual-functions.

[carlosgalvezp]

Fix check order

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

156

Ah, of course. Fixed!

[njames93]

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

26

Should elide the braces here for the option and insert them at the warning stage.

79–85

84

This restriction on only offering a fix if using gsl::at seem a little too restrictive, especially when you factor that the GslHeader is defaulted to an empty string and that is already required for a fix to be emitted.

[carlosgalvezp]

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

84

What would this change for the user? Current behavior is kept, and we can't really predict a good fix if the hint is user-provided. There's many other ways to perform safe indexing than using a free standing function gsl::at-like, and those will require a larger, non-automated refactor.

Another alternative I had in mind is to completely remove the "use gsl::at() instead" hint from the message, and simply warn about the problem (unsafe indexing). CppCoreGuidelines don't actually mention that you need to use gsl::at, at least not from what I can see in the link provided in the docs.

Removing the fix hint from the warning would solve my concerns (confusing information), and we don't need to introduce extra configuration options.

What do you think?

[carlosgalvezp]

I believe this other solution has lower complexity and still achieves my goal, let me know which one you prefer!
https://reviews.llvm.org/D117857

[carlosgalvezp]

Abandoned in favor of https://reviews.llvm.org/D117857

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

 /// This checks that all array subscriptions on static arrays and std::arrays
 /// have a constant index and are within bounds
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.html
 class ProBoundsConstantArrayIndexCheck : public ClangTidyCheck {
   const std::string GslHeader;
   utils::IncludeInserter Inserter;
+  const std::string FixHint;
 
 public:
   ProBoundsConstantArrayIndexCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;

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

 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 ProBoundsConstantArrayIndexCheck::ProBoundsConstantArrayIndexCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), GslHeader(Options.get("GslHeader", "")),
       Inserter(Options.getLocalOrGlobal("IncludeStyle",
-                                        utils::IncludeSorter::IS_LLVM)) {}
+                                        utils::IncludeSorter::IS_LLVM)),
+      FixHint(Options.get("FixHint", "gsl::at()")) {}
 
 void ProBoundsConstantArrayIndexCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "GslHeader", GslHeader);
   Options.store(Opts, "IncludeStyle", Inserter.getStyle());
+  Options.store(Opts, "FixHint", FixHint);
 }
 
 void ProBoundsConstantArrayIndexCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   Inserter.registerPreprocessor(PP);
 }
 
 void ProBoundsConstantArrayIndexCheck::registerMatchers(MatchFinder *Finder) {
if (!Index) {
     SourceRange BaseRange;
     if (const auto *ArraySubscriptE = dyn_cast<ArraySubscriptExpr>(Matched))
       BaseRange = ArraySubscriptE->getBase()->getSourceRange();
     else
       BaseRange =
           dyn_cast<CXXOperatorCallExpr>(Matched)->getArg(0)->getSourceRange();
     SourceRange IndexRange = IndexExpr->getSourceRange();
 
     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()) {
+                     std::string("do not use array subscript when the index is "
+                                 "not an integer constant expression; use ") +
+                         FixHint + " instead");
+
+    if (!GslHeader.empty() && (FixHint == "gsl::at()")) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
            << FixItHint::CreateReplacement(
                   SourceRange(BaseRange.getEnd().getLocWithOffset(1),
                               IndexRange.getBegin().getLocWithOffset(-1)),
                   ", ")
            << FixItHint::CreateReplacement(Matched->getEndLoc(), ")")
            << Inserter.createMainFileIncludeInsertion(GslHeader);
     }
     return;

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-flp37-c
   <clang-tidy/checks/cert-flp37-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Add new option ``cppcoreguidelines-pro-bounds-constant-array-index.FixHint``
+  to allow users to specify a different fix hint than ``gsl::at``.
+
 - Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst

.. option:: GslHeader
 
    The check can generate fixes after this option has been set to the name of
    the include file that contains ``gsl::at()``, e.g. `"gsl/gsl.h"`.
 
 .. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.
+
+.. option:: FixHint
+
+   A string specifying which hint to suggest as a fix, in case users do not want
+   or cannot use GSL. Default is `gsl::at()`. If a non-default fix hint is
+   used, the check will not output fix-it hints, since they could potentially
+   lead to invalid code if the `FixHint` option is not a direct or valid
+   replacement. For example, passing `"MyCustomSafeArrayWrapper::at()"` would be
+   helpful and understandable by the users, but not automatable as a fix-it.

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

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-constant-array-index %t -- \
+// RUN:     -config='{CheckOptions: [{key: cppcoreguidelines-pro-bounds-constant-array-index.FixHint, value: "foo::at()"}]}'
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename T, size_t N>
+struct array {
+  T &operator[](size_t n);
+  T &at(size_t n);
+};
+} // namespace std
+
+namespace gsl {
+template <class T, size_t N>
+T &at(T (&a)[N], size_t index);
+
+template <class T, size_t N>
+T &at(std::array<T, N> &a, size_t index);
+} // namespace gsl
+
+constexpr int const_index(int base) {
+  return base + 3;
+}
+
+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; use foo::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
+  // CHECK-FIXES-NOT: gsl::at(a,  pos / 2 /*comment*/) = 1;
+  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; use foo::at() instead
+  // CHECK-FIXES-NOT: int j = gsl::at(a, pos - 1);
+
+  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
+}
+
+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; use foo::at() instead
+    // CHECK-FIXES-NOT: gsl::at(a, i) = i;
+    gsl::at(a, i) = i + 1; // 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
+}
+
+struct S {
+  int &operator[](int i);
+};
+
+void customOperator() {
+  S s;
+  int i = 0;
+  s[i] = 3; // OK, custom operator
+}