Page MenuHomePhabricator

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

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

logan-5 created this revision.Jan 6 2020, 9:06 AM
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>". 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.Mon, Jan 27, 3:01 PM

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

aaron.ballman added inline comments.Tue, Jan 28, 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.Fri, Feb 7, 3:49 PM
logan-5 marked an inline comment as done.

Changed Whitelist to AllowedIdentifiers.

aaron.ballman accepted this revision.Tue, Feb 11, 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.Tue, Feb 11, 12:44 AM