Skip to content

Conversation

paulhdk
Copy link
Contributor

@paulhdk paulhdk commented Jun 12, 2024

This PR is based on the PR #90043 by Sebastian Wolf who has given us (Manuel 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, Sebastian 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 Piotr Zegar requested here
  • Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by Piotr here
  • Add a more detailed description of what the analysis does as requested by Piotr here

We explicitly don't ignore unused code with TK_IgnoreUnlessSpelledInSource, as requested by Piotr 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?

Carlos Galvez and Congcong Cai 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?

Co-authored-by: Manuel Pietsch manuelpietsch@outlook.de
Co-authored-by: Sebastian Wolf wolf.sebastian@in.tum.de

Copy link

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
Copy link
Member

llvmbot commented Jun 12, 2024

@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
Copy link
Contributor

carlosgalvezp commented Jun 12, 2024

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

HerrCai0907 commented Jun 12, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't think the check should propose using at(). The rule suggests using span, at(), range-for, begin(),end() or algorithms instead of accessing elements in a container with an unchecked-bounds function.

@carlosgalvezp with AvoidSubscriptOperator we constrain the check to be about subscript access only, which, admittedly, the check currently is. However, the rule is not constrained to subscript access and the rule has an example with memset which does not use the subscript operator.
AvoidBoundErrors sounds a little bit too broad IMO, because the rule talks about containers and is in the SL containers section.
What about: cppcoreguidelines-(avoid-)container-unchecked-bounds(-access) or
cppcoreguidelines-(avoid-)container-bounds-errors?
I specifically added container to both names because the check is in SL.con.


We are not sure what the best behaviour for templates is

keep the warning and add the name of the class of the object / the template parameters that lead to the message

This may result in a lot of warnings because every diagnostic to the same location will no longer be combined like it is now, and instead we get N diagnostics (given N instantiations). On the other hand, these warnings would provide context on the type being used, so I think it is better to have that instead of fewer warnings but without the type named.

We could ignore instantiations and diagnose any use of x[] inside template declarations. We can then check, if possible, if the type of x is known to be excluded (e.g., normal type if it is not a dependent type, or TemplateSpecializationType).


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

+1
That would be a good way to accommodate projects that prefer at(), because it requires the project to choose to go with this way of solving it (off-by-default).


disabling the check when exceptions are disabled

That would only apply if the check (is enabled to) proposes to use at().

@carlosgalvezp
Copy link
Contributor

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
Copy link
Contributor Author

paulhdk commented Jun 17, 2024

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
Copy link
Contributor

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

LGTM!

@paulhdk
Copy link
Contributor Author

paulhdk commented Jun 18, 2024

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
Copy link
Contributor

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

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paulhdk
Copy link
Contributor Author

paulhdk commented Jun 26, 2024

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
Copy link
Contributor

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

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I like check name .

As for changes:

  • configuration need a little bit work
  • finding .at may need some work (better would be to have this as matcher)
  • documentation need minor touch

Overall not bad.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main issue with check is:

  • configuration
  • few nits

Copy link
Member

@njames93 njames93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@paulhdk paulhdk force-pushed the prefer-at-over-subscript-operator branch from 1d87cd0 to 743b42b Compare August 4, 2025 08:27
@paulhdk
Copy link
Contributor Author

paulhdk commented Aug 4, 2025

@5chmidti @vbvictor anything you want me to do so we can move forward? I believe I've addressed all the feedback you gave so far and I just rebased. Thank you!

@vbvictor
Copy link
Contributor

vbvictor commented Aug 4, 2025

I'm not sure if I'll get to chose the commit message. If no, please don't forget to add @leunam99 and @sebwolf-de as co-authors alongside myself.

By default, the PR description in GitHub will be the commit message. You can add https://github.blog/news-insights/product-news/commit-together-with-co-authors/ at the end of description. Please use emails, not GitHub names.
Also, we need to clean the commit message from GitHub user tags because those people will receive spam messages from GitHub once the commit lands in main.

…-bounds-avoid-unchecked-container-access.rst

Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'd give @5chmidti some time for final merge as the main reviewer.

…-bounds-avoid-unchecked-container-access.rst

Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
@vbvictor
Copy link
Contributor

vbvictor commented Aug 4, 2025

Last part, could you please make your email public.
https://llvm.org/docs/DeveloperPolicy.html#email-addresses

@paulhdk
Copy link
Contributor Author

paulhdk commented Aug 8, 2025

Last part, could you please make your email public. https://llvm.org/docs/DeveloperPolicy.html#email-addresses

Sorry. Done!

…-bounds-avoid-unchecked-container-access.rst

Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
@vbvictor vbvictor changed the title Enforce SL.con.3: Add check to replace operator[] with at() [Cont.] [clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3 Aug 24, 2025
@vbvictor vbvictor merged commit bc278d0 into llvm:main Aug 24, 2025
10 checks passed
Copy link

@paulhdk Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@carlosgalvezp
Copy link
Contributor

[clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3

Nit: this is not what the check does - it only reports usages of unchecked STL APIs. The user decides (via config options) if it should be replaced with at() or something else.

@paulhdk
Copy link
Contributor Author

paulhdk commented Sep 1, 2025

[clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3

Nit: this is not what the check does - it only reports usages of unchecked STL APIs. The user decides (via config options) if it should be replaced with at() or something else.

@carlosgalvezp: what's the process for updating the commit message?

@vbvictor
Copy link
Contributor

vbvictor commented Sep 1, 2025

[clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3

Nit: this is not what the check does - it only reports usages of unchecked STL APIs. The user decides (via config options) if it should be replaced with at() or something else.

@carlosgalvezp: what's the process for updating the commit message?

We can't alter the commit message after it was merged in main but we can (and should) pay more attention to it in the future

@carlosgalvezp
Copy link
Contributor

Yeah we can't change it now, it's ok :)

@paulhdk
Copy link
Contributor Author

paulhdk commented Sep 4, 2025

We can't alter the commit message after it was merged in main but we can (and should) pay more attention to it in the future

Right. Lesson learned!

Also, I wanted to thank you both @carlosgalvezp and @vbvictor as well as @EugeneZelenko, @5chmidti, and everyone else for getting this PR across the finish line!
My first LLVM contribution - very happy about that!

@carlosgalvezp
Copy link
Contributor

Thank you for the hard work and patience during the code review process! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.