[clang] Add test for CWG405

[Endill]

Summary

P1787: CWG405 is resolved by stating that argument-dependent lookup (sometimes) occurs after an ordinary unqualified lookup (making statements like “finding a variable prevents argument-dependent lookup” formally correct).
Wording: see changes to [basic.lookup.argdep] p1 and p3

This issue seems a duplicate of CWG218, even though it is not officially recognized. A part of a test for CWG218 is reused here, adding cross-references.

[Endill]

Is it fine that we're marking CWG405 as a duplicate even though it's not mentioned as such in official publication?

[aaron.ballman]

In D139095#3963334, @Endill wrote:

Is it fine that we're marking CWG405 as a duplicate even though it's not mentioned as such in official publication?

I don't think we should mark it as a dup -- we want the status in our tests to match the status on the official document, otherwise things get confusing. The two issues are very closely related, but they change different words in the standard and should be tested independently as best we can. However, comments in the test for each DR pointing to the other related DR would be a good idea.

[Endill]

I don't think we should mark it as a dup -- we want the status in our tests to match the status on the official document, otherwise things get confusing.

We can do it the following way then: // dr405: yes \n // NB: also dup 218.
Do I understand correctly that superseded status should be used if and only if it's used in official document as well?

The two issues are very closely related, but they change different words in the standard and should be tested independently as best we can

Make no mistake here: proposed wording for 405 has never made it into the standard. I double-checked this with revision 100 of CWG issues, when this issue still had open status.
P1787 states that CWG405 is resolved by stating that argument-dependent lookup (sometimes) occurs after an ordinary unqualified lookup (making statements like “finding a variable prevents argument-dependent lookup” formally correct).
The only relevant wording in P1787 I can find is for [basic.lookup.argdep] p1 and p3 (too long; not citing them here). Which, curiously enough, clearly originates from resolution of 218, which is already tested. It also has the same intent as proposed resolution for 405.

So I'd like to raise a couple of questions:

1. What test for 405 is going to be if not a copy-and-paste of a part of 218 test?
2. Is it possible to change status of 405 in the official document? Or get a technical rationale for it not being a duplicate of 218.

As a side note, I don't feel too comfortable testing name lookup via side effects like diagnostics. #pragma clang __debug dump is good, but not powerful enough to test ADL. Are those the only options we currently have?

[cor3ntin]

In D139095#3964181, @Endill wrote:

So I'd like to raise a couple of questions:

1. What test for 405 is going to be if not a copy-and-paste of a part of 218 test?

I think this is perfectly fine to have a duplicated test case, I agree with Aaron, we should not invent duplicated status ourselves.
Adding a comment in the test like "Note: this test is identical to the one for CWG405" would be a good idea

2. Is it possible to change status of 405 in the official document? Or get a technical rationale for it not being a duplicate of 218.

Nah, that wouldn't be worth the hassle, even if you got people to agree on the duplicated nature

As a side note, I don't feel too comfortable testing name lookup via side effects like diagnostics. #pragma clang __debug dump is good, but not powerful enough to test ADL. Are those the only options we currently have?

You could do a codegen tests and check that the correct function gets called using its mangled name. There are examples in the drs tests already, grep for "// CHECK: call"

[Endill]

I think this is perfectly fine to have a duplicated test case, I agree with Aaron, we should not invent duplicated status ourselves.
Adding a comment in the test like "Note: this test is identical to the one for CWG405" would be a good idea

Does it mean that duplication with cross-references is the best way to handle this even hypothetically? As opposed to, say, new notation for make_cxx_dr_status like // dr405: dup 405 unofficial.

Nah, that wouldn't be worth the hassle, even if you got people to agree on the duplicated nature

Sad but definitely not unexpected.

You could do a codegen tests and check that the correct function gets called using its mangled name. There are examples in the drs tests already, grep for "// CHECK: call"

Thanks for mentioning this! Could definitely be used as a last resort. I'll try it for some of the subsequent CWG test.
Observing front-end behavior via back-end still doesn't feel good, though. I believe debug facilities should be improved to contain as much DR checks as possible at source and AST level.

[aaron.ballman]

In D139095#3964181, @Endill wrote:

I don't think we should mark it as a dup -- we want the status in our tests to match the status on the official document, otherwise things get confusing.

We can do it the following way then: // dr405: yes \n // NB: also dup 218.

That would be fine by me!

Do I understand correctly that superseded status should be used if and only if it's used in official document as well?

That one is somewhat harder in that it's possible for future changes to the standard to supersede previously resolved issues without the committee going back to mark those old DRs as superseded. However, I think in general, we should stick with the status in the document, and we can use extra comments to add our own information on top of it.

The two issues are very closely related, but they change different words in the standard and should be tested independently as best we can

Make no mistake here: proposed wording for 405 has never made it into the standard. I double-checked this with revision 100 of CWG issues, when this issue still had open status.
P1787 states that CWG405 is resolved by stating that argument-dependent lookup (sometimes) occurs after an ordinary unqualified lookup (making statements like “finding a variable prevents argument-dependent lookup” formally correct).
The only relevant wording in P1787 I can find is for [basic.lookup.argdep] p1 and p3 (too long; not citing them here). Which, curiously enough, clearly originates from resolution of 218, which is already tested. It also has the same intent as proposed resolution for 405.

So I'd like to raise a couple of questions:

1. What test for 405 is going to be if not a copy-and-paste of a part of 218 test?

In terms of test *coverage*, no benefit. In terms of *implementation status*, it makes it clear we considered the DR explicitly instead of leaving future folks to wonder.

In D139095#3966438, @Endill wrote:

I think this is perfectly fine to have a duplicated test case, I agree with Aaron, we should not invent duplicated status ourselves.
Adding a comment in the test like "Note: this test is identical to the one for CWG405" would be a good idea

Does it mean that duplication with cross-references is the best way to handle this even hypothetically? As opposed to, say, new notation for make_cxx_dr_status like // dr405: dup 405 unofficial.

I don't see a need for a new notation yet. These notations are specific to us generating the dr status page with the correct information, and users don't usually care about whether two DRs are dupes of one another so much as "I think this DR applies to my code, does Clang implement it?" kind of questions.

Nah, that wouldn't be worth the hassle, even if you got people to agree on the duplicated nature

Sad but definitely not unexpected.

You could do a codegen tests and check that the correct function gets called using its mangled name. There are examples in the drs tests already, grep for "// CHECK: call"

Thanks for mentioning this! Could definitely be used as a last resort. I'll try it for some of the subsequent CWG test.
Observing front-end behavior via back-end still doesn't feel good, though. I believe debug facilities should be improved to contain as much DR checks as possible at source and AST level.

CodeGen tests would be the approach I'd take; that's not actually testing the backend behavior, that's still testing the frontend IR generation (which is before the backend gets to start mutating/optimizing it).

[Endill]

We can do it the following way then: dr405: yes \n NB: also dup 218.

That would be fine by me!

Which way should we handle this? I'd prefer to do it without test duplication, but making it clear for readers is a serious concern indeed. (Read on, I elaborate on this below.)

What test for 405 is going to be if not a copy-and-paste of a part of 218 test?

In terms of test *coverage*, no benefit. In terms of *implementation status*, it makes it clear we considered the DR explicitly instead of leaving future folks to wonder.

Should we really try to meet an expectation that Clang developers haven't considered something as obvious as explicitly handling a DR not marked as duplicated or superseded officially? It doesn't seem a reasonable expectation to me, and at least my personal attitude have always been the opposite.
To be fully honest here, I don't even remember myself how I came across CWG218, because I did this test back in spring. So marking it as a duplicate (one way or another) on the contrary seems very considerate handling.

CodeGen tests would be the approach I'd take; that's not actually testing the backend behavior, that's still testing the frontend IR generation (which is before the backend gets to start mutating/optimizing it).

You're very much right. Thank you for reminding me of that!

[aaron.ballman]

In D139095#3966626, @Endill wrote:

We can do it the following way then: dr405: yes \n NB: also dup 218.

That would be fine by me!

Which way should we handle this? I'd prefer to do it without test duplication, but making it clear for readers is a serious concern indeed. (Read on, I elaborate on this below.)

Sorry, I was unclear! I meant that I would be fine with:

... test code ... // dr405: yes
                  // NB: also dup 218
...
...
...

... test code ... // dr218: yes
                  // NB: also dup 405

So the test code is duplicated AND we've left a comment linking the two tests together.

What test for 405 is going to be if not a copy-and-paste of a part of 218 test?

In terms of test *coverage*, no benefit. In terms of *implementation status*, it makes it clear we considered the DR explicitly instead of leaving future folks to wonder.

Should we really try to meet an expectation that Clang developers haven't considered something as obvious as explicitly handling a DR not marked as duplicated or superseded officially? It doesn't seem a reasonable expectation to me, and at least my personal attitude have always been the opposite.

To be fully honest here, I don't even remember myself how I came across CWG218, because I did this test back in spring. So marking it as a duplicate (one way or another) on the contrary seems very considerate handling.

There's two approaches: assume it's obvious that since it's a duplicate we thought about it, or put in test coverage + comments to prove we thought about it. I'd prefer putting in the test coverage in this case -- I've been bitten too many times by "was <this> considered?" when doing code archeology in the project (granted, most of the severe cases come from when we were doing far less patch review before committing things).

[Endill]

Reuse a part of CWG218 test, adding cross-references.

[Endill]

clang/test/CXX/drs/dr4xx.cpp

99

I'm surprised that local function declaration prevents ADL, but using-declaration doesn't. It has been working this way all along, so I guess I better take a note of this.

[aaron.ballman]

LGTM

clang/test/CXX/drs/dr4xx.cpp

99

A using-declaration is the idiomatic way to use ADL: https://en.cppreference.com/w/cpp/language/adl (see just after the Notes heading), but pretty much everything about ADL is a surprise to most folks. :-)

clang/test/CXX/drs/dr2xx.cpp

 namespace dr217 { // dr217: yes
   template<typename T> struct S {
     void f(int);
   };
   template<typename T> void S<T>::f(int = 0) {} // expected-error {{default arguments cannot be added}}
 }
 
 namespace dr218 { // dr218: yes
+                  // NB: also dup 405
   namespace A {
     struct S {};
     void f(S);
   }
   namespace B {
     struct S {};
     void f(S);
   }

clang/test/CXX/drs/dr4xx.cpp

 // RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
 // RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
 // RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
 // RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
 // RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: env ASAN_OPTIONS=detect_stack_use_after_return=0 %clang_cc1 -std=c++2b %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
 
 // FIXME: __SIZE_TYPE__ expands to 'long long' on some targets.
 __extension__ typedef __SIZE_TYPE__ size_t;
 
 namespace std { struct type_info; }
 
 namespace dr400 { // dr400: yes
   struct A { int a; struct a {}; }; // expected-note 2{{conflicting}} expected-note {{ambiguous}}
int k = f(p); // ok, finds A::f, even though type XS is a typedef-name
                 // referring to an elaborated-type-specifier naming a
                 // injected-class-name, which is about as far from a
                 // template-id as we can make it.
 }
 
 // dr404: na
 // (NB: also sup 594)
 
+namespace dr405 { // dr405: yes
+                  // NB: also dup 218
+  namespace A {
+    struct S {};
+    void f(S);
+  }
+  namespace B {
+    struct S {};
+    void f(S);
+  }
+
+  struct C {
+    int f;
+    void test1(A::S as) { f(as); } // expected-error {{called object type 'int'}}
+    void test2(A::S as) { void f(); f(as); } // expected-error {{too many arguments}} expected-note {{}}
+    void test3(A::S as) { using A::f; f(as); } // ok
+    void test4(A::S as) { using B::f; f(as); } // ok
+    void test5(A::S as) { int f; f(as); } // expected-error {{called object type 'int'}}
+    void test6(A::S as) { struct f {}; (void) f(as); } // expected-error {{no matching conversion}} expected-note +{{}}
+  };
+
+  namespace D {
+    struct S {};
+    struct X { void operator()(S); } f;
+  }
+  void testD(D::S ds) { f(ds); } // expected-error {{undeclared identifier}}
+
+  namespace E {
+    struct S {};
+    struct f { f(S); };
+  }
+  void testE(E::S es) { f(es); } // expected-error {{undeclared identifier}}
+}
+
 namespace dr406 { // dr406: yes
   typedef struct {
     static int n; // expected-error {{static data member 'n' not allowed in anonymous struct}}
   } A;
   typedef union {
     static int n; // expected-error {{static data member 'n' not allowed in anonymous union}}
   } B;
 }

clang/www/cxx_dr_status.html

<tr id="404">
     <td>CD1</td>
     <td>Unclear reference to construction with non-trivial constructor</td>
     <td class="na" align="center">N/A</td>
   </tr>
   <tr id="405">
     <td><a href="https://wg21.link/cwg405">405</a></td>
     <td>CD6</td>
     <td>Unqualified function name lookup</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">Yes</td>
   </tr>
   <tr id="406">
     <td><a href="https://wg21.link/cwg406">406</a></td>
     <td>CD1</td>
     <td>Static data member in class with name for linkage purposes</td>
     <td class="full" align="center">Yes</td>
   </tr>
   <tr id="407">