logan-5 updated this revision to Diff 236884. logan-5 marked 15 inline comments as done. logan-5 added a comment.
Beefed up tests and split into separate files. Added tests for instantiations of template functions that //do// definitely result in ADL, and made the warning messages more descriptive in those cases (listing out concrete types of arguments in a note). Repository: rG LLVM Github Monorepo 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,160 @@ +// RUN: %check_clang_tidy %s bugprone-unintended-adl %t + +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] +} +} // 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] + ops::stream << aspace::A(); + // no warning + operator<<(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + + ops::stream >> aspace::A(); + // no warning + operator>>(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + + smooth_operator(ops::stream); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl] +} + +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 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); + +// non-whitelisted +void move(Swappable); +void ref(Swappable); + +struct Swappable2 {}; + +} // namespace ns +struct { + template <typename T> + void operator()(T &&); +} ref; + +void test2() { + using namespace std; + ns::Swappable a, b; + swap(a, b); + make_error_code(a); + make_error_condition(a); + move(a); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl] +} + +template <typename T> +void foo(T t) { + using namespace std; + swap(t, t); + make_error_code(t); + make_error_condition(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] + + 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] +} + +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); + +#undef MACRO + +#define MACRO(x) func(x) + + MACRO(aspace::A()); + +#undef MACRO +} 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,49 @@ +// 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] + operator<<(ops::stream, 5); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + ops::stream << aspace::A(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + operator<<(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl] + + ops::stream >> aspace::A(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + operator>>(ops::stream, aspace::A()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl] + + smooth_operator(ops::stream); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' 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,52 @@ +// 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] + 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] + 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] +} 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,48 @@ +.. 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 whitelist. + +.. 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 whitelist. + +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:: Whitelist + + Semicolon-separated list of names that the check ignores. Default is + `swap;make_error_code;make_error_condition`. Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,12 @@ Violating the naming rules above results in undefined behavior. +- 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 whitelist. + - New :doc:`cert-mem57-cpp <clang-tidy/checks/cert-mem57-cpp>` check. Index: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h @@ -0,0 +1,40 @@ +//===--- 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 whitelist. +/// +/// 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> Whitelist; + +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,115 @@ +//===--- 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 <algorithm> + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +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 std::any_of(Names.begin(), Names.end(), + [LookupName = Node.getName().getAsString()]( + StringRef Name) { return Name == LookupName; }); +} + +} // namespace + +UnintendedADLCheck::UnintendedADLCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoreOverloadedOperators(Options.get("IgnoreOverloadedOperators", 1)), + Whitelist(utils::options::parseStringList(Options.get( + "Whitelist", "swap;make_error_code;make_error_condition"))) {} + +void UnintendedADLCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoreOverloadedOperators", IgnoreOverloadedOperators); + Options.store(Opts, "Whitelist", + utils::options::serializeStringList(Whitelist)); +} + +void UnintendedADLCheck::registerMatchers(MatchFinder *Finder) { + const std::vector<StringRef> WhitelistRefs(Whitelist.begin(), + Whitelist.end()); + Finder->addMatcher( + callExpr(usesADL(), unless(ignoredOperator(IgnoreOverloadedOperators)), + callee(functionDecl(unless(hasAnyName(WhitelistRefs))) + .bind("ADLcallee"))) + .bind("ADLcall"), + this); + + Finder->addMatcher( + callExpr(unless(ignoredOperator(IgnoreOverloadedOperators)), + has(unresolvedLookupExpr(requiresADL(), + unless(isSpelledAsAnyOf(WhitelistRefs))) + .bind("templateADLexpr"))) + .bind("templateADLcall"), + this); +} + +static std::string getArgumentTypesAsString(const CallExpr *const 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; +} + +void UnintendedADLCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("ADLcall")) { + // Ignore matches in macros. This avoids large numbers of false positives in + // e.g. assert(). Same below. + if (Call->getBeginLoc().isMacroID()) + return; + + const auto *Callee = Result.Nodes.getNodeAs<FunctionDecl>("ADLcallee"); + diag(Call->getBeginLoc(), "expression calls '%0' through ADL") + << Callee->getQualifiedNameAsString(); + + if (!match(isInTemplateInstantiation(), *Call, *Result.Context).empty()) + diag(Call->getBeginLoc(), "with argument type%s0 %1", DiagnosticIDs::Note) + << Call->getNumArgs() << getArgumentTypesAsString(Call); + } else if (const auto *Call = + Result.Nodes.getNodeAs<CallExpr>("templateADLcall")) { + // Ignore matches in macros. + if (Call->getBeginLoc().isMacroID()) + return; + + const auto *Lookup = + Result.Nodes.getNodeAs<UnresolvedLookupExpr>("templateADLexpr"); + assert(Lookup && + "'templateADLcall' matcher must bind associated unresolved lookup"); + diag(Call->getBeginLoc(), + "unqualified call to '%0' may be resolved through ADL") + << Lookup->getName().getAsString(); + } +} + +} // 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 @@ -46,6 +46,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 @@ -54,6 +54,7 @@ #include "UndefinedMemoryManipulationCheck.h" #include "UndelegatedConstructorCheck.h" #include "UnhandledSelfAssignmentCheck.h" +#include "UnintendedADLCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" @@ -156,6 +157,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