[clang] Add test for CWG399

[Endill]

Summary

P1787: CWG399 is resolved by explicitly appealing to the lookup for the last component of any suitable nested-name-specifier.
Wording: Otherwise, its nested-name-specifier N shall nominate a type. If N has another nested-name-specifier S, Q is looked up as if its lookup context were that nominated by S. ([basic.lookup.qual]/6.2)

CWG399 revisits a resolution to older CWG244. Our test for CWG244 covers many examples from CWG399, and it was updated in 2020 presumably aware of P1787, so I reused CWG244 test. This approach to reusing was discussed in a CWG405 patch review.

[Endill]

clang/test/CXX/drs/dr3xx.cpp

1439

Despite a couple of FIXME in CWG244 test (out of dozens of examples), it claims full availability since Clang 11. I'd take a more conservative approach, declaring partial support, but I think that declaring different availability for the same test would bring unnecessary confusion. So I followed CWG244 availability.

Alternative is to demote CWG244 to partial, but I'm not sure we should go back on our claims for CWG support that has been out for so long.

[shafik]

clang/test/CXX/drs/dr3xx.cpp

1492

You say we accept the next line but it has an expected-error on it?

[Endill]

I think I haven't stressed it enough, but this whole test is copied from dr244, which is written by Richard.

clang/test/CXX/drs/dr3xx.cpp

1492

It's an error because of -pedantic-errors. It's a warning by default.

[shafik]

clang/test/CXX/drs/dr3xx.cpp

1439

I think the bugs are not awful, we should file bug reports if we don't already have them. Some of them seem like they should be not too bad to fix.

CC @aaron.ballman to get a second opinion

1492

That makes a lot more sense, I was wondering what was I missing.

Can we note that in the comment b/c it is pretty confusing otherwise.

I wonder if there is a good reason to not make this ill-formed by default? Worth a bug report.

[shafik]

In D147920#4257369, @Endill wrote:

I think I haven't stressed it enough, but this whole test is copied from dr244, which is written by Richard.

Understood, I appreciate the patience in explaining what I am missing. Sometimes that means things could be explained better.

[Endill]

In D147920#4258680, @shafik wrote:

In D147920#4257369, @Endill wrote:

I think I haven't stressed it enough, but this whole test is copied from dr244, which is written by Richard.

Understood, I appreciate the patience in explaining what I am missing. Sometimes that means things could be explained better.

No worries. Those DRs are involved enough that it could be challenging to understand the context quickly.

clang/test/CXX/drs/dr3xx.cpp

1492

Can we note that in the comment b/c it is pretty confusing otherwise.

I get where you come from, but all DR testing is done under -pedantic-errors. Do you have ideas for a more systematic approach? One of the options is to use -Wpedantic instead, but I expect that to require a decent amount of mechanical replacements for existing tests, which LLVM community doesn't appreciate, as far as I know. I'll edit the comment if we won't come up with something better.

I wonder if there is a good reason to not make this ill-formed by default? Worth a bug report.

Does the fact that we accepted such code since (at least) 3.5 through 10 make for a good reason? https://godbolt.org/z/16GYWh3Po

[Endill]

clang/test/CXX/drs/dr3xx.cpp

1439

If we are to file bug reports, I'm not sure what wording makes those examples ill-formed. Is it qual.general-4.6: The type-name that is or contains Q shall refer to its (original) lookup context (ignoring cv-qualification) under the interpretation established by at least one (successful) lookup performed.? I interpret it as requiring names to the left and to the right of ~ to be found in the same scope (lookup context; namespace dr244 in our case). Could it actually mean that they have to refer to the same type?

[shafik]

clang/test/CXX/drs/dr3xx.cpp

1439

I am not sure maybe @rsmith might be able to help us here.

[Endill]

@aaron.ballman ping

[aaron.ballman]

clang/test/CXX/drs/dr3xx.cpp

1439

I think we want to start being more conservative with claiming support for features and DRs, and that means being more honest with "partial" markings (with comments as to WHY the support is only partial, what's still left to be done, etc). I don't think it's a problem to say "we've discovered enough issues with this that we no longer claim to support it" when that's accurate.

I don't think we have a hard and fast rule for when a bug is sufficiently worrying to merit partial vs full support; it's going to depend on the situation, I think. Failing to diagnose incorrect code is a different kind of problem from diagnosing correct code from crashing bugs from etc. and it's going to be up to the patch author and reviewers to make a value judgement. But that's why I think it's fine for us to update the status when we learn more information, too.

That said, when we do have partial support, we definitely need to file issues to address the remaining bits at some point.

I interpret it as requiring names to the left and to the right of ~ to be found in the same scope (lookup context; namespace dr244 in our case). Could it actually mean that they have to refer to the same type?

I've read that wording a few times now and can't make heads or tails of what it's trying to say. Perhaps @rsmith or @hubert.reinterpretcast can help illuminate us?

[aaron.ballman]

clang/test/CXX/drs/dr3xx.cpp

1439

@Endill reminded me off-list that the FIXME comments here are existing comments; some of these test cases are lifted from the dr244 test cases. Given that and it's been a few weeks and we've not determine what issues to file, I think we should unblock this review as it makes forward progress on our test coverage for dr399. Filing issues would be good, but not a prerequisite for landing this. WDYT @shafik?

[Endill]

@shafik ping

clang/test/CXX/drs/dr3xx.cpp

1439

@Endill reminded me off-list that the FIXME comments here are existing comments; some of these test cases are lifted from the dr244 test cases. Given that and it's been a few weeks and we've not determine what issues to file, I think we should unblock this review as it makes forward progress on our test coverage for dr399. Filing issues would be good, but not a prerequisite for landing this. WDYT @shafik?

[shafik]

LGTM, apologies, it fell off my radar

[cor3ntin]

I'm seeing test failures on main

error: 'error' diagnostics expected but not seen: 
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1456: refers to a member in namespace
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1457: refers to a member in namespace
error: 'error' diagnostics seen but not expected: 
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1456: use of undeclared identifier 'dr244'
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1457: use of undeclared identifier 'dr244'
error: 'note' diagnostics expected but not seen: 
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1441: type 'dr244::B' found by destructor name lookup
error: 'note' diagnostics seen but not expected: 
  File /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/test/CXX/drs/dr3xx.cpp Line 1441: type 'dr399::B' found by destructor name lookup
6 errors generated.

[Endill]

Thank you!
I'm going to fix this now.

[Endill]

Should be fixed by https://github.com/llvm/llvm-project/commit/abbb22cc0c9c33dedb8d53c2bd3e703f92baace7.

clang/test/CXX/drs/dr2xx.cpp

namespace dr243 { // dr243: yes
   struct B {
     operator A() = delete; // expected-error 0-1{{extension}} expected-note {{candidate}}
   } b;
   A a1(b);
   A a2 = b; // expected-error {{ambiguous}}
 }
 
 namespace dr244 { // dr244: 11
+                  // NB: this test is reused by dr399
   struct B {}; // expected-note {{type 'dr244::B' found by destructor name lookup}}
   struct D : B {};
 
   D D_object;
   typedef B B_alias;
   B* B_ptr = &D_object;
 
   void f() {

clang/test/CXX/drs/dr3xx.cpp

namespace example2 {
     void test() {
       f<A>(0); // expected-error {{no matching function}}
       f<B>(0); // expected-error {{no matching function}}
       g<C>(0); // expected-error {{no matching function}}
       h<D>(0); // expected-error {{no matching function}}
     }
   }
 }
+
+namespace dr399 { // dr399: 11
+                  // NB: reuse dr244 test 
+  struct B {}; // expected-note {{type 'dr244::B' found by destructor name lookup}}
+  struct D : B {};
+
+  D D_object;
+  typedef B B_alias;
+  B* B_ptr = &D_object;
+
+  void f() {
+    D_object.~B(); // expected-error {{does not match the type 'D' of the object being destroyed}}
+    D_object.B::~B();
+    D_object.D::~B(); // FIXME: Missing diagnostic for this.
+    B_ptr->~B();
+    B_ptr->~B_alias();
+    B_ptr->B_alias::~B();
+    B_ptr->B_alias::~B_alias();
+    B_ptr->dr244::~B(); // expected-error {{refers to a member in namespace}}
+    B_ptr->dr244::~B_alias(); // expected-error {{refers to a member in namespace}}
+  }
+
+  template<typename T, typename U>
+  void f(T *B_ptr, U D_object) {
+    D_object.~B(); // FIXME: Missing diagnostic for this.
+    D_object.B::~B();
+    D_object.D::~B(); // FIXME: Missing diagnostic for this.
+    B_ptr->~B();
+    B_ptr->~B_alias();
+    B_ptr->B_alias::~B();
+    B_ptr->B_alias::~B_alias();
+    B_ptr->dr399::~B(); // expected-error {{does not refer to a type name}}
+    B_ptr->dr399::~B_alias(); // expected-error {{does not refer to a type name}}
+  }
+  template void f<B, D>(B*, D);
+
+  namespace N {
+    template<typename T> struct E {};
+    typedef E<int> F;
+  }
+  void g(N::F f) {
+    typedef N::F G; // expected-note {{found by destructor name lookup}}
+    f.~G();
+    f.G::~E(); // expected-error {{ISO C++ requires the name after '::~' to be found in the same scope as the name before '::~'}}
+    f.G::~F(); // expected-error {{undeclared identifier 'F' in destructor name}}
+    f.G::~G();
+    // This is technically ill-formed; E is looked up in 'N::' and names the
+    // class template, not the injected-class-name of the class. But that's
+    // probably a bug in the standard.
+    f.N::F::~E(); // expected-error {{ISO C++ requires the name after '::~' to be found in the same scope as the name before '::~'}}
+    // This is valid; we look up the second F in the same scope in which we
+    // found the first one, that is, 'N::'.
+    f.N::F::~F();
+    // This is technically ill-formed; G is looked up in 'N::' and is not found.
+    // Rejecting this seems correct, but most compilers accept, so we do also.
+    f.N::F::~G(); // expected-error {{qualified destructor name only found in lexical scope; omit the qualifier to find this type name by unqualified lookup}}
+  }
+
+  // Bizarrely, compilers perform lookup in the scope for qualified destructor
+  // names, if the nested-name-specifier is non-dependent. Ensure we diagnose
+  // this.
+  namespace QualifiedLookupInScope {
+    namespace N {
+      template <typename> struct S { struct Inner {}; };
+    }
+    template <typename U> void f(typename N::S<U>::Inner *p) {
+      typedef typename N::S<U>::Inner T;
+      p->::dr399::QualifiedLookupInScope::N::S<U>::Inner::~T(); // expected-error {{no type named 'T' in}}
+    }
+    template void f<int>(N::S<int>::Inner *); // expected-note {{instantiation of}}
+
+    template <typename U> void g(U *p) {
+      typedef U T;
+      p->T::~T();
+      p->U::~T();
+      p->::dr399::QualifiedLookupInScope::N::S<int>::Inner::~T(); // expected-error {{'T' does not refer to a type name}}
+    }
+    template void g(N::S<int>::Inner *);
+  }
+}

clang/www/cxx_dr_status.html

<tr id="398">
     <td>CD1</td>
     <td>Ambiguous wording on naming a type in deduction</td>
     <td class="full" align="center">Yes</td>
   </tr>
   <tr id="399">
     <td><a href="https://cplusplus.github.io/CWG/issues/399.html">399</a></td>
     <td>CD6</td>
     <td>Destructor lookup redux</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">Clang 11</td>
   </tr>
   <tr id="400">
     <td><a href="https://cplusplus.github.io/CWG/issues/400.html">400</a></td>
     <td>CD1</td>
     <td>Using-declarations and the "struct hack"</td>
     <td class="full" align="center">Yes</td>
   </tr>
   <tr id="401">