Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -29,6 +29,7 @@ #include "LambdaFunctionNameCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" +#include "MapSubscriptOperatorLookupCheck.h" #include "MisplacedOperatorInStrlenInAllocCheck.h" #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" @@ -108,6 +109,8 @@ "bugprone-macro-parentheses"); CheckFactories.registerCheck( "bugprone-macro-repeated-side-effects"); + CheckFactories.registerCheck( + "bugprone-map-subscript-operator-lookup"); CheckFactories.registerCheck( "bugprone-misplaced-operator-in-strlen-in-alloc"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -24,6 +24,7 @@ LambdaFunctionNameCheck.cpp MacroParenthesesCheck.cpp MacroRepeatedSideEffectsCheck.cpp + MapSubscriptOperatorLookupCheck.cpp MisplacedOperatorInStrlenInAllocCheck.cpp MisplacedPointerArithmeticInAllocCheck.cpp MisplacedWideningCastCheck.cpp Index: clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h @@ -0,0 +1,38 @@ +//===--- MapSubscriptOperatorLookupCheck.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_BUGPRONE_MAPSUBSCRIPTOPERATORLOOKUPCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MAPSUBSCRIPTOPERATORLOOKUPCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Detects map lookups done with operator[]. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-map-subscript-operator-lookup.html +class MapSubscriptOperatorLookupCheck : public ClangTidyCheck { +public: + MapSubscriptOperatorLookupCheck(StringRef Name, ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + std::string MapTypes; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MAPSUBSCRIPTOPERATORLOOKUPCHECK_H Index: clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp @@ -0,0 +1,70 @@ +//===--- MapSubscriptOperatorLookupCheck.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 "MapSubscriptOperatorLookupCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +MapSubscriptOperatorLookupCheck::MapSubscriptOperatorLookupCheck( + llvm::StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + MapTypes(Options.get("MapTypes", "::std::map;::std::unordered_map")) {} + +bool MapSubscriptOperatorLookupCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus; +} + +void MapSubscriptOperatorLookupCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "MapTypes", MapTypes); +} + +void MapSubscriptOperatorLookupCheck::registerMatchers(MatchFinder *Finder) { + auto MapTypeVec = utils::options::parseStringList(MapTypes); + Finder->addMatcher( + cxxOperatorCallExpr( + callee(cxxMethodDecl(hasName("operator[]"), + ofClass(hasAnyName(std::vector( + MapTypeVec.begin(), MapTypeVec.end())))) + .bind("operator")), + anyOf( + // the return value is used in const context + hasParent(implicitCastExpr( + anyOf(hasImplicitDestinationType(isConstQualified()), + // for scalar types const usage of the return value is + // seen as lvalue -> rvalue cast + hasCastKind(CK_LValueToRValue)))), + // address of the return value is taken, but the resulting pointer + // is used as pointer-to-const + hasParent(unaryOperator( + hasOperatorName("&"), + hasParent(implicitCastExpr(hasImplicitDestinationType( + pointsTo(isConstQualified())))))))) + .bind("call"), + this); +} + +void MapSubscriptOperatorLookupCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs("call"); + const auto *Operator = Result.Nodes.getNodeAs("operator"); + diag(Call->getBeginLoc(), "do not use operator[] for %0 lookup") + << Operator->getParent()->getName(); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -75,6 +75,12 @@ New checks ^^^^^^^^^^ + +- New :doc:`bugprone-map-subscript-operator-lookup + ` check. + + Finds map lookups done with ``operator[]``. + - New :doc:`cppcoreguidelines-avoid-non-const-global-variables ` check. Finds non-const global variables as described in check I.2 of C++ Core Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst @@ -0,0 +1,45 @@ +.. title:: clang-tidy - bugprone-map-subscript-operator-lookup + +bugprone-map-subscript-operator-lookup +====================================== + +Finds map lookups done with ``operator[]``. + +Using ``std::map::operator[]`` for finding values from the map is a common +source of bugs in C++ programs. The operator's side effect of possibly inserting +a value to the map can easily cause surprises. The usage of the operator can be +replaced with ``at()`` or ``find()`` in cases where it's used to only look up +values from the map with no intention of modifying it. + +This check warns when the operator is used for map lookup: + +.. code-block:: c++ + + std::map m; + const auto i = m[1]; // warning + +The check only warns about *lookups* done with ``operator[]``. Modifying the map +using it is still ok: + +.. code-block:: c++ + + std::map m; + m[1] = 2; // ok + +In general, doing any mutating operation through the reference returned by the +operator is allowed by this check. If the reference is used as +reference-to-const, a warning is given: + +.. code-block:: c++ + + std::map m; + m[1].setter(1); // ok + const auto a = m[1].getter(); // warning + +Options +------- + +.. option:: MapTypes + + Semicolon-separated list of map types to check. Defaults to + ``::std::map;::std::unordered_map``. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -64,6 +64,7 @@ `bugprone-lambda-function-name `_, `bugprone-macro-parentheses `_, "Yes" `bugprone-macro-repeated-side-effects `_, + `bugprone-map-subscript-operator-lookup `_, `bugprone-misplaced-operator-in-strlen-in-alloc `_, "Yes" `bugprone-misplaced-pointer-arithmetic-in-alloc `_, "Yes" `bugprone-misplaced-widening-cast `_, Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp @@ -0,0 +1,45 @@ +// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: bugprone-map-subscript-operator-lookup.MapTypes, \ +// RUN: value: "::ns::Map"}]}' \ +// RUN: -- + +namespace ns { + +template +struct Map { + V &operator[](const K &); + V &operator[](K &&); +}; + +template +struct DerivedMap : public Map {}; + +using IntMap = Map; + +} // namespace ns + +void warning() { + ns::Map M; + auto A = M[1]; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for Map lookup [bugprone-map-subscript-operator-lookup] + + ns::DerivedMap DM; + auto B = DM[1]; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for Map lookup [bugprone-map-subscript-operator-lookup] + + ns::IntMap IM; + auto C = IM[1]; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for Map lookup [bugprone-map-subscript-operator-lookup] +} + +void noWarning() { + ns::Map MOk; + MOk[1] = 2; + + ns::DerivedMap DMOk; + DMOk[1] = 2; + + ns::IntMap IMOk; + IMOk[1] = 2; +} Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp @@ -0,0 +1,267 @@ +// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t + +namespace std { + +template +struct allocator {}; + +template +struct equal_to {}; + +template +struct hash {}; + +template +struct less {}; + +template +struct pair {}; + +template , + typename Allocator = std::allocator>> +struct map { + T &operator[](const Key &); + T &operator[](Key &&); +}; + +// the check should be able to match std lib calls even if the functions are +// declared inside inline namespaces +inline namespace v1 { + +template , + typename KeyEqual = std::equal_to, + typename Allocator = std::allocator>> +struct unordered_map { + T &operator[](const Key &); + T &operator[](Key &&); +}; + +} // namespace v1 +} // namespace std + +struct Foo { + int getter() const; + void setter(int); + bool operator==(const Foo &) const; +}; + +using FooPtr = Foo *; + +void readInt(const int &); +void writeInt(int &); +void copyInt(int); + +void readFoo(const Foo &); +void writeFoo(Foo &); +void copyFoo(Foo); + +void copyFooPtr(Foo *); +void copyConstFooPtr(const Foo *); + +std::map IntMap; +std::map FooMap; +std::unordered_map FooPtrMap; + +void copyFromMap() { + int A = IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + Foo B = FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + FooPtr C = FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +void constRefFromMap() { + const int &A = IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + const Foo &B = FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + const FooPtr &C = FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +void constPtrFromMap() { + const int *A = &IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + const Foo *B = &FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + const FooPtr *C = &FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +int returnInt() { + return IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +Foo returnFoo() { + return FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +FooPtr returnFooPtr() { + return FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +const int &returnRefToConstInt() { + return IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +const Foo &returnRefToConstFoo() { + return FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +const FooPtr &returnRefToConstFooPtr() { + return FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +const int *returnPtrToConstInt() { + return &IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +const Foo *returnPtrToConstFoo() { + return &FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +const FooPtr *returnPtrToConstFooPtr() { + return &FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +void callReadX() { + readInt(IntMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + readFoo(FooMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void callCopyX() { + copyInt(IntMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + copyFoo(FooMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + copyFooPtr(FooPtrMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] + copyConstFooPtr(FooPtrMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +void passAddrToFunctionTakingConstPtr() { + copyConstFooPtr(&FooMap[1]); + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void callConstMemf() { + FooMap[1].getter(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void deref() { + Foo F = *FooPtrMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] + FooPtrMap[1]->getter(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] + FooPtrMap[1]->setter(1); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup] +} + +void arithmetic() { + int A = 1 + IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void comparison() { + bool A = IntMap[1] == 1; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + bool B = FooMap[1] == Foo{}; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void convertedToBool() { + if (IntMap[1]) + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + ; +} + +void thrown() { + throw IntMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] + throw FooMap[1]; + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup] +} + +void writeToMap() { + IntMap[1] = 1; + FooMap[1] = Foo{}; + FooPtrMap[1] = new Foo; +} + +void refFromMap() { + int &A = IntMap[1]; + Foo &B = FooMap[1]; + FooPtr &C = FooPtrMap[1]; +} + +void ptrFromMap() { + int *A = &IntMap[1]; + Foo *B = &FooMap[1]; + FooPtr *C = &FooPtrMap[1]; +} + +int &returnRefToInt() { + return IntMap[1]; +} + +Foo &returnRefToFoo() { + return FooMap[1]; +} + +FooPtr &returnRefToFooPtr() { + return FooPtrMap[1]; +} + +int *returnPtrToInt() { + return &IntMap[1]; +} + +Foo *returnPtrToFoo() { + return &FooMap[1]; +} + +FooPtr *returnPtrToFooPtr() { + return &FooPtrMap[1]; +} + +void callWriteX() { + writeInt(IntMap[1]); + writeFoo(FooMap[1]); +} + +void passAddrToFunctionTakingPtr() { + copyFooPtr(&FooMap[1]); +} + +void callMemf() { + FooMap[1].setter(1); +} + +void increment() { + ++IntMap[1]; + IntMap[1]++; +} + +void unused() { + IntMap[1]; + FooMap[1]; + FooPtrMap[1]; +}