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

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
- Rename the analysis from AvoidBoundsErrorsCheck to
PreferAtOverSubscriptOperatorCheck as requested by Piotr
- Add a more detailed description of what the analysis does as requested
by Piotr

We explicitly don't ignore unused code with
`TK_IgnoreUnlessSpelledInSource`, as requested by Piotr,
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 <[email protected]>
Co-authored-by: Sebastian Wolf <[email protected]>

---------

Co-authored-by: EugeneZelenko <[email protected]>
Co-authored-by: Baranov Victor <[email protected]>

Author: 3 people

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

@@ -21,6 +21,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule STATIC
   OwningMemoryCheck.cpp
   PreferMemberInitializerCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
+  ProBoundsAvoidUncheckedContainerAccess.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp

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

@@ -36,6 +36,7 @@
 #include "OwningMemoryCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
+#include "ProBoundsAvoidUncheckedContainerAccess.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
@@ -107,6 +108,8 @@ class CppCoreGuidelinesModule : public ClangTidyModule {
         "cppcoreguidelines-prefer-member-initializer");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
+    CheckFactories.registerCheck<ProBoundsAvoidUncheckedContainerAccess>(
+        "cppcoreguidelines-pro-bounds-avoid-unchecked-container-access");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(

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

@@ -0,0 +1,262 @@
+//===--- ProBoundsAvoidUncheckedContainerAccess.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 "ProBoundsAvoidUncheckedContainerAccess.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+static constexpr llvm::StringRef DefaultExclusionStr =
+    "::std::map;::std::unordered_map;::std::flat_map";
+
+ProBoundsAvoidUncheckedContainerAccess::ProBoundsAvoidUncheckedContainerAccess(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      ExcludedClasses(utils::options::parseStringList(
+          Options.get("ExcludeClasses", DefaultExclusionStr))),
+      FixMode(Options.get("FixMode", None)),
+      FixFunction(Options.get("FixFunction", "gsl::at")),
+      FixFunctionEmptyArgs(Options.get("FixFunctionEmptyArgs", FixFunction)) {}
+
+void ProBoundsAvoidUncheckedContainerAccess::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ExcludeClasses",
+                utils::options::serializeStringList(ExcludedClasses));
+  Options.store(Opts, "FixMode", FixMode);
+  Options.store(Opts, "FixFunction", FixFunction);
+  Options.store(Opts, "FixFunctionEmptyArgs", FixFunctionEmptyArgs);
+}
+
+// TODO: if at() is defined in another class in the class hierarchy of the class
+// that defines the operator[] we matched on, findAlternative() will not detect
+// it.
+static const CXXMethodDecl *
+findAlternativeAt(const CXXMethodDecl *MatchedOperator) {
+  const CXXRecordDecl *Parent = MatchedOperator->getParent();
+  const QualType SubscriptThisObjType =
+      MatchedOperator->getFunctionObjectParameterReferenceType();
+
+  for (const CXXMethodDecl *Method : Parent->methods()) {
+    // Require 'Method' to be as accessible as 'MatchedOperator' or more
+    if (MatchedOperator->getAccess() < Method->getAccess())
+      continue;
+
+    if (MatchedOperator->isConst() != Method->isConst())
+      continue;
+
+    const QualType AtThisObjType =
+        Method->getFunctionObjectParameterReferenceType();
+    if (SubscriptThisObjType != AtThisObjType)
+      continue;
+
+    if (!Method->getNameInfo().getName().isIdentifier() ||
+        Method->getName() != "at")
+      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 nullptr;
+}
+
+void ProBoundsAvoidUncheckedContainerAccess::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(
+      mapAnyOf(cxxOperatorCallExpr, cxxMemberCallExpr)
+          .with(callee(
+              cxxMethodDecl(
+                  hasOverloadedOperatorName("[]"),
+                  anyOf(parameterCountIs(0), parameterCountIs(1)),
+                  unless(matchers::matchesAnyListedName(ExcludedClasses)))
+                  .bind("operator")))
+          .bind("caller"),
+      this);
+}
+
+void ProBoundsAvoidUncheckedContainerAccess::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("caller");
+
+  if (FixMode == None) {
+    diag(MatchedExpr->getCallee()->getBeginLoc(),
+         "possibly unsafe 'operator[]', consider bounds-safe alternatives")
+        << MatchedExpr->getCallee()->getSourceRange();
+    return;
+  }
+
+  if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(MatchedExpr)) {
+    // Case: a[i]
+    const auto LeftBracket = SourceRange(OCE->getCallee()->getBeginLoc(),
+                                         OCE->getCallee()->getBeginLoc());
+    const auto RightBracket =
+        SourceRange(OCE->getOperatorLoc(), OCE->getOperatorLoc());
+
+    if (FixMode == At) {
+      // Case: a[i] => a.at(i)
+      const auto *MatchedOperator =
+          Result.Nodes.getNodeAs<CXXMethodDecl>("operator");
+      const CXXMethodDecl *Alternative = findAlternativeAt(MatchedOperator);
+
+      if (!Alternative) {
+        diag(MatchedExpr->getCallee()->getBeginLoc(),
+             "possibly unsafe 'operator[]', consider "
+             "bounds-safe alternatives")
+            << MatchedExpr->getCallee()->getSourceRange();
+        return;
+      }
+
+      diag(MatchedExpr->getCallee()->getBeginLoc(),
+           "possibly unsafe 'operator[]', consider "
+           "bounds-safe alternative 'at()'")
+          << MatchedExpr->getCallee()->getSourceRange()
+          << FixItHint::CreateReplacement(LeftBracket, ".at(")
+          << FixItHint::CreateReplacement(RightBracket, ")");
+
+      diag(Alternative->getBeginLoc(), "viable 'at()' is defined here",
+           DiagnosticIDs::Note)
+          << Alternative->getNameInfo().getSourceRange();
+
+    } else if (FixMode == Function) {
+      // Case: a[i] => f(a, i)
+      //
+      // Since C++23, the subscript operator may also be called without an
+      // argument, which makes the following distinction necessary
+      const bool EmptySubscript =
+          MatchedExpr->getDirectCallee()->getNumParams() == 0;
+
+      if (EmptySubscript) {
+        auto D = diag(MatchedExpr->getCallee()->getBeginLoc(),
+                      "possibly unsafe 'operator[]'%select{, use safe "
+                      "function '%1() instead|}0")
+                 << FixFunctionEmptyArgs.empty() << FixFunctionEmptyArgs.str()
+                 << MatchedExpr->getCallee()->getSourceRange();
+        if (!FixFunctionEmptyArgs.empty()) {
+          D << FixItHint::CreateInsertion(OCE->getArg(0)->getBeginLoc(),
+                                          FixFunctionEmptyArgs.str() + "(")
+            << FixItHint::CreateRemoval(LeftBracket)
+            << FixItHint::CreateReplacement(RightBracket, ")");
+        }
+      } else {
+        diag(MatchedExpr->getCallee()->getBeginLoc(),
+             "possibly unsafe 'operator[]', use safe function '%0()' instead")
+            << FixFunction.str() << MatchedExpr->getCallee()->getSourceRange()
+            << FixItHint::CreateInsertion(OCE->getArg(0)->getBeginLoc(),
+                                          FixFunction.str() + "(")
+            << FixItHint::CreateReplacement(LeftBracket, ", ")
+            << FixItHint::CreateReplacement(RightBracket, ")");
+      }
+    }
+  } else if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(MatchedExpr)) {
+    // Case: a.operator[](i) or a->operator[](i)
+    const auto *Callee = dyn_cast<MemberExpr>(MCE->getCallee());
+
+    if (FixMode == At) {
+      // Cases: a.operator[](i) => a.at(i) and a->operator[](i) => a->at(i)
+
+      const auto *MatchedOperator =
+          Result.Nodes.getNodeAs<CXXMethodDecl>("operator");
+
+      const CXXMethodDecl *Alternative = findAlternativeAt(MatchedOperator);
+      if (!Alternative) {
+        diag(Callee->getBeginLoc(), "possibly unsafe 'operator[]', consider "
+                                    "bounds-safe alternative 'at()'")
+            << Callee->getSourceRange();
+        return;
+      }
+      diag(MatchedExpr->getCallee()->getBeginLoc(),
+           "possibly unsafe 'operator[]', consider "
+           "bounds-safe alternative 'at()'")
+          << FixItHint::CreateReplacement(
+                 SourceRange(Callee->getMemberLoc(), Callee->getEndLoc()),
+                 "at");
+
+      diag(Alternative->getBeginLoc(), "viable 'at()' defined here",
+           DiagnosticIDs::Note)
+          << Alternative->getNameInfo().getSourceRange();
+
+    } else if (FixMode == Function) {
+      // Cases: a.operator[](i) => f(a, i) and a->operator[](i) => f(*a, i)
+      const auto *Callee = dyn_cast<MemberExpr>(MCE->getCallee());
+
+      const bool EmptySubscript =
+          MCE->getMethodDecl()->getNumNonObjectParams() == 0;
+
+      std::string BeginInsertion =
+          (EmptySubscript ? FixFunctionEmptyArgs.str() : FixFunction.str()) +
+          "(";
+
+      if (Callee->isArrow())
+        BeginInsertion += "*";
+
+      // Since C++23, the subscript operator may also be called without an
+      // argument, which makes the following distinction necessary
+      if (EmptySubscript) {
+        auto D = diag(MatchedExpr->getCallee()->getBeginLoc(),
+                      "possibly unsafe 'operator[]'%select{, use safe "
+                      "function '%1()' instead|}0")
+                 << FixFunctionEmptyArgs.empty() << FixFunctionEmptyArgs.str()
+                 << Callee->getSourceRange();
+
+        if (!FixFunctionEmptyArgs.empty()) {
+          D << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(),
+                                          BeginInsertion)
+            << FixItHint::CreateRemoval(
+                   SourceRange(Callee->getOperatorLoc(),
+                               MCE->getRParenLoc().getLocWithOffset(-1)));
+        }
+      } else {
+        diag(Callee->getBeginLoc(),
+             "possibly unsafe 'operator[]', use safe function '%0()' instead")
+            << FixFunction.str() << Callee->getSourceRange()
+            << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(),
+                                          BeginInsertion)
+            << FixItHint::CreateReplacement(
+                   SourceRange(
+                       Callee->getOperatorLoc(),
+                       MCE->getArg(0)->getBeginLoc().getLocWithOffset(-1)),
+                   ", ");
+      }
+    }
+  }
+}
+
+} // namespace clang::tidy::cppcoreguidelines
+
+namespace clang::tidy {
+using P = cppcoreguidelines::ProBoundsAvoidUncheckedContainerAccess;
+
+llvm::ArrayRef<std::pair<P::FixModes, StringRef>>
+OptionEnumMapping<P::FixModes>::getEnumMapping() {
+  static constexpr std::pair<P::FixModes, StringRef> Mapping[] = {
+      {P::None, "none"}, {P::At, "at"}, {P::Function, "function"}};
+  return {Mapping};
+}
+} // namespace clang::tidy

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

@@ -0,0 +1,56 @@
+//===--- ProBoundsAvoidUncheckedContainerAccess.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_PRO_BOUNDS_AVOID_UNCHECKED_CONTAINER_ACCESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_AVOID_UNCHECKED_CONTAINER_ACCESS_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Flags calls to operator[] in STL containers and suggests replacing it with
+/// safe alternatives.
+///
+/// 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/pro-bounds-avoid-unchecked-container-access.html
+class ProBoundsAvoidUncheckedContainerAccess : public ClangTidyCheck {
+public:
+  ProBoundsAvoidUncheckedContainerAccess(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;
+
+  enum FixModes { None, At, Function };
+
+private:
+  // A list of class names that are excluded from the warning
+  std::vector<llvm::StringRef> ExcludedClasses;
+  // Setting which fix to suggest
+  FixModes FixMode;
+  llvm::StringRef FixFunction;
+  llvm::StringRef FixFunctionEmptyArgs;
+};
+} // namespace clang::tidy::cppcoreguidelines
+
+namespace clang::tidy {
+template <>
+struct OptionEnumMapping<
+    cppcoreguidelines::ProBoundsAvoidUncheckedContainerAccess::FixModes> {
+  static ArrayRef<std::pair<
+      cppcoreguidelines::ProBoundsAvoidUncheckedContainerAccess::FixModes,
+      StringRef>>
+  getEnumMapping();
+};
+} // namespace clang::tidy
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_AVOID_UNCHECKED_CONTAINER_ACCESS_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -135,6 +135,13 @@ New checks
   Detects default initialization (to 0) of variables with ``enum`` type where
   the enum has no enumerator with value of 0.
 
+- New :doc:`cppcoreguidelines-pro-bounds-avoid-unchecked-container-access
+  <clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access>`
+  check.
+
+  Finds calls to ``operator[]`` in STL containers and suggests replacing them
+  with safe alternatives.
+
 - New :doc:`llvm-mlir-op-builder
   <clang-tidy/checks/llvm/use-new-mlir-op-builder>` check.