Enforce SL.con.3: Add check to replace operator[] with at() [Cont.]

[paulhdk]

This PR is based on the PR #90043 by @sebwolf-de, who has given us (@leunam99 and myself) permission to continue his work.

The original PR message reads:

The string based test to find out whether the check is applicable on the class is not ideal, but I did not find a more elegant way, yet. If there is more clang-query magic available, that I'm not aware of, I'm happy to adapt that.

As part of the reviews for that PR, @sebwolf-de changed the following:

• Detect viable classes automatically instead of looking for fixed names
• Disable fix-it suggestions

This PR contains the same changes and, in addition, addresses further feedback provided by the maintainers.

Changes in addition to the original PR:

• Exclude std::map, std::flat_map, and std::unordered_map from the analysis by default, and add the ability for users to exclude additional classes from the analysis
• Add the tests @PiotrZSL requested here
• Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by @PiotrZSL here
• Add a more detailed description of what the analysis does as requested by @PiotrZSL here

We explicitly don't ignore unused code with TK_IgnoreUnlessSpelledInSource, as requested by @PiotrZSL here, because it caused the template-related tests to fail.

We are not sure what the best behaviour for templates is; should we:

• not warn if using at() will make a different instantiation not compile?
• warn at the place that requires the template instantiation?
• keep the warning and add the name of the class of the object / the template parameters that lead to the message?
• not warn in templates at all because the code is implicit?

@carlosgalvezp and @HerrCai0907 discussed the possibility of disabling the check when exceptions are disabled, but we were unsure whether they'd reached a conclusion and whether it was still relevant when fix-it suggestions are disabled.

What do you think?

[github-actions[bot]]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Paul Heidekrüger (PBHDK)

Changes

This PR is based on the PR #90043 by @sebwolf-de, who has given us (@leunam99 and myself) permission to continue his work.

The original PR message reads:

> The string based test to find out whether the check is applicable on the class is not ideal, but I did not find a more elegant way, yet. > If there is more clang-query magic available, that I'm not aware of, I'm happy to adapt that.

As part of the reviews for that PR, @sebwolf-de changed the following:

• Detect viable classes automatically instead of looking for fixed names
• Disable fix-it suggestions

This PR contains the same changes and, in addition, addresses further feedback provided by the maintainers.

Changes in addition to the original PR:

• Exclude std::map, std::flat_map, and std::unordered_set from the analysis by default, and add the ability for users to exclude additional classes from the analysis
• Add the tests @PiotrZSL requested here
• Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by @PiotrZSL here
• Add a more detailed description of what the analysis does as requested by @PiotrZSL here

We explicitly don't ignore unused code with TK_IgnoreUnlessSpelledInSource, as requested by @PiotrZSL here, because it caused the template-related tests to fail.

We are not sure what the best behaviour for templates is; should we:

• not warn if using at() will make a different instantiation not compile?
• warn at the place that requires the template instantiation?
• keep the warning and add the name of the class of the object / the template parameters that lead to the message?
• not warn in templates at all because the code is implicit?

@carlosgalvezp and @HerrCai0907 discussed the possibility of disabling the check when exceptions are disabled, but we were unsure whether they'd reached a conclusion and whether it was still relevant when fix-it suggestions are disabled.

What do you think?

---

Full diff: https://github.com/llvm/llvm-project/pull/95220.diff

8 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt (+1)
• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp (+3)
• (added) clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp (+124)
• (added) clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h (+40)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
• (added) clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst (+31)
• (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
• (added) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp (+142)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
index eb35bbc6a538f..fd436859ad04a 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -20,6 +20,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule
   NoMallocCheck.cpp
   NoSuspendWithLockCheck.cpp
   OwningMemoryCheck.cpp
+  PreferAtOverSubscriptOperatorCheck.cpp
   PreferMemberInitializerCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index e9f0201615616..565a99a865519 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -34,6 +34,7 @@
 #include "NoMallocCheck.h"
 #include "NoSuspendWithLockCheck.h"
 #include "OwningMemoryCheck.h"
+#include "PreferAtOverSubscriptOperatorCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
@@ -102,6 +103,8 @@ class CppCoreGuidelinesModule : public ClangTidyModule {
         "cppcoreguidelines-non-private-member-variables-in-classes");
     CheckFactories.registerCheck<OwningMemoryCheck>(
         "cppcoreguidelines-owning-memory");
+    CheckFactories.registerCheck<PreferAtOverSubscriptOperatorCheck>(
+        "cppcoreguidelines-prefer-at-over-subscript-operator");
     CheckFactories.registerCheck<PreferMemberInitializerCheck>(
         "cppcoreguidelines-prefer-member-initializer");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
new file mode 100644
index 0000000000000..dc036e23e2af1
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
@@ -0,0 +1,124 @@
+//===--- PreferAtOverSubscriptOperatorCheck.cpp - clang-tidy --------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PreferAtOverSubscriptOperatorCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include <algorithm>
+#include <numeric>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+static constexpr std::array<llvm::StringRef, 3> DefaultExclusions = {
+    llvm::StringRef("std::map"), llvm::StringRef("std::unordered_map"),
+    llvm::StringRef("std::flat_map")};
+
+PreferAtOverSubscriptOperatorCheck::PreferAtOverSubscriptOperatorCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {
+
+  ExcludedClasses = clang::tidy::utils::options::parseStringList(
+      Options.get("ExcludeClasses", ""));
+  ExcludedClasses.insert(ExcludedClasses.end(), DefaultExclusions.begin(),
+                         DefaultExclusions.end());
+}
+
+void PreferAtOverSubscriptOperatorCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+
+  if (ExcludedClasses.size() == DefaultExclusions.size()) {
+    Options.store(Opts, "ExcludeClasses", "");
+    return;
+  }
+
+  // Sum up the sizes of the defaults ( + semicolons), so we can remove them
+  // from the saved options
+  size_t DefaultsStringLength =
+      std::transform_reduce(DefaultExclusions.begin(), DefaultExclusions.end(),
+                            DefaultExclusions.size(), std::plus<>(),
+                            [](llvm::StringRef Name) { return Name.size(); });
+
+  std::string Serialized =
+      clang::tidy::utils::options::serializeStringList(ExcludedClasses);
+
+  Options.store(Opts, "ExcludeClasses",
+                Serialized.substr(0, Serialized.size() - DefaultsStringLength));
+}
+
+const CXXMethodDecl *findAlternative(const CXXRecordDecl *MatchedParent,
+                                     const CXXMethodDecl *MatchedOperator) {
+  for (const CXXMethodDecl *Method : MatchedParent->methods()) {
+    const bool CorrectName = Method->getNameInfo().getAsString() == "at";
+    if (!CorrectName)
+      continue;
+
+    const bool SameReturnType =
+        Method->getReturnType() == MatchedOperator->getReturnType();
+    if (!SameReturnType)
+      continue;
+
+    const bool SameNumberOfArguments =
+        Method->getNumParams() == MatchedOperator->getNumParams();
+    if (!SameNumberOfArguments)
+      continue;
+
+    for (unsigned ArgInd = 0; ArgInd < Method->getNumParams(); ArgInd++) {
+      const bool SameArgType =
+          Method->parameters()[ArgInd]->getOriginalType() ==
+          MatchedOperator->parameters()[ArgInd]->getOriginalType();
+      if (!SameArgType)
+        continue;
+    }
+
+    return Method;
+  }
+  return static_cast<CXXMethodDecl *>(nullptr);
+}
+
+void PreferAtOverSubscriptOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  // Need a callExpr here to match CXXOperatorCallExpr ``(&a)->operator[](0)``
+  // and CXXMemberCallExpr ``a[0]``.
+  Finder->addMatcher(
+      callExpr(
+          callee(
+              cxxMethodDecl(hasOverloadedOperatorName("[]")).bind("operator")),
+          callee(cxxMethodDecl(hasParent(
+              cxxRecordDecl(hasMethod(hasName("at"))).bind("parent")))))
+          .bind("caller"),
+      this);
+}
+
+void PreferAtOverSubscriptOperatorCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const CallExpr *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("caller");
+  const CXXMethodDecl *MatchedOperator =
+      Result.Nodes.getNodeAs<CXXMethodDecl>("operator");
+  const CXXRecordDecl *MatchedParent =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("parent");
+
+  std::string ClassIdentifier = MatchedParent->getQualifiedNameAsString();
+
+  if (std::find(ExcludedClasses.begin(), ExcludedClasses.end(),
+                ClassIdentifier) != ExcludedClasses.end())
+    return;
+
+  const CXXMethodDecl *Alternative =
+      findAlternative(MatchedParent, MatchedOperator);
+  if (!Alternative)
+    return;
+
+  diag(MatchedExpr->getBeginLoc(),
+       "found possibly unsafe operator[], consider using at() instead");
+  diag(Alternative->getBeginLoc(), "alternative at() defined here",
+       DiagnosticIDs::Note);
+}
+
+} // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
new file mode 100644
index 0000000000000..f2450a7ab3470
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
@@ -0,0 +1,40 @@
+//===--- PreferAtOverSubscriptOperatorCheck.h - clang-tidy ------*- C++ -*-===//
+//===--- PreferMemberInitializerCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Enforce CPP core guidelines SL.con.3
+///
+/// See
+/// https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.html
+class PreferAtOverSubscriptOperatorCheck : public ClangTidyCheck {
+public:
+  PreferAtOverSubscriptOperatorCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  // A list of class names that are excluded from the warning
+  std::vector<llvm::StringRef> ExcludedClasses;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6bf70c5cf4f8a..c88e59e453265 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -160,6 +160,11 @@ New checks
   Replaces nested ``std::min`` and ``std::max`` calls with an initializer list
   where applicable.
 
+- New :doc:`cppcoreguidelines-prefer-at-over-subscript-operator
+  <clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator>` check.
+
+  Flags the unsafe ``operator[]`` and suggests replacing it with ``at()``.
+
 - New :doc:`modernize-use-designated-initializers
   <clang-tidy/checks/modernize/use-designated-initializers>` check.
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
new file mode 100644
index 0000000000000..42a2100f32582
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - cppcoreguidelines-prefer-at-over-subscript-operator
+
+cppcoreguidelines-prefer-at-over-subscript-operator
+=====================================
+
+This check flags all uses of ``operator[]`` where an equivalent (same parameter and return types) ``at()`` method exists and suggest using that instead.
+
+For example the code
+
+.. code-block:: c++
+  std::array<int, 3> a;
+  int b = a[4];
+
+will generate a warning but 
+
+.. code-block:: c++
+  std::unique_ptr<int> a;
+  int b = a[0];
+
+will not.
+
+The classes ``std::map``, ``std::unordered_map`` and ``std::flat_map`` are excluded from this check, because for them the subscript operator has a defined behaviour when a key does not exist (inserting a new element).
+
+Options
+-------
+
+.. option:: ExcludeClasses
+
+    Semicolon-delimited list of class names that should additionally be excluded from this check. By default empty. 
+
+This check enforces part of the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index a698cecc0825c..d83dad31b3547 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -189,6 +189,7 @@ Clang-Tidy Checks
    :doc:`cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc>`,
    :doc:`cppcoreguidelines-no-suspend-with-lock <cppcoreguidelines/no-suspend-with-lock>`,
    :doc:`cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory>`,
+   :doc:`cppcoreguidelines-prefer-at-over-subscript-operator <cppcoreguidelines/prefer-at-over-subscript-operator>`,
    :doc:`cppcoreguidelines-prefer-member-initializer <cppcoreguidelines/prefer-member-initializer>`, "Yes"
    :doc:`cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines/pro-bounds-array-to-pointer-decay>`,
    :doc:`cppcoreguidelines-pro-bounds-constant-array-index <cppcoreguidelines/pro-bounds-constant-array-index>`, "Yes"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp
new file mode 100644
index 0000000000000..cc7088bffeda9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp
@@ -0,0 +1,142 @@
+namespace std {
+  template<typename T, unsigned size>
+  struct array {
+    T operator[](unsigned i) {
+      return T{1};
+    }
+    T at(unsigned i) {
+      return T{1};
+    }
+  };
+
+  template<typename T, typename V>
+  struct map {
+    T operator[](unsigned i) {
+      return T{1};
+    }
+    T at(unsigned i) {
+      return T{1};
+    }
+  };
+
+  template<typename T>
+  struct unique_ptr {
+    T operator[](unsigned i) {
+      return T{1};
+    }
+  };
+
+  template<typename T>
+  struct span {
+    T operator[](unsigned i) {
+      return T{1};
+    }
+  };
+} // namespace std
+
+namespace json {
+  template<typename T>
+  struct node{
+    T operator[](unsigned i) {
+      return T{1};
+    }
+  };
+} // namespace json
+
+struct SubClass : std::array<int, 3> {};
+
+class ExcludedClass1 {
+  public:
+    int operator[](unsigned i) {
+      return 1;
+    }
+    int at(unsigned i) {
+      return 1;
+    }
+};
+
+class ExcludedClass2 {
+  public:
+    int operator[](unsigned i) {
+      return 1;
+    }
+    int at(unsigned i) {
+      return 1;
+    }
+};
+
+
+// RUN: %check_clang_tidy %s \
+// RUN: cppcoreguidelines-prefer-at-over-subscript-operator %t -- \
+// RUN: -config='{CheckOptions: {cppcoreguidelines-prefer-at-over-subscript-operator.ExcludeClasses: "ExcludedClass1;ExcludedClass2"}}'
+std::array<int, 3> a;
+
+auto b = a[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto c = a[1+1];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+constexpr int Index = 1;
+auto d = a[Index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+int e(int Ind) {
+  return a[Ind];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+}
+
+auto f = (&a)->operator[](1);
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto g = a.at(0);
+
+std::unique_ptr<int> p;
+auto q = p[0];
+
+std::span<int> s;
+auto t = s[0];
+
+json::node<int> n;
+auto m = n[0];
+
+SubClass Sub;
+auto r = Sub[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+typedef std::array<int, 3> ar;
+ar BehindDef;
+auto u = BehindDef[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+template<typename T> int TestTemplate(T t){
+  return t[0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+}
+
+auto v = TestTemplate<>(a);
+auto w = TestTemplate<>(p);
+
+//explicitely excluded classes / struct / template
+ExcludedClass1 E1;
+auto x1 = E1[0];
+
+ExcludedClass2 E2;
+auto x2 = E1[0];
+
+std::map<int,int> TestMap;
+auto y = TestMap[0];
+
+#define SUBSCRIPT_BEHIND_MARCO(x) a[x]
+#define ARG_BEHIND_MACRO 0
+#define OBJECT_BEHIND_MACRO a
+
+auto m1 = SUBSCRIPT_BEHIND_MARCO(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto m2 = a[ARG_BEHIND_MACRO];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto m3 = OBJECT_BEHIND_MACRO[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]

[carlosgalvezp]

Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by @PiotrZSL

I'm strongly opposed to this, because it's conflating "how to solve the problem" with "what the problem is".

If we want to focus on the problem, the check could be named "AvoidSubscriptOperator". This way, the solution to the problem is open for users to decide. I still think "AvoidBoundErrors" is the best name since it maps exactly to what the guidelines call it.

discussed the possibility of disabling the check when exceptions are disabled,

I don't think this is a good idea, as it introduces a dependency towards Clang compiler. And it's a lot more complicated than what I'm proposing.

I would appreciate more elaboration as to why this check must use at() and cannot be made optional.

[leunam99]

I still think "AvoidBoundErrors" is the best name since it maps exactly to what the guidelines call it.

For me, this sounds to general for what the check currently does. The Enforcement section at the end of the rule SL.con.3 states:

Issue a diagnostic for any call to a standard-library function that is not bounds-checked.

The guidelines do not provide a list of such functions and we are not familliar enough with the standard library to create a comprehensive list ourself, so at this time, we are unable to implement the full rule.

[carlosgalvezp]

The check certainly doesn't need to fully implement the guideline right now, it can be done in small steps iteratively. I envision that over time the check will fully implement the rule as much as possible.

We typically want a 1:1 mapping between a check and a rule. In other words, it would not be good to have N checks implementing different aspects of SL.3, it should be only 1 check.

The reason I'm being picky with the name is that it's costly to change it later, it takes 2 clang tidy releases i.e 1 year.

[HerrCai0907]

discussed the possibility of disabling the check when exceptions are disabled,

I don't think this is a good idea, as it introduces a dependency towards Clang compiler. And it's a lot more complicated than what I'm proposing.

exception information is in LangOptions. It should be the same as c++ version.

[HerrCai0907]

Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by @PiotrZSL

I'm strongly opposed to this, because it's conflating "how to solve the problem" with "what the problem is".

If we want to focus on the problem, the check could be named "AvoidSubscriptOperator". This way, the solution to the problem is open for users to decide. I still think "AvoidBoundErrors" is the best name since it maps exactly to what the guidelines call it.

I also agree AvoidBoundErrors is a better name since it can keep consistency with original guidelines.

[carlosgalvezp]

exception information is in LangOptions. It should be the same as c++ version.

I see, that's good. Worth mentioning is that just because "-fno-except" is not present, it does not mean that a project bans exceptions. For example libstdc++/libc++ is typically compiled with exceptions; this won't play well with client code compiled with -fno-except.

Even if exceptions were allowed, one can also argue if out-of-bounds is a place that grants usage of exceptions. In some views, exceptions are meant only for truly exceptional situations (e.g. out-of-memory), whereas out-of-bounds is generally seen as a programmer error (contract violation).

I don't want to dive deep into such discussion, my point is that error handling is a very debatable topic and also a core design choice to any C++ project, and as such I don't think clang-tidy shouldn't be taking a stance on it. Otherwise some people will be forced to disable a great check (detects violations) due to unwanted fix-it/suggestions (which junior devs may blindly follow without full context).

I think clang-tidy could offer an enum option for the fix, like:

FixMode: None        // no fix (default)
FixMode: at             // use <container>::at()
FixMode: function   // use a user-defined function, for example gsl::at(), as shown in the C++ Core Guidelines

[carlosgalvezp]

I specifically added container to both names because the check is in SL.con

Sounds good to me! My main goal is not constraining the check to only operator[]; in the future someone might want to also implement functionality to detect e.g. std::memset as you mentioned, and in that case it would be awkward to create a new check even though it covers the same rule.

Btw, I realize that this check is part of the bounds profile (Bounds.4), so for consistency it should probably be named cppcoreguidelines-pro-bounds-.... This becomes then the last remaining check to complete the bounds profile!

[paulhdk]

Btw, I realize that this check is part of the bounds profile (Bounds.4), so for consistency it should probably be named cppcoreguidelines-pro-bounds-....

What do you think of: cppcoreguidelines-pro-bounds-avoid-unchecked-container-accesses? That's just me attempting to capture all of the previous suggestions in one name.

This becomes then the last remaining check to complete the bounds profile!

That was actually @leunam99's and my main our motivation for getting involved with this work 🙂

[carlosgalvezp]

cppcoreguidelines-pro-bounds-avoid-unchecked-container-accesses

LGTM!

[paulhdk]

Btw, I realize that this check is part of the bounds profile (Bounds.4), so for consistency it should probably be named cppcoreguidelines-pro-bounds-.... This becomes then the last remaining check to complete the bounds profile!

Does the fact that this check implements the bounds profile need to be mentioned anywhere else? Or is it enough to have it be implicit, e.g., via the name.

[carlosgalvezp]

Does the fact that this check implements the bounds profile need to be mentioned anywhere else? Or is it enough to have it be implicit, e.g., via the name.

Not that I'm aware of. It could be worth mentioning in the check documentation though, see for example existing checks: https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-array-to-pointer-decay.html

[paulhdk]

The name sounds nice. Thanks. Please address the concerns here and in #90043 to always suggest at as well. An option to enable suggesting and providing a fix to use at (or gsl::at), like others have suggested, sounds like the best option.

Thank you for taking another look as well as your comments. On it!

[carlosgalvezp]

Nit: consider updating also the commit message / title of this PR to reflect the new behavior of this check (namely, to not enforce at()).

[carlosgalvezp]

It seems that this check is very similar up the cppcoreguidelines-pro-bounds-constant-array-index check, Is there not a way this could be merged with that check. I'm mindful that people will have both checks enabled and get 2 different warnings for the same violation

Since it's 2 different rules from the guidelines I think it's better to have them as separate checks to have a 1:1 mapping. People can easily suppress both of they want via NOLINT(cppcoreguidelines-probounds-* if needed.

Otherwise the check will do "too much" and there will come a day where someone will want to split the check into two as it has happened in the past.

[leunam99]

We now added FIXITs with the different modes as suggested. Some notes about the implementation:

• In the None / Function mode, we thought that checking if an at method exists does not make sense anymore and warn on every subscript operator use (besides the explicit exclusions) Then, it might be unintuitive that changing a setting called FIXMODE affects if a warning is emmitted, so we emit this general warning in any mode if we do not have a concrete alternative. Do you agree with this behaviour? Or will this result in too many false positives?
• The Function mode does not yet check if a valid version of the function exists. Is there a simple way to check if a function template (or function) exists that can fit some specific parameters? Should/Can we do something reasonable if the function is not imported?
• We prefixed the option names with 'Subscript' so that if the check later will be extended, the naming does not get confusing.
• For Templates the fix does not work. We will handle this after it is more clear how Templates should be treated.

[paulhdk]

Hi all,

just giving this a polite bump. Please do let us know if you have any questions or if there's anything we can do to make reviewing easier.

[EugeneZelenko]

Please rebase from main.

[paulhdk]

Sorry for letting this sit for long!

I've addressed the most recent comments.

Based on what @leunam99 wrote above, the following questions are still unresolved:

• It is still unclear to us how templates should be addressed when suggesting fixes.
 For instance, what should happen in this case:
 https://github.com/llvm/llvm-project/blob/98483ae1581c9a12fc7b4c8b5b64330db8292c29/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.cpp?plain=1#L176-L184
• Should we worry about the cases where the subscript operator can have 0 parameters or more than 1 parameter in C++23? At the moment we’re accounting for the case where there is no parameter, but don't explicitly handle multiple parameters.
• As @carlosgalvezp noted, there are still open comments. I’ve resolved them or responded to those that we’re uncertain about. @PiotrZSL, it would be great if you could have another look!

Sorry again, for taking so long.

[paulhdk]

Gentle bump. Thank you for taking a look!

[paulhdk]

Hi all!

I've requested reviews from everyone who has reviewed the check so far. Would be awesome if we could get this merged soon! Attaching my previous comments with unresolved questions below.

Based on what @leunam99 wrote above, the following questions are still unresolved:

• It is still unclear to us how templates should be addressed when suggesting fixes.
 For instance, what should happen in this case:
 https://github.com/llvm/llvm-project/blob/98483ae1581c9a12fc7b4c8b5b64330db8292c29/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.cpp?plain=1#L176-L184

• Should we worry about the cases where the subscript operator can have 0 parameters or more than 1 parameter in C++23? At the moment we’re accounting for the case where there is no parameter, but don't explicitly handle multiple parameters.

• As @carlosgalvezp noted, there are still open comments. I’ve resolved them or responded to those that we’re uncertain about. @PiotrZSL, it would be great if you could have another look!

Thank you!

[paulhdk]

LGTM for my changes, here a few nits that are just nice to have (doesn't much block PR). The only important change is single ticks instead of double tick in default value of ExcludeClasses option.

P.S. Commented without official approval because I didn't have time to thoughtfully read all matchers and check code. Hope to do it in the future if there wouldn't be any feedback from previous reviewers.

Thank you for your quick reply and feedback, @vbvictor! I've incorporated all of the requested changes.

[paulhdk]

Polite bump.

Hi all!

I've requested reviews from everyone who has reviewed the check so far. Would be awesome if we could get this merged soon! Attaching my previous comments with unresolved questions below.

Based on what @leunam99 wrote above, the following questions are still unresolved:

• It is still unclear to us how templates should be addressed when suggesting fixes.
> For instance, what should happen in this case:
> https://github.com/llvm/llvm-project/blob/98483ae1581c9a12fc7b4c8b5b64330db8292c29/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.cpp?plain=1#L176-L184
• Should we worry about the cases where the subscript operator can have 0 parameters or more than 1 parameter in C++23? At the moment we’re accounting for the case where there is no parameter, but don't explicitly handle multiple parameters.
• As @carlosgalvezp noted, there are still open comments. I’ve resolved them or responded to those that we’re uncertain about. @PiotrZSL, it would be great if you could have another look!

Thank you!

[paulhdk]

I've marked all the conversations as resolved. Please do reopen anything that you feel still needs addressing.

@vbvictor would this be good to go from your side?

Thank you!

[paulhdk]

The rebase caused the "CHECK-FIXES-FUNC" tests to break. Will have to do some digging. Will try to fix ASAP.

[paulhdk]

Tests are fixed for me locally. PTAL. Thank you!

[paulhdk]

@vbvictor the Linux CI checks failed, but the tests passed. I'm unsure what the issue is here. Do you have a better insight here?

[vbvictor]

the Linux CI checks failed, but the tests passed.

I've seen this happen before, seems like a CI issue, not the check issue. I think in a week or so it would be fixed.

would this be good to go from your side?

LGTM for my changes and a brief look at check code, sorry I don't have time to review it properly for now.

Ping @HerrCai0907, @carlosgalvezp, @PiotrZSL, @5chmidti. I hope we could deliver this check before 21th release.

[paulhdk]

@vbvictor, it's been roughly two weeks now; shall we give the CI another go?