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

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43
+      Whitelist(
+          utils::options::parseStringList(Options.get("Whitelist", "swap"))) {}
+
----------------
Quuxplusone wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > logan-5 wrote:
> > > > JonasToth wrote:
> > > > > 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.
> > > > 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.
> > > 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 :)
> > 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.
> +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](http://eel.is/c++draft/iterator.range#1.sentence-2): `data`, `begin`, `end`, `rbegin`, `rend`, `crbegin`, `crend`, `size`, `ssize`, and `empty`. This might deserve input from the libc++ implementors.
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.


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


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:61
+void templateFunction(T t) {
+  swap(t, t);
+
----------------
Quuxplusone wrote:
> 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?
> 
I believe this addressed by my juggling the tests around a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282