logan-5 updated this revision to Diff 243307.
logan-5 marked an inline comment as done.
logan-5 added a comment.
Changed `Whitelist` to `AllowedIdentifiers`.
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,213 @@
+// 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 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]
+}
+
+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]
+
+ 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,49 @@
+.. 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 list of allowed identifiers.
+
+.. 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`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -81,6 +81,12 @@
<clang-tidy/checks/bugprone-reserved-identifier>` check.
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 list of allowed identifiers.
- 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,41 @@
+//===--- 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 list of allowed
+/// identifiers.
+///
+/// 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;
+
+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,118 @@
+//===--- 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)),
+ AllowedIdentifiers(utils::options::parseStringList(
+ Options.get("AllowedIdentifiers",
+ "swap;make_error_code;make_error_condition;data;begin;"
+ "end;rbegin;rend;crbegin;crend;size;ssize;empty"))) {}
+
+void UnintendedADLCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IgnoreOverloadedOperators", IgnoreOverloadedOperators);
+ Options.store(Opts, "AllowedIdentifiers",
+ utils::options::serializeStringList(AllowedIdentifiers));
+}
+
+void UnintendedADLCheck::registerMatchers(MatchFinder *Finder) {
+ const std::vector<StringRef> AllowedIdentifierRefs(AllowedIdentifiers.begin(),
+ AllowedIdentifiers.end());
+ Finder->addMatcher(
+ callExpr(usesADL(), unless(ignoredOperator(IgnoreOverloadedOperators)),
+ callee(functionDecl(unless(hasAnyName(AllowedIdentifierRefs)))
+ .bind("ADLcallee")))
+ .bind("ADLcall"),
+ this);
+
+ Finder->addMatcher(
+ callExpr(unless(ignoredOperator(IgnoreOverloadedOperators)),
+ has(unresolvedLookupExpr(
+ requiresADL(),
+ unless(isSpelledAsAnyOf(AllowedIdentifierRefs)))
+ .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
@@ -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
