[clang-tools-extra] Enforce SL.con.3: Add check to replace operator[] with at() [Cont.] (PR #95220)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 12 03:12:09 PDT 2024
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/95220 at github.com>
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: Paul Heidekrüger (PBHDK)
<details>
<summary>Changes</summary>
This PR is based on the PR #<!-- -->90043 by @<!-- -->sebwolf-de, who has given us (@<!-- -->leunam99 and myself) [permission to continue his work](https://github.com/llvm/llvm-project/pull/90043#issuecomment-2137455627).
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](https://github.com/llvm/llvm-project/pull/90043#discussion_r1579858850)
- Rename the analysis from AvoidBoundsErrorsCheck to PreferAtOverSubscriptOperatorCheck as requested by @<!-- -->PiotrZSL [here](https://github.com/llvm/llvm-project/pull/90043#discussion_r1579853430)
- Add a more detailed description of what the analysis does as requested by @<!-- -->PiotrZSL [here](https://github.com/llvm/llvm-project/pull/90043#discussion_r1579847155)
We explicitly don't ignore unused code with `TK_IgnoreUnlessSpelledInSource`, as requested by @<!-- -->PiotrZSL [here](https://github.com/llvm/llvm-project/pull/90043#discussion_r1579844550), 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
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/95220
More information about the cfe-commits
mailing list