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

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked 2 inline comments as done.
logan-5 added a comment.

Thanks for the feedback.

In D72282#1918253 <https://reviews.llvm.org/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.



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


================
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)
+
----------------
EricWF wrote:
> 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?
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282