logan-5 updated this revision to Diff 250272. logan-5 marked 2 inline comments as done. logan-5 added a comment.
Re-juggled and made `bool OnlyDependsOnFundamentalArraySizes(QualType QualTy)` recursive. Made `::` assert more useful. Thanks @Quuxplusone for both good ideas. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72282/new/ https://reviews.llvm.org/D72282 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-generic-lambdas.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-operators.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp @@ -0,0 +1,332 @@ +// RUN: %check_clang_tidy %s bugprone-unintended-adl %t -- \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: bugprone-unintended-adl.AllowedNamespaces, value: 'ignore_me;also::ignore::me'}, \ +// RUN: ]}" -- + +namespace aspace { +struct A {}; +void func(const A &); +} // namespace aspace + +namespace bspace { +void func(int); +void test() { + aspace::A a; + func(5); + func(a); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'aspace::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} aspace::func(a);{{$}} +} +} // namespace bspace + +namespace ops { + +struct Stream { +} stream; +Stream &operator<<(Stream &s, int) { + return s; +} +Stream &operator<<(Stream &s, aspace::A) { + return s; +} +template <typename IStream> +IStream &operator>>(IStream &s, int) { + return s; +} +template <typename IStream> +IStream &operator>>(IStream &s, aspace::A) { + return s; +} +void smooth_operator(Stream); + +} // namespace ops + +void ops_test() { + ops::stream << 5; + // no warning + operator<<(ops::stream, 5); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, 5);{{$}} + ops::stream << aspace::A(); + // no warning + // CHECK-FIXES: {{^}} ops::stream << aspace::A();{{$}} + operator<<(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, aspace::A());{{$}} + + ops::stream >> aspace::A(); + // no warning + // CHECK-FIXES: {{^}} ops::stream >> aspace::A();{{$}} + operator>>(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator>>(ops::stream, aspace::A());{{$}} + + smooth_operator(ops::stream); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::smooth_operator(ops::stream);{{$}} +} + +namespace std { +// return types don't matter, returning 'void' everywhere for simplicity + +template <typename T> +void swap(T &a, T &b); +template <typename T> +void make_error_code(T); +template <typename T> +void make_error_condition(T); +template <typename T> +void data(T); +template <typename T> +void begin(T); +template <typename T> +void end(T); +template <typename T> +void rbegin(T); +template <typename T> +void rend(T); +template <typename T> +void crbegin(T); +template <typename T> +void crend(T); +template <typename T> +void size(T); +template <typename T> +void ssize(T); +template <typename T> +void empty(T); + +template <typename T> +void move(T &&); +template <typename T> +void forward(T &&); + +struct byte {}; + +} // namespace std +namespace ns { + +struct Swappable {}; + +// whitelisted +void swap(Swappable &a, Swappable &b); +void make_error_code(Swappable); +void make_error_condition(Swappable); +void data(Swappable); +void begin(Swappable); +void end(Swappable); +void rbegin(Swappable); +void rend(Swappable); +void crbegin(Swappable); +void crend(Swappable); +void size(Swappable); +void ssize(Swappable); +void empty(Swappable); + +// non-whitelisted +void move(Swappable); +void ref(Swappable); + +struct Swappable2 {}; + +} // namespace ns +struct { + template <typename T> + void operator()(T &&); +} ref; + +void test2() { + // TODO add granularity for detecting functions that may always be called unqualified, + // versus those that can only be called through the 'using' 'two-step' + using namespace std; + ns::Swappable a, b; + swap(a, b); + make_error_code(a); + make_error_condition(a); + data(a); + begin(a); + end(a); + rbegin(a); + rend(a); + crbegin(a); + crend(a); + size(a); + ssize(a); + empty(a); + + move(a); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ns::move(a);{{$}} +} + +template <typename T> +void foo(T t) { + using namespace std; + swap(t, t); + make_error_code(t); + make_error_condition(t); + data(t); + begin(t); + end(t); + rbegin(t); + rend(t); + crbegin(t); + crend(t); + size(t); + ssize(t); + empty(t); + + move(t); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl] + // CHECK-MESSAGES: [[@LINE-2]]:3: note: with argument type 'struct ns::Swappable' + // CHECK-MESSAGES: [[@LINE-3]]:3: warning: unqualified call to 'move' may be resolved through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} move(t);{{$}} + + std::swap(t, t); + std::move(t); + + ref(t); // function objects bypass ADL, this always calls ::ref + ::ref(t); +} + +template <typename T, typename U> +void operator<<(T &&, U &&); + +template <typename T> +void bar(T t) { + t << 5; + operator<<(t, 5); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-MESSAGES: [[@LINE-2]]:3: note: with argument types 'struct ops::Stream', 'int' + // CHECK-MESSAGES: [[@LINE-3]]:3: warning: unqualified call to 'operator<<' may be resolved through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} operator<<(t, 5);{{$}} +} + +void instantiator() { + foo(ns::Swappable()); // instantiation will use ADL + foo(5); // instantiation will not use ADL + + bar(ops::Stream()); // instantiation will use ADL + bar(aspace::A()); // instantiation will not use ADL +} + +template <typename T> +void macro_test(T t) { +#define MACRO(x) find_if(x, x, x) + + MACRO(t); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'find_if' may be resolved through ADL [bugprone-unintended-adl] + +#undef MACRO + +#define MACRO(x) func(x) + + MACRO(aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'aspace::func' through ADL [bugprone-unintended-adl] + +#undef MACRO +} + +template <class T> +void array_bound_test() { + char buf[sizeof(T)]; + memset(buf, '\0', sizeof buf); + + int buf2[sizeof(T) * 2]; + foo(buf2); + + T buf3[5]; + foo(buf3); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'foo' may be resolved through ADL [bugprone-unintended-adl] + + T buf4[sizeof(T)]; + foo(buf4); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'foo' may be resolved through ADL [bugprone-unintended-adl] + + char buf5[sizeof(T)][sizeof(T)]; + foo(buf5); + + foo(__func__); +} + +namespace friends { + +struct befriender { + friend void func(const befriender &); +}; +void func(const befriender &) {} + +struct befriender2; +void func(const befriender2 &) {} +struct befriender2 { + friend void func(const befriender2 &); +}; + +struct befriender3; +void func(const befriender3 &); +struct befriender3 { + friend void func(const befriender3 &) {} +}; + +struct befriender4 { + friend void func(const befriender4 &) {} +}; +void func(const befriender4 &); + +struct hidden_befriender { + friend void func(const hidden_befriender &) {} +}; + +} // namespace friends + +void hidden_friend_test() { + func(friends::befriender()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'friends::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} friends::func(friends::befriender());{{$}} + + func(friends::befriender2()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'friends::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} friends::func(friends::befriender2());{{$}} + + func(friends::befriender3()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'friends::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} friends::func(friends::befriender3());{{$}} + + func(friends::befriender4()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'friends::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} friends::func(friends::befriender4());{{$}} + + // no warning for hidden friends + func(friends::hidden_befriender()); + // CHECK-FIXES: {{^}} func(friends::hidden_befriender());{{$}} +} + +namespace dont_ignore_me { +struct A {}; +void func(A); +} // namespace dont_ignore_me +namespace ignore_me { +struct A {}; +void func(A); +} // namespace ignore_me + +namespace also { +namespace ignore { +namespace me { +struct A {}; +void func(A); +} // namespace me +} // namespace ignore +} // namespace also + +int ns_whitelist_test() { + func(dont_ignore_me::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'dont_ignore_me::func' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} dont_ignore_me::func(dont_ignore_me::A());{{$}} + + func(ignore_me::A()); + // CHECK-FIXES: {{^}} func(ignore_me::A());{{$}} + + func(also::ignore::me::A()); + // CHECK-FIXES: {{^}} func(also::ignore::me::A());{{$}} +} Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-operators.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-operators.cpp @@ -0,0 +1,64 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s bugprone-unintended-adl %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: bugprone-unintended-adl.IgnoreOverloadedOperators, value: 0}, \ +// RUN: ]}' -- + +namespace aspace { +struct A {}; +} // namespace aspace + +namespace ops { + +struct Stream { +} stream; +Stream &operator<<(Stream &s, int) { + return s; +} +Stream &operator<<(Stream &s, aspace::A) { + return s; +} +template <typename IStream> +IStream &operator>>(IStream &s, int) { + return s; +} +template <typename IStream> +IStream &operator>>(IStream &s, aspace::A) { + return s; +} +void smooth_operator(Stream); + +} // namespace ops + +void ops_test() { + ops::stream << 5; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, 5);{{$}} + operator<<(ops::stream, 5); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, 5);{{$}} + ops::stream << aspace::A(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, aspace::A());{{$}} + operator<<(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator<<(ops::stream, aspace::A());{{$}} + + ops::stream >> aspace::A(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator>>(ops::stream, aspace::A());{{$}} + operator>>(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::operator>>(ops::stream, aspace::A());{{$}} + + smooth_operator(ops::stream); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} ops::smooth_operator(ops::stream);{{$}} +} + +template <class T> +void foo(T t) { + t, 0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'operator,' may be resolved through ADL [bugprone-unintended-adl] + !t; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'operator!' may be resolved through ADL [bugprone-unintended-adl] +} Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-generic-lambdas.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-generic-lambdas.cpp @@ -0,0 +1,55 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s bugprone-unintended-adl %t + +namespace std { + +template <typename It, typename Pred> +It find_if(It begin, It end, Pred pred) { + for (; begin != end; ++begin) { + if (pred(*begin)) + break; + } + return begin; +} + +} // namespace std + +namespace ns { + +struct S {}; +void foo(S); +void bar(S, S); + +} // namespace ns + +void foo(int); +void bar(int, int); + +void test() { + { + auto l = [](auto x) { foo(x); }; + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: expression calls 'ns::foo' through ADL [bugprone-unintended-adl] + // CHECK-MESSAGES: [[@LINE-2]]:27: note: with argument type 'struct ns::S' + // CHECK-MESSAGES: [[@LINE-3]]:27: warning: unqualified call to 'foo' may be resolved through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} auto l = [](auto x) { foo(x); };{{$}} + l(5); + l(ns::S()); + } + { + auto l = [](auto x) { bar(x, x); }; + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: expression calls 'ns::bar' through ADL [bugprone-unintended-adl] + // CHECK-MESSAGES: [[@LINE-2]]:27: note: with argument types 'struct ns::S', 'struct ns::S' + // CHECK-MESSAGES: [[@LINE-3]]:27: warning: unqualified call to 'bar' may be resolved through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} auto l = [](auto x) { bar(x, x); };{{$}} + l(5); + l(ns::S()); + } + + int x = 0; + [&](auto &c) { std::find_if(c.begin(), c.end(), + [&](auto &e) { return e.first == x; }); }; + + [&](auto &c) { std::find_if(c.begin(), c.end(), + [&](auto &e) { return find_if(e, e, x); }); }; + // CHECK-MESSAGES: [[@LINE-1]]:53: warning: unqualified call to 'find_if' may be resolved through ADL [bugprone-unintended-adl] + // CHECK-FIXES: {{^}} [&](auto &e) { return find_if(e, e, x); }); };{{$}} +} Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst @@ -0,0 +1,56 @@ +.. title:: clang-tidy - bugprone-unintended-adl + +bugprone-unintended-adl +======================= + +Finds usages of ADL (argument-dependent lookup), or potential ADL in the case +of templates, that are not on the provided lists of allowed identifiers and +namespaces. + +.. code-block:: c++ + + namespace aspace { + struct A {}; + void func(const A &); + } // namespace aspace + + namespace bspace { + void func(int); + void test() { + aspace::A a; + func(5); + func(a); // calls 'aspace::func' through ADL + } + } // namespace bspace + +(Example respectfully borrowed from `Abseil TotW #49 <https://abseil.io/tips/49>`_.) + +ADL can be surprising, and can lead to `subtle bugs +<https://bugs.llvm.org/show_bug.cgi?id=44398>`_ without the utmost attention. +However, it very is useful for lookup of overloaded operators, and for +customization points within libraries (e.g. ``swap`` in the C++ standard +library). As such, this check can be configured to ignore calls to overloaded +operators as well as other legitimate uses of ADL specified in a list of +allowed identifiers. + +This check does not suggest any fixes. + +Options +------- + +.. option:: IgnoreOverloadedOperators + + If non-zero, ignores calls to overloaded operators using operator syntax + (e.g. ``a + b``), but not function call syntax (e.g. ``operator+(a, b)``). + Default is `1`. + +.. option:: AllowedIdentifiers + + Semicolon-separated list of names that the check ignores. Default is + `swap;make_error_code;make_error_condition;data;begin;end;rbegin;rend;crbegin;crend;size;ssize;empty`. + +.. option:: AllowedNamespaces + + Semicolon-separated list of namespace names (e.g. `foo;bar::baz`). If the + check finds an unqualified call that resolves to a function in a namespace + in this list, the call is ignored. Default is an empty list. Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -82,6 +82,13 @@ Checks for usages of identifiers reserved for use by the implementation. +- New :doc:`bugprone-unintended-adl + <clang-tidy/checks/bugprone-unintended-adl>` check. + + Finds usages of ADL (argument-dependent lookup), or potential ADL in the case + of templates, that are not on the provided lists of allowed identifiers and + namespaces. + - New :doc:`cert-oop57-cpp <clang-tidy/checks/cert-oop57-cpp>` check. Index: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h @@ -0,0 +1,42 @@ +//===--- UnintendedADLCheck.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_UNINTENDEDADLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNINTENDEDADLCHECK_H + +#include "../ClangTidyCheck.h" +#include <string> +#include <vector> + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds usages of ADL (argument-dependent lookup), or potential ADL in the +/// case of templates, that are not on the provided lists of allowed identifiers +/// and namespaces. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unintended-adl.html +class UnintendedADLCheck : public ClangTidyCheck { + const bool IgnoreOverloadedOperators; + const std::vector<std::string> AllowedIdentifiers; + const std::vector<std::string> AllowedNamespaces; + +public: + UnintendedADLCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNINTENDEDADLCHECK_H Index: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp @@ -0,0 +1,211 @@ +//===--- UnintendedADLCheck.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 "UnintendedADLCheck.h" +#include "../utils/ASTUtils.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Tooling/FixIt.h" +#include <algorithm> + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static std::string getNestedNameSpecifierAsString(const NamedDecl &Decl) { + std::string N; + llvm::raw_string_ostream OS(N); + Decl.printNestedNameSpecifier(OS, Decl.getASTContext().getPrintingPolicy()); + return OS.str(); +} + +namespace { + +AST_MATCHER_P(CallExpr, ignoredOperator, bool, IgnoreOverloadedOperators) { + return IgnoreOverloadedOperators && isa<CXXOperatorCallExpr>(Node); +} + +AST_MATCHER(UnresolvedLookupExpr, requiresADL) { return Node.requiresADL(); } + +AST_MATCHER_P(UnresolvedLookupExpr, isSpelledAsAnyOf, std::vector<StringRef>, + Names) { + return llvm::find(Names, Node.getName().getAsString()) != Names.end(); +} + +AST_MATCHER(FunctionDecl, isHiddenFriend) { + return Node.getFriendObjectKind() == Decl::FOK_Undeclared; +} + +AST_MATCHER_P(FunctionDecl, isInAnyNamespace, std::vector<std::string>, + Namespaces) { + std::string NNS = getNestedNameSpecifierAsString(Node); + // Remove the `::` at the end. + assert(StringRef(NNS).endswith("::")); + NNS.erase(NNS.end() - 2, NNS.end()); + return llvm::find(Namespaces, NNS) != Namespaces.end(); +} + +AST_MATCHER(BinaryOperator, overloadableBinaryOperator) { + return Node.getOverloadedOperator(Node.getOpcode()) != OO_None; +} + +AST_MATCHER(UnaryOperator, overloadableUnaryOperator) { + return Node.getOverloadedOperator(Node.getOpcode()) != OO_None; +} + +} // namespace + +UnintendedADLCheck::UnintendedADLCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoreOverloadedOperators(Options.get("IgnoreOverloadedOperators", 1)), + AllowedIdentifiers(utils::options::parseStringList( + Options.get("AllowedIdentifiers", + "swap;make_error_code;make_error_condition;data;begin;" + "end;rbegin;rend;crbegin;crend;size;ssize;empty"))), + AllowedNamespaces(utils::options::parseStringList( + Options.get("AllowedNamespaces", ""))) {} + +void UnintendedADLCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoreOverloadedOperators", IgnoreOverloadedOperators); + Options.store(Opts, "AllowedIdentifiers", + utils::options::serializeStringList(AllowedIdentifiers)); + Options.store(Opts, "AllowedNamespaces", + utils::options::serializeStringList(AllowedNamespaces)); +} + +void UnintendedADLCheck::registerMatchers(MatchFinder *Finder) { + const std::vector<StringRef> AllowedIdentifierRefs(AllowedIdentifiers.begin(), + AllowedIdentifiers.end()); + Finder->addMatcher( + callExpr(usesADL(), unless(ignoredOperator(IgnoreOverloadedOperators)), + callee(functionDecl( + unless(anyOf(isHiddenFriend(), + hasAnyName(AllowedIdentifierRefs), + isInAnyNamespace(AllowedNamespaces)))) + .bind("ADLcallee"))) + .bind("ADLcall"), + this); + + Finder->addMatcher( + callExpr(unless(ignoredOperator(IgnoreOverloadedOperators)), + has(unresolvedLookupExpr( + requiresADL(), + unless(isSpelledAsAnyOf(AllowedIdentifierRefs))) + .bind("templateADLexpr"))) + .bind("templateADLcall"), + this); + if (!IgnoreOverloadedOperators) { + Finder->addMatcher( + binaryOperator(overloadableBinaryOperator(), isTypeDependent()) + .bind("templateBinaryOperator"), + this); + Finder->addMatcher( + unaryOperator(overloadableUnaryOperator(), isTypeDependent()) + .bind("templateUnaryOperator"), + this); + } +} + +static std::string getArgumentTypesAsString(const CallExpr &Call) { + auto Begin = Call.arg_begin(); + const auto End = Call.arg_end(); + assert(Begin != End); + std::string Result; + llvm::raw_string_ostream OS(Result); + OS << "'" << (*Begin)->getType().getAsString() << "'"; + for (++Begin; Begin != End; ++Begin) + OS << ", '" << (*Begin)->getType().getAsString() << "'"; + return Result; +} + +static bool IsCallInTemplateInstantiation(const CallExpr &Call, + ASTContext &Context) { + return !match(isInTemplateInstantiation(), Call, Context).empty(); +} + +static FixItHint GetFixIt(const CallExpr &Call, StringRef CalleeQualName, + ASTContext &Context) { + if (isa<CXXOperatorCallExpr>(Call)) { + std::string RewrittenCall = (CalleeQualName + "(").str(); + auto ArgIt = Call.arg_begin(); + const auto End = Call.arg_end(); + assert(ArgIt != End); + RewrittenCall += tooling::fixit::getText(**ArgIt, Context); + for (++ArgIt; ArgIt != End; ++ArgIt) { + RewrittenCall += ", "; + RewrittenCall += tooling::fixit::getText(**ArgIt, Context); + } + RewrittenCall += ')'; + return FixItHint::CreateReplacement(Call.getSourceRange(), RewrittenCall); + } + return FixItHint::CreateReplacement(Call.getCallee()->getSourceRange(), + CalleeQualName); +} + +static bool OnlyDependsOnFundamentalArraySizes(QualType QualTy) { + if (!QualTy->isDependentType() || QualTy->isBuiltinType()) + return true; + if (const auto *DependentTy = dyn_cast<DependentSizedArrayType>(QualTy)) + return OnlyDependsOnFundamentalArraySizes(DependentTy->getElementType()); + return false; +} + +void UnintendedADLCheck::check(const MatchFinder::MatchResult &Result) { + // Diagnose concrete uses of ADL. + if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("ADLcall")) { + const auto *Callee = Result.Nodes.getNodeAs<FunctionDecl>("ADLcallee"); + std::string CalleeQualName = Callee->getQualifiedNameAsString(); + bool InTemplateInstantiation = + IsCallInTemplateInstantiation(*Call, *Result.Context); + + auto Diag = [&] { + return diag(Call->getBeginLoc(), "expression calls '%0' through ADL") + << CalleeQualName; + }; + if (!InTemplateInstantiation) + Diag() << GetFixIt(*Call, CalleeQualName, *Result.Context); + else { + Diag(); + diag(Call->getBeginLoc(), "with argument type%s0 %1", DiagnosticIDs::Note) + << Call->getNumArgs() << getArgumentTypesAsString(*Call); + } + return; + } + + // Diagnose possible uses of ADL in templates. + auto Diag = [this](SourceLocation Loc, StringRef Name) { + diag(Loc, "unqualified call to '%0' may be resolved through ADL") << Name; + }; + if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("templateADLcall")) { + // These are false positives. + if (llvm::all_of(Call->arguments(), [](const Expr *Arg) { + return OnlyDependsOnFundamentalArraySizes(Arg->getType()); + })) + return; + + const auto *Lookup = + Result.Nodes.getNodeAs<UnresolvedLookupExpr>("templateADLexpr"); + assert(Lookup && + "'templateADLcall' matcher must bind associated unresolved lookup"); + Diag(Call->getBeginLoc(), Lookup->getName().getAsString()); + } else if (const auto *OpExpr = Result.Nodes.getNodeAs<BinaryOperator>( + "templateBinaryOperator")) + Diag(OpExpr->getBeginLoc(), ("operator" + OpExpr->getOpcodeStr()).str()); + else if (const auto *OpExpr = + Result.Nodes.getNodeAs<UnaryOperator>("templateUnaryOperator")) + Diag(OpExpr->getBeginLoc(), + ("operator" + OpExpr->getOpcodeStr(OpExpr->getOpcode())).str()); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang 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 @@ -48,6 +48,7 @@ UndefinedMemoryManipulationCheck.cpp UndelegatedConstructorCheck.cpp UnhandledSelfAssignmentCheck.cpp + UnintendedADLCheck.cpp UnusedRaiiCheck.cpp UnusedReturnValueCheck.cpp UseAfterMoveCheck.cpp 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 @@ -56,6 +56,7 @@ #include "UndefinedMemoryManipulationCheck.h" #include "UndelegatedConstructorCheck.h" #include "UnhandledSelfAssignmentCheck.h" +#include "UnintendedADLCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" @@ -162,6 +163,8 @@ "bugprone-undelegated-constructor"); CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>( "bugprone-unhandled-self-assignment"); + CheckFactories.registerCheck<UnintendedADLCheck>( + "bugprone-unintended-adl"); CheckFactories.registerCheck<UnusedRaiiCheck>( "bugprone-unused-raii"); CheckFactories.registerCheck<UnusedReturnValueCheck>(
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits