Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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"

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.

#include <iostream>
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 {

bool isApplicable(const QualType &Type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

static?

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

Choose a reason for hiding this comment

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

check may break std::map currently.
for example:

   someMap[key] = 10; -> someMap.at(key) = 10;

This would change behavior of code.

Copy link

Choose a reason for hiding this comment

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

We had a look at the standard library; the same applies to std::unordered_map and std::flat_map too, correct? So, by default, we'd be excluding std::map, std::unordered_map, and std::flat_map. Can you think of any others?

}
if (TypeStr.find("std::unordered_map") != std::string::npos) {
Result = true;
}
if (TypeStr.find("std::flat_map") != std::string::npos) {
Result = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

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,

// TODO Add std::span with C++26
return Result;
}

void AvoidBoundsErrorsCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
callExpr(callee(cxxMethodDecl(hasName("operator[]")).bind("f")))
Copy link
Member

Choose a reason for hiding this comment

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

instead of hasName use hasOverloadedOperatorName.
and instead of callExpr use cxxOperatorCallExpr, and verify on this level that object got also .at method.

.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");
Copy link
Member

Choose a reason for hiding this comment

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

use more proper names than x/f

const auto Type = MatchedFunction->getThisType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use auto - type is not spelled explicitly.

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

Choose a reason for hiding this comment

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

may not work with macros.


// 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, ")");
Copy link
Member

Choose a reason for hiding this comment

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

Do not operate at strings, operate by using tokens/source location, because otherwise, single space will break code.


diag(MatchedExpr->getBeginLoc(), "Do not use operator[], use at() instead.")
Copy link
Member

Choose a reason for hiding this comment

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

keep diagnostic an lower case, not need for point at the end

<< FixItHint::CreateReplacement(Range, NewCode);
Copy link
Member

Choose a reason for hiding this comment

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

you could verify at this level that exceptions are enabled (check LangOptions).
Because fix may not be perfect, you may consider attaching fix to note instead of warning.
And raise also a note that would point where operator[] is defined, and where method at is.

}

} // namespace clang::tidy::cppcoreguidelines
Original file line number Diff line number Diff line change
@@ -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;
};
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()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use double back-ticks for language constructs. Same in documentation.


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

Choose a reason for hiding this comment

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

This usually placed at the end of documentation.

Copy link
Member

Choose a reason for hiding this comment

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

in first line put short description what actually check does, and synchronize it with release notes and doxygen documentation for a class

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);
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.

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