Enforce SL.con.3: Add check to replace operator[] with at() #90043
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review 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. |
|
@llvm/pr-subscribers-clang-tidy Author: Sebastian Wolf (sebwolf-de) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/90043.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp
new file mode 100644
index 00000000000000..524c21b5bdb818
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp
@@ -0,0 +1,81 @@
+//===--- AvoidBoundsErrorsCheck.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 "AvoidBoundsErrorsCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+#include <iostream>
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+bool isApplicable(const QualType &Type) {
+ const auto TypeStr = Type.getAsString();
+ bool Result = false;
+ // Only check for containers in the std namespace
+ if (TypeStr.find("std::vector") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::array") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::deque") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::map") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::unordered_map") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::flat_map") != std::string::npos) {
+ Result = true;
+ }
+ // TODO Add std::span with C++26
+ return Result;
+}
+
+void AvoidBoundsErrorsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(callee(cxxMethodDecl(hasName("operator[]")).bind("f")))
+ .bind("x"),
+ this);
+}
+
+void AvoidBoundsErrorsCheck::check(const MatchFinder::MatchResult &Result) {
+ const ASTContext &Context = *Result.Context;
+ const SourceManager &Source = Context.getSourceManager();
+ const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("x");
+ const auto *MatchedFunction = Result.Nodes.getNodeAs<CXXMethodDecl>("f");
+ const auto Type = MatchedFunction->getThisType();
+ if (!isApplicable(Type)) {
+ return;
+ }
+
+ // Get original code.
+ const SourceLocation b(MatchedExpr->getBeginLoc());
+ const SourceLocation e(MatchedExpr->getEndLoc());
+ const std::string OriginalCode =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(b, e), Source,
+ getLangOpts())
+ .str();
+ const auto Range = SourceRange(b, e);
+
+ // Build replacement.
+ std::string NewCode = OriginalCode;
+ const auto BeginOpen = NewCode.find("[");
+ NewCode.replace(BeginOpen, 1, ".at(");
+ const auto BeginClose = NewCode.find("]");
+ NewCode.replace(BeginClose, 1, ")");
+
+ diag(MatchedExpr->getBeginLoc(), "Do not use operator[], use at() instead.")
+ << FixItHint::CreateReplacement(Range, NewCode);
+}
+
+} // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h
new file mode 100644
index 00000000000000..f915729cd7bbee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h
@@ -0,0 +1,32 @@
+//===--- AvoidBoundsErrorsCheck.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_AVOIDBOUNDSERRORSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDBOUNDSERRORSCHECK_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/avoid-bounds-errors.html
+class AvoidBoundsErrorsCheck : public ClangTidyCheck {
+public:
+ AvoidBoundsErrorsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDBOUNDSERRORSCHECK_H
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
index eb35bbc6a538fe..4972d20417928d 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
)
add_clang_library(clangTidyCppCoreGuidelinesModule
+ AvoidBoundsErrorsCheck.cpp
AvoidCapturingLambdaCoroutinesCheck.cpp
AvoidConstOrRefDataMembersCheck.cpp
AvoidDoWhileCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index e9f0201615616f..525bbc7a42adaa 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -19,6 +19,7 @@
#include "../performance/NoexceptMoveConstructorCheck.h"
#include "../performance/NoexceptSwapCheck.h"
#include "../readability/MagicNumbersCheck.h"
+#include "AvoidBoundsErrorsCheck.h"
#include "AvoidCapturingLambdaCoroutinesCheck.h"
#include "AvoidConstOrRefDataMembersCheck.h"
#include "AvoidDoWhileCheck.h"
@@ -57,6 +58,8 @@ namespace cppcoreguidelines {
class CppCoreGuidelinesModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<AvoidBoundsErrorsCheck>(
+ "cppcoreguidelines-avoid-bounds-errors");
CheckFactories.registerCheck<AvoidCapturingLambdaCoroutinesCheck>(
"cppcoreguidelines-avoid-capturing-lambda-coroutines");
CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5b1feffb89ea06..1949f18a586ed8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -131,6 +131,11 @@ New checks
to reading out-of-bounds data due to inadequate or incorrect string null
termination.
+- New :doc:`cppcoreguidelines-avoid-bounds-errors
+ <clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors>` check.
+
+ Flags the unsafe `operator[]` and replaces 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/avoid-bounds-errors.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors.rst
new file mode 100644
index 00000000000000..8fb2e3bfde0981
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-bounds-errors
+
+cppcoreguidelines-avoid-bounds-errors
+=====================================
+
+This check enforces the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline.
+It flags all uses of `operator[]` on `std::vector`, `std::array`, `std::deque`, `std::map`, `std::unordered_map`, and `std::flat_map` and suggests to replace it with `at()`.
+Note that `std::span` and `std::mdspan` do not support `at()` as of C++23, so the use of `operator[]` is not flagged.
+
+For example the code
+
+.. code-block:: c++
+ std::array<int, 3> a;
+ int b = a[4];
+
+will be replaced by
+
+.. code-block:: c++
+ std::vector<int, 3> a;
+ int b = a.at(4);
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5d9d487f75f9cb..8b472fca2d8ca1 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -174,6 +174,7 @@ Clang-Tidy Checks
:doc:`cert-oop58-cpp <cert/oop58-cpp>`,
:doc:`concurrency-mt-unsafe <concurrency/mt-unsafe>`,
:doc:`concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous>`,
+ :doc:`cppcoreguidelines-avoid-bounds-errors <cppcoreguidelines/avoid-bounds-errors>`, "Yes"
:doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines <cppcoreguidelines/avoid-capturing-lambda-coroutines>`,
:doc:`cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members>`,
:doc:`cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp
new file mode 100644
index 00000000000000..23453b1f2df218
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp
@@ -0,0 +1,66 @@
+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>
+ 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
+
+
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-bounds-errors %t
+std::array<int, 3> a;
+
+auto b = a[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto b = a.at(0);
+auto c = a[1+1];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto c = a.at(1+1);
+constexpr int index = 1;
+auto d = a[index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto d = a.at(index);
+
+int e(int index) {
+ return a[index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: return a.at(index);
+}
+
+auto f = 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];
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Sebastian Wolf (sebwolf-de) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/90043.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp
new file mode 100644
index 00000000000000..524c21b5bdb818
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.cpp
@@ -0,0 +1,81 @@
+//===--- AvoidBoundsErrorsCheck.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 "AvoidBoundsErrorsCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+#include <iostream>
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+bool isApplicable(const QualType &Type) {
+ const auto TypeStr = Type.getAsString();
+ bool Result = false;
+ // Only check for containers in the std namespace
+ if (TypeStr.find("std::vector") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::array") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::deque") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::map") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::unordered_map") != std::string::npos) {
+ Result = true;
+ }
+ if (TypeStr.find("std::flat_map") != std::string::npos) {
+ Result = true;
+ }
+ // TODO Add std::span with C++26
+ return Result;
+}
+
+void AvoidBoundsErrorsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(callee(cxxMethodDecl(hasName("operator[]")).bind("f")))
+ .bind("x"),
+ this);
+}
+
+void AvoidBoundsErrorsCheck::check(const MatchFinder::MatchResult &Result) {
+ const ASTContext &Context = *Result.Context;
+ const SourceManager &Source = Context.getSourceManager();
+ const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("x");
+ const auto *MatchedFunction = Result.Nodes.getNodeAs<CXXMethodDecl>("f");
+ const auto Type = MatchedFunction->getThisType();
+ if (!isApplicable(Type)) {
+ return;
+ }
+
+ // Get original code.
+ const SourceLocation b(MatchedExpr->getBeginLoc());
+ const SourceLocation e(MatchedExpr->getEndLoc());
+ const std::string OriginalCode =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(b, e), Source,
+ getLangOpts())
+ .str();
+ const auto Range = SourceRange(b, e);
+
+ // Build replacement.
+ std::string NewCode = OriginalCode;
+ const auto BeginOpen = NewCode.find("[");
+ NewCode.replace(BeginOpen, 1, ".at(");
+ const auto BeginClose = NewCode.find("]");
+ NewCode.replace(BeginClose, 1, ")");
+
+ diag(MatchedExpr->getBeginLoc(), "Do not use operator[], use at() instead.")
+ << FixItHint::CreateReplacement(Range, NewCode);
+}
+
+} // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h
new file mode 100644
index 00000000000000..f915729cd7bbee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidBoundsErrorsCheck.h
@@ -0,0 +1,32 @@
+//===--- AvoidBoundsErrorsCheck.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_AVOIDBOUNDSERRORSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDBOUNDSERRORSCHECK_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/avoid-bounds-errors.html
+class AvoidBoundsErrorsCheck : public ClangTidyCheck {
+public:
+ AvoidBoundsErrorsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDBOUNDSERRORSCHECK_H
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
index eb35bbc6a538fe..4972d20417928d 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
)
add_clang_library(clangTidyCppCoreGuidelinesModule
+ AvoidBoundsErrorsCheck.cpp
AvoidCapturingLambdaCoroutinesCheck.cpp
AvoidConstOrRefDataMembersCheck.cpp
AvoidDoWhileCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index e9f0201615616f..525bbc7a42adaa 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -19,6 +19,7 @@
#include "../performance/NoexceptMoveConstructorCheck.h"
#include "../performance/NoexceptSwapCheck.h"
#include "../readability/MagicNumbersCheck.h"
+#include "AvoidBoundsErrorsCheck.h"
#include "AvoidCapturingLambdaCoroutinesCheck.h"
#include "AvoidConstOrRefDataMembersCheck.h"
#include "AvoidDoWhileCheck.h"
@@ -57,6 +58,8 @@ namespace cppcoreguidelines {
class CppCoreGuidelinesModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<AvoidBoundsErrorsCheck>(
+ "cppcoreguidelines-avoid-bounds-errors");
CheckFactories.registerCheck<AvoidCapturingLambdaCoroutinesCheck>(
"cppcoreguidelines-avoid-capturing-lambda-coroutines");
CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5b1feffb89ea06..1949f18a586ed8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -131,6 +131,11 @@ New checks
to reading out-of-bounds data due to inadequate or incorrect string null
termination.
+- New :doc:`cppcoreguidelines-avoid-bounds-errors
+ <clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors>` check.
+
+ Flags the unsafe `operator[]` and replaces 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/avoid-bounds-errors.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors.rst
new file mode 100644
index 00000000000000..8fb2e3bfde0981
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-bounds-errors
+
+cppcoreguidelines-avoid-bounds-errors
+=====================================
+
+This check enforces the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline.
+It flags all uses of `operator[]` on `std::vector`, `std::array`, `std::deque`, `std::map`, `std::unordered_map`, and `std::flat_map` and suggests to replace it with `at()`.
+Note that `std::span` and `std::mdspan` do not support `at()` as of C++23, so the use of `operator[]` is not flagged.
+
+For example the code
+
+.. code-block:: c++
+ std::array<int, 3> a;
+ int b = a[4];
+
+will be replaced by
+
+.. code-block:: c++
+ std::vector<int, 3> a;
+ int b = a.at(4);
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5d9d487f75f9cb..8b472fca2d8ca1 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -174,6 +174,7 @@ Clang-Tidy Checks
:doc:`cert-oop58-cpp <cert/oop58-cpp>`,
:doc:`concurrency-mt-unsafe <concurrency/mt-unsafe>`,
:doc:`concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous>`,
+ :doc:`cppcoreguidelines-avoid-bounds-errors <cppcoreguidelines/avoid-bounds-errors>`, "Yes"
:doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines <cppcoreguidelines/avoid-capturing-lambda-coroutines>`,
:doc:`cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members>`,
:doc:`cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp
new file mode 100644
index 00000000000000..23453b1f2df218
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-bounds-errors.cpp
@@ -0,0 +1,66 @@
+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>
+ 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
+
+
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-bounds-errors %t
+std::array<int, 3> a;
+
+auto b = a[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto b = a.at(0);
+auto c = a[1+1];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto c = a.at(1+1);
+constexpr int index = 1;
+auto d = a[index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: auto d = a.at(index);
+
+int e(int index) {
+ return a[index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Do not use operator[], use at() instead. [cppcoreguidelines-avoid-bounds-errors]
+// CHECK-FIXES: return a.at(index);
+}
+
+auto f = 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];
|
| #include "AvoidBoundsErrorsCheck.h" | ||
| #include "clang/ASTMatchers/ASTMatchFinder.h" | ||
| #include "clang/Lex/Lexer.h" | ||
|
|
| #include "clang/Lex/Lexer.h" | ||
|
|
||
| #include <iostream> | ||
| using namespace clang::ast_matchers; |
There was a problem hiding this comment.
Please separate with newline.
|
|
||
| namespace clang::tidy::cppcoreguidelines { | ||
|
|
||
| bool isApplicable(const QualType &Type) { |
| const SourceManager &Source = Context.getSourceManager(); | ||
| const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("x"); | ||
| const auto *MatchedFunction = Result.Nodes.getNodeAs<CXXMethodDecl>("f"); | ||
| const auto Type = MatchedFunction->getThisType(); |
There was a problem hiding this comment.
Please do not use auto - type is not spelled explicitly.
| - New :doc:`cppcoreguidelines-avoid-bounds-errors | ||
| <clang-tidy/checks/cppcoreguidelines/avoid-bounds-errors>` check. | ||
|
|
||
| Flags the unsafe `operator[]` and replaces it with `at()`. |
There was a problem hiding this comment.
Please use double back-ticks for language constructs. Same in documentation.
| cppcoreguidelines-avoid-bounds-errors | ||
| ===================================== | ||
|
|
||
| This check enforces the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline. |
There was a problem hiding this comment.
This usually placed at the end of documentation.
|
Please note: the guidelines do not require one to replace [] with at(), that's just one of the possible solutions. Actually at() is banned in many codebases where exceptions are banned. It would be good to make this fix-it opt-in, configurable via option, so the check only emits a warning by default. |
+1 The advices this is trying to get at is:
|
| if (TypeStr.find("std::vector") != std::string::npos) { | ||
| Result = true; | ||
| } | ||
| if (TypeStr.find("std::array") != std::string::npos) { | ||
| Result = true; | ||
| } | ||
| if (TypeStr.find("std::deque") != std::string::npos) { | ||
| Result = true; | ||
| } | ||
| if (TypeStr.find("std::map") != std::string::npos) { | ||
| Result = true; | ||
| } | ||
| if (TypeStr.find("std::unordered_map") != std::string::npos) { | ||
| Result = true; | ||
| } | ||
| if (TypeStr.find("std::flat_map") != std::string::npos) { | ||
| Result = true; | ||
| } |
There was a problem hiding this comment.
Nope, instead verify that callee got an .at method that take same arguments, got same const/rref qualifier.
In such case this will work for all containers that support this notation, not only those,
|
|
||
| void AvoidBoundsErrorsCheck::registerMatchers(MatchFinder *Finder) { | ||
| Finder->addMatcher( | ||
| callExpr(callee(cxxMethodDecl(hasName("operator[]")).bind("f"))) |
There was a problem hiding this comment.
instead of hasName use hasOverloadedOperatorName.
and instead of callExpr use cxxOperatorCallExpr, and verify on this level that object got also .at method.
| const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("x"); | ||
| const auto *MatchedFunction = Result.Nodes.getNodeAs<CXXMethodDecl>("f"); |
There was a problem hiding this comment.
use more proper names than x/f
| Lexer::getSourceText(CharSourceRange::getTokenRange(b, e), Source, | ||
| getLangOpts()) | ||
| .str(); | ||
| const auto Range = SourceRange(b, e); |
| std::string NewCode = OriginalCode; | ||
| const auto BeginOpen = NewCode.find("["); | ||
| NewCode.replace(BeginOpen, 1, ".at("); | ||
| const auto BeginClose = NewCode.find("]"); | ||
| NewCode.replace(BeginClose, 1, ")"); |
There was a problem hiding this comment.
Do not operate at strings, operate by using tokens/source location, because otherwise, single space will break code.
| cppcoreguidelines-avoid-bounds-errors | ||
| ===================================== | ||
|
|
||
| This check enforces the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline. |
There was a problem hiding this comment.
in first line put short description what actually check does, and synchronize it with release notes and doxygen documentation for a class
| const auto BeginClose = NewCode.find("]"); | ||
| NewCode.replace(BeginClose, 1, ")"); | ||
|
|
||
| diag(MatchedExpr->getBeginLoc(), "Do not use operator[], use at() instead.") |
There was a problem hiding this comment.
keep diagnostic an lower case, not need for point at the end
| @@ -57,6 +58,8 @@ namespace cppcoreguidelines { | |||
| class CppCoreGuidelinesModule : public ClangTidyModule { | |||
| public: | |||
| void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { | |||
| CheckFactories.registerCheck<AvoidBoundsErrorsCheck>( | |||
| "cppcoreguidelines-avoid-bounds-errors"); | |||
There was a problem hiding this comment.
check name cppcoreguidelines-avoid-bounds-errors is too generic for what check is doing, name it something like cppcoreguidelines-prefer-at-over-subscript-operator, or something simillar. Figure something out.
|
|
||
| .. code-block:: c++ | ||
| std::vector<int, 3> a; | ||
| int b = a.at(4); |
There was a problem hiding this comment.
consider adding option to specify excluded classes, if class detection will be automatic.
|
|
||
|
|
||
| // RUN: %check_clang_tidy %s cppcoreguidelines-avoid-bounds-errors %t | ||
| std::array<int, 3> a; |
There was a problem hiding this comment.
add tests with:
- std::map,
- where operator[] is behind macro
- where argument to operator is behind macro
- where object is behind macro
- where class derive from std::map/array
- where std::array is behind typedef
- where object is an template parameter, and method is called once with class that has .at and once with class that has not
- ...
There was a problem hiding this comment.
As suggested, we added a test where the object is a template parameter and the method is called once with a class that has at() and once with a class that has not, but we are not sure what the best behaviour is. Should we
- not warn, because the fix suggested in the message will lead the other instantiation to 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?
|
I am adding @leunam99 and @PBHDK to the PR, who will contribute in the next few weeks. |
Since we cannot push to this PR, we will be submitting a new PR with @sebwolf-de's + our work, correct, @EugeneZelenko, @PiotrZSL, @HerrCai0907? |
Could you please resolve conversations for fixed comments? |
Once we're clear on all our questions (see comments above), we'll submit a new PR that'll resolve all comments. I don't know if you can assign us to this PR somehow, but if not, someone else would have to mark them as resolved. |
|
Check if you can commit to sebwolf-de:avoid-bounds-error-check |
Unfortunately, we can't. We do not have access to Sebastian's LLVM fork. |
|
Yes, you can just submit new PR, reference this one. |
|
To be clear and reiterate my previous comment: this check should NOT require users to use at(). That behavior should be opt-in. It should only warn about using operator[]. It's up to the users to figure out what the best replacement is. |
I don't think it is a good solution. For this kind of projects, they should disable this check directly. Maybe it is optimization to disable this check if exception feature is disabled. |
Can you elaborate? As I wrote above, the C++ Core Guidelines do not require using Warning about operator[] is good; but the fix is not globally accepted best practice (as per comments above). All I'm saying is that if this fix is wanted, it should be opt-in, which should be fairly easy to achieve?
Using |
Done! Here it is: #95220 |
|
Could this PR be closed? @sebwolf-de, @paulhdk |
|
I guess so? It's the work that #95220 is based on. |
|
I'm in the process of looking at old/duplicate PRs to lower number of ongoing open requests (currently 87) so closing would be helpful. |
|
Closed as now review process is done in #95220 |
….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 <manuelpietsch@outlook.de> Co-authored-by: Sebastian Wolf <wolf.sebastian@in.tum.de> --------- Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com> Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
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.