[Clang] Fix PR28101

[rZhBoYao]

Summary

Fixes PR28101
(https://llvm.org/PR28101)
by setting identifier for invalid member variables with template parameters,
so that the invalid declarators would not crash clang.

[erichkeane]

Can you add some tests please? Additionally, we don't typically use LLVM_UNLIKELY like you did there.

[aaron.ballman]

FWIW, this appears to cause quite a few failures in precommit CI that should be addressed as well: https://buildkite.com/llvm-project/premerge-checks/builds/68803#45a9b858-fa66-4a4b-8c4a-20c2cef820e5

Failed Tests (11):
  Clang :: CXX/basic/basic.lookup/basic.lookup.qual/class.qual/p2.cpp
  Clang :: CXX/drs/dr3xx.cpp
  Clang :: CXX/special/class.ctor/p1.cpp
  Clang :: CXX/temp/temp.deduct.guide/p1.cpp
  Clang :: CodeGenObjCXX/block-in-template-inst.mm
  Clang :: Parser/explicit-bool.cpp
  Clang :: SemaCXX/constant-expression-cxx11.cpp
  Clang :: SemaCXX/constructor.cpp
  Clang :: SemaCXX/conversion-function.cpp
  Clang :: SemaCXX/cxx2a-compat.cpp
  Clang :: SemaCXX/destructor.cpp

[rZhBoYao]

I added a test. There were 17 module and PCH related failed tests on my local machine; however, when I ran the test on then ToT the same 17 tests failed, but on GitHub it does show 2 successful checks. So maybe it's something else.

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

6485–6488 ↗

(On Diff #392774)

This fix looks incredibly specific to the bug report, how does it handle other situations, like:

template <typename T, template <typename> typename U>
class PR28101 {
public:
  PR28101(void *) {}
  T(PR28101<T, U<T>>) {} // Do we err here?
};

template <typename T>
struct S {};

PR28101<int, S> foo() { return PR28101<int, S>(nullptr); }

clang/test/CXX/temp/temp.decls/temp.mem/p3.cpp

12–13 ↗

(On Diff #392774)

It seems incorrect that we're issuing the same diagnostic twice. Is this because of template instantiation, or some other reason?

16 ↗

(On Diff #392774)

The test location is a bit novel -- [temp.mem]p3 is "A member function template shall not be declared virtual." and I don't see anything related to p3 in this test. I think the test case is reasonable enough, but it should probably live in SemaCXX instead of here.

[rZhBoYao]

The previous diff was indeed very specific and doesn't handle

template <typename T>
struct A {
  A(void*) {}
  T A<T>{}; // expected-error{{member 'A' cannot have template arguments}}
};

A<int> instantiate() { return {nullptr}; }

I have since realized the invalid decorator is the problem and the parentheses are insignificant. It's like:

template <typename T> struct S {};
int S<int>{};

which gcc diagnoses to

error: invalid declarator before '{' token

Then I found there was a new diagnostic added by D120881. So the fix is pretty straightforward at this point. I simply stand on the shoulder of the giant and set the invalid flag of the declarator.

[rZhBoYao]

Don't break template declarations.
re-clang-format

[rZhBoYao]

Diagnose "same name as its class" before setting the declarator invalid as otherwise it would not be diagnosed. This also aligns with gcc's behavior.

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

3430

I see we are diagnosing this ONLY inside of the case where it is a template. Where is this caught when we are in the non-template case, why doesn't THAT catch these cases, and what would it take to do so?

clang/test/SemaCXX/PR28101.cpp

9

This isn't necessary, the compiler in verify mode should see all errors. I see above at most 2 invocations of the compiler as necessary.

14

This second error here is strange, I don't see it as particularly useful here.

I see gcc at least gives the full name (int A<int>::A), perhaps that makes this more useful?

17

How come there are no notes in this test that say 'in instantiation of...'? I would expect those in at least some of these cases, right?

[rZhBoYao]

clang/lib/Sema/SemaDeclCXX.cpp

3430

CheckCompletedCXXClass should catch diag::err_member_name_of_class. However, these FieldDecl have no name hence uncaught. Maybe we should set the identifier for this declarator here.

clang/test/SemaCXX/PR28101.cpp

9

Instantiation of each of the 4 classes could crash on its own, so I thought I'd separate them into 4 invocations.

17

Because it was caught before instantiation and when instantiating the FieldDecl is gone after calling D.setInvalidType();. If I set the identifier for the FieldDecl and let CheckCompletedCXXClass handle the "has the same name as its class" error, there will be the "in instantiation of..." note.

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

3430

I'm not sure what the side effect of that is... Can you give it a shot?

clang/test/SemaCXX/PR28101.cpp

9

They shouldn't crash afterwards, right? So this shouldn't be a problem.

17

I think we definitely need to do so there, the instantiation trace not being here makes this a much less useful diagnostic.

[rZhBoYao]

This passes check-clang-semacxx on my machine.

[erichkeane]

Please add a 'release notes'. Otherwise, LGTM as long as the bots are ok with it!

[rZhBoYao]

By 'release notes' do you mean a more detailed commit message?

[erichkeane]

Nope, see release notes here: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst

[rZhBoYao]

Added an entry in release notes. Waiting for CI...

clang/docs/ReleaseNotes.rst

 - We now ignore full expressions when traversing cast subexpressions. This
   fixes `Issue 53044 <https://github.com/llvm/llvm-project/issues/53044>`_.
 - Allow `-Wno-gnu` to silence GNU extension diagnostics for pointer arithmetic
   diagnostics. Fixes `Issue 54444 <https://github.com/llvm/llvm-project/issues/54444>`_.
 - Placeholder constraints, as in `Concept auto x = f();`, were not checked when modifiers
   like ``auto&`` or ``auto**`` were added. These constraints are now checked.
   This fixes  `Issue 53911 <https://github.com/llvm/llvm-project/issues/53911>`_
   and  `Issue 54443 <https://github.com/llvm/llvm-project/issues/54443>`_.
+- Previously invalid member variables with template parameters would crash clang.
+  Now fixed by setting identifiers for them.
+  This fixes `Issue 28475 (PR28101) <https://github.com/llvm/llvm-project/issues/28475>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 
 Non-comprehensive list of changes in this release

clang/lib/Sema/SemaDeclCXX.cpp

if (isInstField) {
     }
 
     if (D.getName().getKind() == UnqualifiedIdKind::IK_TemplateId) {
       Diag(D.getIdentifierLoc(), diag::err_member_with_template_arguments)
           << II
           << SourceRange(D.getName().TemplateId->LAngleLoc,
                          D.getName().TemplateId->RAngleLoc)
           << D.getName().TemplateId->LAngleLoc;
+      D.SetIdentifier(Name.getAsIdentifierInfo(), Loc);
     }
 
     if (SS.isSet() && !SS.isInvalid()) {
       // The user provided a superfluous scope specifier inside a class
       // definition:
       //
       // class X {
       //   int X::member;

clang/test/SemaCXX/PR28101.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+template <typename T> struct A {
+  A(void *) {}
+  T(A<T>){}; // expected-error{{member 'A' cannot have template arguments}}\
+  // expected-error2{{member 'A' has the same name as its class}}
+};
+// Don't crash.
+A<int> instantiate1() { return {nullptr}; } // expected-note{{in instantiation of template class 'A<int>' requested here}}
+
+template <typename T> struct B {
+  B(void *) {}
+  T B<T>{}; // expected-error{{member 'B' cannot have template arguments}}\
+  // expected-error2{{member 'B' has the same name as its class}}
+};
+// Don't crash.
+B<int> instantiate2() { return {nullptr}; } // expected-note{{in instantiation of template class 'B<int>' requested here}}
+
+template <typename T> struct S {};
+
+template <typename T> struct C {
+  C(void *) {}
+  T S<T>{}; // expected-error{{member 'S' cannot have template arguments}}
+};
+// Don't crash.
+C<int> instantiate3() { return {nullptr}; }
+
+template <typename T, template <typename> typename U> class D {
+public:
+  D(void *) {}
+  T(D<T, U<T>>) {} // expected-error{{member 'D' cannot have template arguments}}\
+  // expected-error{{expected ';' at end of declaration list}}\
+  // expected-error2{{member 'D' has the same name as its class}}
+};
+// Don't crash.
+D<int, S> instantiate4() { return D<int, S>(nullptr); } // expected-note{{in instantiation of template class 'D<int, S>' requested here}}