[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Eric Fiselier via Phabricator via cfe-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.



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



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27
+class UnintendedADLCheck : public ClangTidyCheck {
+  const bool IgnoreOverloadedOperators;
+  const std::vector<std::string> AllowedIdentifiers;
----------------
I think we should always ignore operators. I don't see value in having a mode where every comparison triggers this warning.



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:54
+  move(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl]
+  forward(a);
----------------
JonasToth wrote:
> logan-5 wrote:
> > Quuxplusone wrote:
> > > 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.
> > 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.
> 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.
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:202
+void macro_test(T t) {
+#define MACRO(x) find_if(x, x, x)
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72282/new/

https://reviews.llvm.org/D72282