Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
//===--- 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"

Copy link
Contributor

Choose a reason for hiding this comment

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

Excessive newline.

using namespace clang::ast_matchers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate with newline.


namespace clang::tidy::cppcoreguidelines {

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 a = 0; a < Method->getNumParams(); a++) {
const bool SameArgType =
Method->parameters()[a]->getOriginalType() ==
MatchedOperator->parameters()[a]->getOriginalType();
if (!SameArgType)
continue;
}

return Method;
}
return static_cast<CXXMethodDecl *>(nullptr);
}

void AvoidBoundsErrorsCheck::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 AvoidBoundsErrorsCheck::check(const MatchFinder::MatchResult &Result) {
const ASTContext &Context = *Result.Context;
const SourceManager &Source = Context.getSourceManager();
const CallExpr *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("caller");
const CXXMethodDecl *MatchedOperator =
Result.Nodes.getNodeAs<CXXMethodDecl>("operator");
const CXXRecordDecl *MatchedParent =
Result.Nodes.getNodeAs<CXXRecordDecl>("parent");

const CXXMethodDecl *Alternative =
findAlternative(MatchedParent, MatchedOperator);
if (!Alternative)
return;

const SourceLocation AlternativeSource(Alternative->getBeginLoc());

diag(MatchedExpr->getBeginLoc(),
"found possibly unsafe operator[], consider using at() instead");
diag(Alternative->getBeginLoc(), "alternative at() defined here",
DiagnosticIDs::Note);
}

} // namespace clang::tidy::cppcoreguidelines
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//===--- 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) {}
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;
};
PiotrZSL marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

ignore implicit code with TK_IgnoreUnlessSpelledInSource.


} // namespace clang::tidy::cppcoreguidelines

#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDBOUNDSERRORSCHECK_H
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
)

add_clang_library(clangTidyCppCoreGuidelinesModule
AvoidBoundsErrorsCheck.cpp
AvoidCapturingLambdaCoroutinesCheck.cpp
AvoidConstOrRefDataMembersCheck.cpp
AvoidDoWhileCheck.cpp
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -57,6 +58,8 @@ namespace cppcoreguidelines {
class CppCoreGuidelinesModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<AvoidBoundsErrorsCheck>(
"cppcoreguidelines-avoid-bounds-errors");
Copy link
Member

Choose a reason for hiding this comment

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

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.

CheckFactories.registerCheck<AvoidCapturingLambdaCoroutinesCheck>(
"cppcoreguidelines-avoid-capturing-lambda-coroutines");
CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.. title:: clang-tidy - cppcoreguidelines-avoid-bounds-errors

cppcoreguidelines-avoid-bounds-errors
=====================================

This check 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);
Copy link
Member

Choose a reason for hiding this comment

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

consider adding option to specify excluded classes, if class detection will be automatic.


This check enforces the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline.
1 change: 1 addition & 0 deletions clang-tools-extra/docs/clang-tidy/checks/list.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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>`,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
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;
Copy link
Member

Choose a reason for hiding this comment

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

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
  • ...

Copy link

Choose a reason for hiding this comment

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

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?


auto b = a[0];
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-avoid-bounds-errors]
auto c = a[1+1];
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-avoid-bounds-errors]
constexpr int index = 1;
auto d = a[index];
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-avoid-bounds-errors]

int e(int index) {
return a[index];
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-avoid-bounds-errors]
}

auto f = (&a)->operator[](1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-avoid-bounds-errors]

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];