diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/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( "bugprone-unhandled-self-assignment"); + CheckFactories.registerCheck( + "bugprone-unintended-adl"); CheckFactories.registerCheck( "bugprone-unused-raii"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h new file mode 100644 --- /dev/null +++ b/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 +#include + +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 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 diff --git a/clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp @@ -0,0 +1,89 @@ +//===--- 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/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +namespace { + +AST_MATCHER_P(CallExpr, ignoredOperator, bool, IgnoreOverloadedOperators) { + return IgnoreOverloadedOperators && isa(Node); +} + +AST_MATCHER(UnresolvedLookupExpr, requiresADL) { return Node.requiresADL(); } + +AST_MATCHER_P(UnresolvedLookupExpr, isSpelledAsAnyOf, std::vector, + 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"))) {} + +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 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)), + hasDescendant( + unresolvedLookupExpr(requiresADL(), + unless(isSpelledAsAnyOf(WhitelistRefs))) + .bind("templateADLexpr"))) + .bind("templateADLcall"), + this); +} + +void UnintendedADLCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Call = Result.Nodes.getNodeAs("ADLcall")) { + const auto *Callee = Result.Nodes.getNodeAs("ADLcallee"); + diag(Call->getBeginLoc(), "expression calls '%0' through ADL") + << Callee->getQualifiedNameAsString(); + } else { + Call = Result.Nodes.getNodeAs("templateADLcall"); + assert(Call); + const auto *Lookup = + Result.Nodes.getNodeAs("templateADLexpr"); + diag(Call->getBeginLoc(), + "unqualified call to '%0' may be resolved through ADL") + << Lookup->getName().getAsString(); + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,12 @@ Violating the naming rules above results in undefined behavior. +- New :doc:`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 ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst @@ -0,0 +1,47 @@ +.. 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 `_.) + +ADL can be surprising, and can lead to `subtle bugs +`_ 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`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp @@ -0,0 +1,75 @@ +// 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; +} +void smooth_operator(Stream); +} // namespace ops + +void test() { + ops::stream << 5; + operator<<(ops::stream, 5); + // 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 ns { +struct Swappable {}; +void swap(Swappable &a, Swappable &b); // whitelisted by default + +void move(Swappable) {} // non-whitelisted +template +void forward(T) {} // non-whitelisted + +struct Swappable2 {}; +} // namespace ns + +void move(ns::Swappable2); +auto ref = [](ns::Swappable2) {}; + +void test2() { + ns::Swappable a, b; + using namespace std; + swap(a, b); + move(a); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl] + forward(a); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::forward' through ADL [bugprone-unintended-adl] +} + +template +void templateFunction(T t) { + swap(t, t); + + move(t); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'move' may be resolved through ADL [bugprone-unintended-adl] + + ns::move(t); + ::move(t); + + ref(t); + ::ref(t); + + t << 5; + operator<<(t, 5); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'operator<<' may be resolved through ADL [bugprone-unintended-adl] +}