Page MenuHomePhabricator

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

Authored by logan-5 on Jan 6 2020, 9:06 AM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 9:06 AM
Eugene.Zelenko added inline comments.
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 updated this revision to Diff 236420.Jan 6 2020, 10:55 AM
logan-5 marked 5 inline comments as done.

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.

40

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

Eugene.Zelenko added inline comments.Jan 6 2020, 11:03 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
40

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

JonasToth added inline comments.
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 marked an inline comment as done.Jan 6 2020, 2:15 PM
logan-5 added inline comments.
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 updated this revision to Diff 236593.Jan 7 2020, 8:01 AM

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 added inline comments.Jan 7 2020, 9:08 AM
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 marked 2 inline comments as done.Jan 7 2020, 4:55 PM

@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 added inline comments.Jan 8 2020, 1:58 AM
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 added inline comments.Jan 8 2020, 7:09 AM
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 added inline comments.
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/?

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 updated this revision to Diff 236884.Jan 8 2020, 12:29 PM
logan-5 marked 15 inline comments as done.

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

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.

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

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 marked an inline comment as done.Jan 9 2020, 10:41 AM
logan-5 added inline comments.
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 updated this revision to Diff 238328.Jan 15 2020, 11:51 AM

Added TODO comment.

logan-5 marked 4 inline comments as done.Jan 15 2020, 11:52 AM
logan-5 updated this revision to Diff 240691.Jan 27 2020, 3:01 PM

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

aaron.ballman added inline comments.Jan 28 2020, 5:30 AM
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 updated this revision to Diff 243307.Feb 7 2020, 3:49 PM
logan-5 marked an inline comment as done.

Changed Whitelist to AllowedIdentifiers.

aaron.ballman accepted this revision.Feb 11 2020, 12:44 AM
aaron.ballman added reviewers: mclow.lists, EricWF.

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.

This revision is now accepted and ready to land.Feb 11 2020, 12:44 AM

Just giving this a friendly ping!

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 requested changes to this revision.Mar 11 2020, 3:35 PM
  • 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?

This revision now requires changes to proceed.Mar 11 2020, 3:35 PM
ldionne removed a subscriber: ldionne.Mar 11 2020, 3:40 PM
Quuxplusone added inline comments.Mar 11 2020, 5:26 PM
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 marked 2 inline comments as done.Mar 11 2020, 7:35 PM

Thanks for the feedback.

  • 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 added inline comments.Mar 11 2020, 7:46 PM
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 marked an inline comment as done.Mar 11 2020, 8:03 PM
logan-5 added inline comments.
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 updated this revision to Diff 250130.Mar 12 2020, 10:41 PM
logan-5 marked 3 inline comments as done.

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 added inline comments.Mar 13 2020, 11:01 AM
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 updated this revision to Diff 250272.Mar 13 2020, 11:55 AM
logan-5 marked 2 inline comments as done.

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

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