Page MenuHomePhabricator
Differential D72282 Diff 240691 clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

Changeset View

Standalone View

View Options

clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

  • This file was added.
// RUN: %check_clang_tidy %s bugprone-unintended-adl %t
JonasTothUnsubmitted
Done
ReplyInline Actions

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.

JonasToth: why 14 or later? `ADL` exists in the prior standards, too. Additionally we need a test for…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

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?

logan-5: 14 or later just because of the generic lambdas in some of the tests. Is it worth separating…
JonasTothUnsubmitted
Done
ReplyInline Actions

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: Yes. Usually we have the "base"-version of c++ for most of the test, that are common and extra…
namespace aspace {
struct A {};
void func(const A &);
} // namespace aspace
namespace bspace {
void func(int);
void test() {
aspace::A a;
func(5);
func(a);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'aspace::func' through ADL [bugprone-unintended-adl]
}
} // namespace bspace
namespace ops {
struct Stream {
} stream;
Stream &operator<<(Stream &s, int) {
return s;
}
Stream &operator<<(Stream &s, aspace::A) {
return s;
}
template <typename IStream>
IStream &operator>>(IStream &s, int) {
return s;
JonasTothUnsubmitted
Done
ReplyInline Actions

Templated overloaded operators should be tested, too, e.g.:

template <class IStream>
OStream &operator>>(IStream& is, MyClass foo);
JonasToth: Templated overloaded operators should be tested, too, e.g.: ``` template <class IStream>…
}
template <typename IStream>
IStream &operator>>(IStream &s, aspace::A) {
return s;
}
void smooth_operator(Stream);
} // namespace ops
void ops_test() {
ops::stream << 5;
// no warning
operator<<(ops::stream, 5);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl]
ops::stream << aspace::A();
// no warning
operator<<(ops::stream, aspace::A());
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl]
ops::stream >> aspace::A();
// no warning
operator>>(ops::stream, aspace::A());
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator>>' through ADL [bugprone-unintended-adl]
smooth_operator(ops::stream);
QuuxplusoneUnsubmitted
Not Done
ReplyInline Actions

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.

Quuxplusone: This is awesome. :) Please also consider ADL where there's more than one associated namespace…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

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.

logan-5: It's interesting, that code only triggers the check (i.e. my AST matchers only think it's doing…
JonasTothUnsubmitted
Not Done
ReplyInline Actions

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.

JonasToth: The matcher itself only ask the `CallExpr->usesADL()` (not sure about the method name right now…
EricWFUnsubmitted
Not Done
ReplyInline Actions

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.

EricWF: A call expression only "usesADL" if the callee was found only through ADL lookup. If it's found…
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl]
}
namespace std {
// return types don't matter, returning 'void' everywhere for simplicity
JonasTothUnsubmitted
Done
ReplyInline Actions

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.

JonasToth: This function is not instantiated right now, is it? Can you please write a function that would…
template <typename T>
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

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?

Quuxplusone: This is not the idiomatic way of calling `swap`: there is no ADL swap for `int`, for example…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

I believe this addressed by my juggling the tests around a bit.

logan-5: I believe this addressed by my juggling the tests around a bit.
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

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

Quuxplusone: Juggling the tests around doesn't address the fact that any code that does `swap(a,b)` without…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

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: Thanks for clarifying. I do think that is out of scope for my goals for this patch, but I think…
void swap(T &a, T &b);
template <typename T>
void make_error_code(T);
template <typename T>
void make_error_condition(T);
template <typename T>
void data(T);
template <typename T>
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

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/?

Quuxplusone: Ah, this is subtle. Lookup on `ref` finds the global variable `ref` and therefore doesn't do…
void begin(T);
template <typename T>
void end(T);
template <typename T>
void rbegin(T);
template <typename T>
void rend(T);
template <typename T>
void crbegin(T);
template <typename T>
void crend(T);
template <typename T>
void size(T);
template <typename T>
void ssize(T);
template <typename T>
void empty(T);
template <typename T>
void move(T &&);
template <typename T>
void forward(T &&);
struct byte {};
} // namespace std
namespace ns {
struct Swappable {};
// whitelisted
void swap(Swappable &a, Swappable &b);
void make_error_code(Swappable);
void make_error_condition(Swappable);
void data(Swappable);
void begin(Swappable);
void end(Swappable);
void rbegin(Swappable);
void rend(Swappable);
void crbegin(Swappable);
void crend(Swappable);
void size(Swappable);
void ssize(Swappable);
void empty(Swappable);
// non-whitelisted
void move(Swappable);
void ref(Swappable);
struct Swappable2 {};
} // namespace ns
struct {
template <typename T>
void operator()(T &&);
} ref;
void test2() {
// TODO add granularity for detecting functions that may always be called unqualified,
// versus those that can only be called through the 'using' 'two-step'
using namespace std;
ns::Swappable a, b;
swap(a, b);
make_error_code(a);
make_error_condition(a);
data(a);
begin(a);
end(a);
rbegin(a);
rend(a);
crbegin(a);
crend(a);
size(a);
ssize(a);
empty(a);
move(a);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl]
}
template <typename T>
void foo(T t) {
using namespace std;
swap(t, t);
make_error_code(t);
make_error_condition(t);
data(t);
begin(t);
end(t);
rbegin(t);
rend(t);
crbegin(t);
crend(t);
size(t);
ssize(t);
empty(t);
move(t);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl]
// CHECK-MESSAGES: [[@LINE-2]]:3: note: with argument type 'struct ns::Swappable'
// CHECK-MESSAGES: [[@LINE-3]]:3: warning: unqualified call to 'move' may be resolved through ADL [bugprone-unintended-adl]
std::swap(t, t);
std::move(t);
ref(t); // function objects bypass ADL, this always calls ::ref
::ref(t);
}
template <typename T, typename U>
void operator<<(T &&, U &&);
template <typename T>
void bar(T t) {
t << 5;
operator<<(t, 5);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl]
// CHECK-MESSAGES: [[@LINE-2]]:3: note: with argument types 'struct ops::Stream', 'int'
// CHECK-MESSAGES: [[@LINE-3]]:3: warning: unqualified call to 'operator<<' may be resolved through ADL [bugprone-unintended-adl]
}
void instantiator() {
foo(ns::Swappable()); // instantiation will use ADL
foo(5); // instantiation will not use ADL
bar(ops::Stream()); // instantiation will use ADL
bar(aspace::A()); // instantiation will not use ADL
}
template <typename T>
void macro_test(T t) {
#define MACRO(x) find_if(x, x, x)
EricWFUnsubmitted
Done
ReplyInline Actions

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?

EricWF: Arguably this is *exactly* the kind of code we want to diagnose. The call in the macro either…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

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?

logan-5: Fair enough. Disabling the check for macros does seem short sighted on closer thought. When I…
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

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?

Quuxplusone: Ouch, is that because `__func__` is an array of char with dependent length? ``` template<class…
logan-5AuthorUnsubmitted
Done
ReplyInline Actions

Yep. So, I suppose the check should not trigger for expressions of DependentSizedArrayType of char? Or of any built-in type, really?

logan-5: Yep. So, I suppose the check should not trigger for expressions of `DependentSizedArrayType` of…
MACRO(t);
#undef MACRO
#define MACRO(x) func(x)
MACRO(aspace::A());
#undef MACRO
}