Paul =?utf-8?q?Heidekrüger?= <paul.heidekrue...@tum.de>, Paul =?utf-8?q?Heidekrüger?= <paul.heidekrue...@tum.de>, Paul =?utf-8?q?Heidekrüger?= <paul.heidekrue...@tum.de>, Paul =?utf-8?q?Heidekrüger?= <paul.heidekrue...@tum.de>, Paul =?utf-8?q?Heidekrüger?= <paul.heidekrue...@tum.de> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/95...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
