[clang-tidy] Add `bugprone-unintended-adl`

[logan-5]

Summary

This patch adds bugprone-unintended-adl which flags uses of ADL that are not on the provided whitelist.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

14

Unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst

7

Please follow 80 character length limit. Same below.

27

Please use double back-ticks for swap (or std::swap?).

36

Indentation. Please use double back-ticks for a + b and operator+(a, b).

40

Indentation. Please use double back-ticks for swap (or std::swap?).

[logan-5]

Addressed some formatting stuff.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst

36

I'm not sure what you would like me to change about the indentation. This follows the same format (three spaces) I've found in the documentation for other checks.

[logan-5]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst

40

Since the 'swap' here is referring to the default value of the option string, shouldn't it just be single backticks?

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst

40

It was about swap above, not about option default value.

[JonasToth]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

do you mean std::swap? If you it should be fully qualified.
Doesn't std::error_code rely on adl, too? I think std::cout << and other streams of the STL rely on it too, and probably many more code-constructs that are commonly used.

That means, the list should be extended to at least all standard-library facilities that basically required ADL to work. And then we need data on different code bases (e.g. LLVM is a good start) how much noise gets generated.

[logan-5]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

I distinctly don't mean std::swap -- I want to whitelist any unqualified function call spelled simply swap.

Overloaded operators are the poster child for ADL's usefulness, so that's why this check has a special default-on IgnoreOverloadedOperators option. That whitelists a whole ton of legitimate stuff including std::cout << x and friends.

I don't see a ton of discussion online about error_code/make_error_code and ADL being very much intertwined. I'm not particularly familiar with those constructs myself though, and I could just be out of the loop. I do see a fair number of unqualified calls to make_error_code within LLVM, though most of those resolve to llvm::make_error_code, the documentation for which says it exists because std::make_error_code can't be reliably/portably used with ADL. That makes me think make_error_code would belong in LLVM's project-specific whitelist configuration, not the check's default.

Speaking of which, I did run this check over LLVM while developing and found it not particularly noisy as written. That is, it generated a fair number of warnings, but only on constructs that, when examined closely, were a little suspicious or non-obvious.

[logan-5]

Clean up some false positives in macro expansions and in expressions with nested potential ADL usages (e.g. in lambdas passed as function parameters).

[JonasToth]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

I don't have a solid understanding of the error_code world as well. All I know is, that you specialize some templates with your own types in order to use the generic error_code-world.
AFAIK that needs some form of ADL at some point, but that could even happen through the overloaded operators (== and !=), in which case that would already be handled. (maybe @aaron.ballman knows more?)

But overloaded operators being ignored by default is good and that point is gone :)

84

please add a message to the assertion to describe why you are asserting this condition, to better understand the cause of the error, e.g. assert(Call && "Matcher should only match for two distinct cases"); or something like this.

90

Can't you just bind directly to the unresolvedExpr?

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

2

why 14 or later? ADL exists in the prior standards, too.

Additionally we need a test for where overloaded operators are not ignored and create the warnings.

30

Templated overloaded operators should be tested, too, e.g.:

template <class IStream>
OStream &operator>>(IStream& is, MyClass foo);

61

This function is not instantiated right now, is it?
Can you please write a function that would resolve to ADL for one instantiation and wouldn't for another one?
That should create multiple diagnostics for the same source-location and would be confusing.

[logan-5]

@JonasToth Thanks for the feedback. Will be updating the diff in the next day or so.

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

90

I need the "templateADLCall" node for the diagnostic caret, and the "templateADLexpr" for the name/spelling of the call. I might totally be misunderstanding what you're suggesting here.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

2

14 or later just because of the generic lambdas in some of the tests. Is it worth separating those tests out into their own files so that we don't have to pass this flag here?

[JonasToth]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

90

Ah, i overlooked that bit. Then The Lookup should be asserted as well, to ensure the matchers works in all circumstances. (future refactorings included)

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

2

Yes. Usually we have the "base"-version of c++ for most of the test, that are common and extra test-files for newer features or requirements.

[JonasToth]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

Yes, make_error_code is used via ADL. --> https://www.boost.org/doc/libs/1_72_0/libs/outcome/doc/html/motivation/plug_error_code.html
I think that should be in the default list for ignored functions, as it is a standard facility.

[• Quuxplusone]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

+1, both make_error_code and make_error_condition should be on the whitelist. (I am the author of [P0824 "Summary of SG14 discussion of <system_error>"](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0824r1.html#best-arthur). I also confirm that libc++ <system_error> does call them unqualified on purpose.)

I would like to see some discussion and/or TODO-comments about the other standard designated customization points: data, begin, end, rbegin, rend, crbegin, crend, size, ssize, and empty. This might deserve input from the libc++ implementors.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

55

This is awesome. :) Please also consider ADL where there's more than one associated namespace, something like this: https://godbolt.org/z/S73Gzy

template<class T> struct Templated { };
Templated<std::byte> testX() {
    Templated<std::byte> x;
    using std::move;
    return move(x);  // "correctly" resolves to std::move today, but still does unintended ADL
}

Please add a test isomorphic to the above, unless you think it's already covered by one of the existing tests.

62

This is not the idiomatic way of calling swap: there is no ADL swap for int, for example (so templateFunction<int> will hard-error during instantiation). It would probably be scope-creep to try to handle the "std::swap two-step", but can you leave a TODO comment somewhere to revisit this issue?

70

Ah, this is subtle. Lookup on ref finds the global variable ref and therefore doesn't do ADL. But if global variable ref didn't exist, then this would warn: std::ref is not an ADL customization point.

Could you either leave a explanatory comment here, or (if the subtle confusion with std::ref is not germane to the test case) s/ref/foo/?

[• Quuxplusone]

Repeating for emphasis: This is awesome. :)

Having the opt-in option to not-ignore overloaded operators is good; please keep that option (in case anyone tries to talk you into removing it).

For more than you wanted to know about ADL, why it's bad for library writers, and why a compiler check is a great idea, see https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/ (It ends with a secret bonus test case, where certain unqualified infix operators do not use ADL even though they look like they should.)

[logan-5]

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).

[logan-5]

Hi @Quuxplusone, glad you found your way here. I thought of adding you as a reviewer out the gate but then I didn't.

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

44

Added make_error_condition to the whitelist.

My inclination would be to just add all those standard customization points to the default whitelist. Users can easily supply a smaller whitelist if they want.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

55

It's interesting, that code only triggers the check (i.e. my AST matchers only think it's doing ADL) without the using std::move. I admit I'm a bit confused as to why.

62

I believe this addressed by my juggling the tests around a bit.

[JonasToth]

@Quuxplusone thank you for the input! I think it is a good idea if the library-implementors take a look at it. I myself don't have enough knowledge of C++ and the libraries to judge all this stuff.
So who to ping or to add as reviewer?

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

55

The matcher itself only ask the CallExpr->usesADL() (not sure about the method name right now, but basically that). So the reason is probably hidden somewhere in Sema, if the matcher is the issue.

[• Quuxplusone]

Re which libc++ folks could give feedback on this ADL-diagnosing patch: I don't know precisely, but the candidates are few! @mclow.lists @EricWF @ldionne @zoecarver @CaseyCarter.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

62

Juggling the tests around doesn't address the fact that any code that does swap(a,b) without doing using std::swap; first (or begin(a) without using std::begin;) is almost certainly broken for primitive types.

My naive thought is that you would not do using std::make_error_code; because make_error_code is definitely never going to be used with primitive types. So "functions okay to call via ADL" and "functions that require the std::swap two-step" actually are slightly different whitelists.

I was saying that although this issue is probably out-of-scope for what you're doing in this patch, still, it would be nice to leave a TODO somewhere. ...Or maybe you say "nah, that's so far out of scope I don't want to think about it, and it may never get done, so even leaving a TODO is inappropriate."

[logan-5]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

62

Thanks for clarifying. I do think that is out of scope for my goals for this patch, but I think a TODO comment is reasonable.

[logan-5]

Added TODO comment.

[logan-5]

Rebased with trunk. Updated whitelist to include more standard designated customization points.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

28

I'd prefer a more descriptive name for this (and the user-facing option + documentation). How about AllowedIdentifiers or ADLEnabledIdentifiers or some such?

[logan-5]

Changed Whitelist to AllowedIdentifiers.

[aaron.ballman]

LGTM, but I think it would be helpful if someone from the libc++ side also double-checked the behavior of patch if possible. I've added a couple potential reviewers, but they may be busy with wg21 meetings this week. If we don't hear anything back from them in a few weeks, then I think the patch can go in and we can handle issues post-commit.

[logan-5]

Just giving this a friendly ping!

[EricWF]

I'll be picking this up seriously this week.
I'm currently getting an internal version of this clang-tidy reviewed,
and my plan was to submit it upstream after because I didn't see this
patch until now (bad email filters).

I think we can unify the two checks,

Thanks for working on this.

[EricWF]

• This check should suggest fixes. It knows what function is actually being resolved, and it has enough information to create a minimally qualified name that resolves it.
• This check should ignore hidden friends, since their entire purpose is to be called via ADL.
• This check should allow whitelisting namespaces that opt-out of ADL into their namespace. This makes it much easier to roll out the clang-tidy incrementally.

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

28

I think we should always ignore operators. I don't see value in having a mode where every comparison triggers this warning.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

55

A call expression only "usesADL" if the callee was found only through ADL lookup. If it's found through normal lookup, then it doesn't "use adl", even though ADL lookup is also performed and that ADL lookup considers std::move.

I've been considering adding CallExpr::considersADL, but I'm not convinced it's needed.
There's more than enough work to be done cleaning up call's that depend on ADL.

203

Arguably this is *exactly* the kind of code we want to diagnose.

The call in the macro either,

• Is a "customization point" and should be whitelisted. Or,
• It resolves the same in expansion (and can be qualified), Or,
• It is a bug.

You mentioned false positives in things like assert. Can you provide examples?

[• Quuxplusone]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

28

I think there's value in that mode, for library writers (not libc++) who really care about finding every unintentional ADL in their whole library. The standard library is designed not-to-work with types like [Holder<Incomplete>](https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/), but someone else's library might be designed to work even in that case, and then they'd want to hear about ADL lookups for things like operator,. Besides, it's just 1 extra line of code in the patch, isn't it?

However, I now think I may not understand how this check works. I thought it looked for unqualified calls (even in templates) that "may" use ADL, but now that I look again at the tests, it seems to trigger only on concrete calls (in concrete template instantiations) that "do" use ADL, which sounds still useful but much less comprehensive than I had thought.

I think it would catch

template<class T> void foo(T t) { t, 0; }
struct S { friend void operator,(S, int); };
template void foo(S);

but not

template<class T> void foo(T t) { t, 0; }
struct S { friend void operator,(S, int); };
template void foo(int);

or

template<class T> void foo(T t) { t, 0; }

is that right?

[logan-5]

Thanks for the feedback.

In D72282#1918253, @EricWF wrote:

• This check should suggest fixes. It knows what function is actually being resolved, and it has enough information to create a minimally qualified name that resolves it.

True, though of course it will only be able to do so for the non-template version of the warning, since in the template version it only knows that the call _may_ be resolved through ADL.

• This check should ignore hidden friends, since their entire purpose is to be called via ADL.
• This check should allow whitelisting namespaces that opt-out of ADL into their namespace. This makes it much easier to roll out the clang-tidy incrementally.

I want to make absolutely sure I understand the last bullet. You'd like a whitelist of namespaces where, if a call is resolved by ADL to a function within any of those namespaces, the check doesn't fire?

I like all these suggestions. I'll work on an updated patch.

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

28

@Quuxplusone your initial understanding was right; the check fires on both templates that "may" use ADL as well as concrete instantiations that "do." There are tests for both, but I see now that I failed to add tests for the "may" case for operators, which I'll do.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

203

Fair enough. Disabling the check for macros does seem short sighted on closer thought.

When I run the check over LLVM in debug, assert expands to (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0). If the assert() is inside a function template, the check claims that unqualified call to '__assert_rtn' may be resolved through ADL. Inspecting the AST, this seems to be due to the fact that __func__ has dependent type. I suppose __func__ could be special cased to be ignored, or all uglified names, or something?

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

203

Ouch, is that because __func__ is an array of char with dependent length?

template<class T>
void foo() {
    char buf[sizeof(T)];
    memset(buf, '\0', sizeof buf);
}

Unqualified call to memset, where one of the arguments has dependent type char[sizeof(T)] — does that trigger the diagnostic?

[logan-5]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

203

Yep. So, I suppose the check should not trigger for expressions of DependentSizedArrayType of char? Or of any built-in type, really?

[logan-5]

The check now suggests fixes for concrete (i.e. non-template) uses of ADL. Re-enabled the check for macros, and eliminated some false positives with arrays of dependent size. The check now ignores hidden friends. Added a namespace whitelist. Added some tests and ensured that "don't-ignore-overloaded-operators" mode works properly in templates.

[• Quuxplusone]

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

52

As long as you're asserting anyway, I think you should assert that the last two characters are ::.

160

Never mind me if this becomes too ugly, but, I would have expected bool OnlyDependsOnFundamentalArraySizes(const Expr *) to be recursive, and I would have expected the llvm::all_of-over-a-parameter-list to happen in the caller. To properly handle corner cases like

template<class T> void test() {
    char a[sizeof(T)][sizeof(T)]; foo(a);
}

Experimentally, it appears that Clang thinks that the type of char b[sizeof(T)]; ... (b+1) ... is dependent. (It's definitely char*, but it comes from applying + to an expression with dependent type.) I have no idea what the paper standard says about it.

[logan-5]

Re-juggled and made bool OnlyDependsOnFundamentalArraySizes(QualType QualTy) recursive. Made :: assert more useful. Thanks @Quuxplusone for both good ideas.

[logan-5]

Pinging this. I believe all feedback from @EricWF was addressed back in my update to the patch in March.

[FelixDombek]

@EricWF requested changes which have since been added, but he has been inactive for half a year; what is the process to go forward with this?

[logan-5]

It's been a year and a half since I heard anything about this patch and I'd honestly kind of forgotten about it. I'm still around/lurking though, and would assist with getting it landed if it's still good to go.

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

28

Oops: this should maybe be OS.flush(); return N; so that N is NRVO'd or else implicit-moved. return OS.str(); does a copy.

128

I can't imagine it super matters for raw_string_ostream, but I think pedantically this needs an OS.flush() before the return.

[fx-carton]

Great check!

Would it be possible to add an option to show the warning if a using directive is used? As in:

namespace ns {
    struct s {};
    void func(const s&) {
    }
}

using ns::s;
using ns::func; // without this the check is triggered

int main(void) {
    s t;
    func(t);
    return 0;
}

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
+#include "UnintendedADLCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "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>(
         "bugprone-unused-return-value");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnhandledSelfAssignmentCheck.cpp
+  UnintendedADLCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyCppCoreGuidelinesModule
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp

+//===--- 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

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-reserved-identifier
 
   Checks for usages of identifiers reserved for use by the C++ implementation. The C++ standard reserves the following names for such use:
   * identifiers with a double underscore anywhere;
   * identifiers that begin with an underscore followed by an uppercase letter;
   * identifiers in the global namespace that begin with an underscore.
   
   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.
 
   Checks if an object of type with extended alignment is allocated by using
   the default ``operator new``.
 
 - New alias :doc:`cert-pos44-c
   <clang-tidy/checks/cert-pos44-c>` to

clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst

+.. 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`.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-generic-lambdas.cpp

+// 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]
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl-operators.cpp

+// 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]
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

+// 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() {
+  // 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);
+  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
+}