This patch adds bugprone-unintended-adl which flags uses of ADL that are not on the provided whitelist.
Details
Diff Detail
Event Timeline
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?). |
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. |
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst | ||
---|---|---|
40 | Since the 'swap' here is referring to the default value of the option string, shouldn't it just be single backticks? |
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst | ||
---|---|---|
40 | It was about swap above, not about option default value. |
clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp | ||
---|---|---|
44 | do you mean std::swap? If you it should be fully qualified. 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. |
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. |
Clean up some false positives in macro expansions and in expressions with nested potential ADL usages (e.g. in lambdas passed as function parameters).
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. 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? |
@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? |
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. |
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 |
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.)
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." |
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. |
Rebased with trunk. Updated whitelist to include more standard designated customization points.
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? |
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.
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.
- 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. | |
203 | Arguably this is *exactly* the kind of code we want to diagnose. The call in the macro either,
You mentioned false positives in things like assert. Can you provide examples? |
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? |
Thanks for the feedback.
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? |
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? |
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? |
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.
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. |
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.
@EricWF requested changes which have since been added, but he has been inactive for half a year; what is the process to go forward with this?
It's been a year and a half since I heard anything about this patch and I'd honestly kind of forgotten about it. I'm still around/lurking though, and would assist with getting it landed if it's still good to go.
clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp | ||
---|---|---|
27 | Oops: this should maybe be OS.flush(); return N; so that N is NRVO'd or else implicit-moved. return OS.str(); does a copy. | |
127 | I can't imagine it super matters for raw_string_ostream, but I think pedantically this needs an OS.flush() before the return. |
Great check!
Would it be possible to add an option to show the warning if a using directive is used? As in:
namespace ns { struct s {}; void func(const s&) { } } using ns::s; using ns::func; // without this the check is triggered int main(void) { s t; func(t); return 0; }
Unnecessary empty line.