[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

8

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.

13

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?

16

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

8

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

16

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

8

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

16

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/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;
+      if (cast<CXXRecordDecl>(CurContext)->getDeclName() == Name)
+        Diag(Loc, diag::err_member_name_of_class) << Name;
+      D.setInvalidType();
     }
 
     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 -DCASE_1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -DCASE_2 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -DCASE_3 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -DCASE_4 -std=c++17 %s
+
+// Don't crash.
+
+#ifdef CASE_1
+
+template <typename T> struct A {
+  A(void *) {}
+  T(A<T>){}; // expected-error{{member 'A' cannot have template arguments}}\
+  // expected-error{{member 'A' has the same name as its class}}
+};
+
+A<int> instantiate() { return {nullptr}; }
+
+#elifdef CASE_2
+
+template <typename T> struct A {
+  A(void *) {}
+  T A<T>{}; // expected-error{{member 'A' cannot have template arguments}}\
+  // expected-error{{member 'A' has the same name as its class}}
+};
+
+A<int> instantiate() { return {nullptr}; }
+
+#elifdef CASE_3
+
+template <typename T> struct S {};
+
+template <typename T> struct A {
+  A(void *) {}
+  T S<T>{}; // expected-error{{member 'S' cannot have template arguments}}
+};
+
+A<int> instantiate() { return {nullptr}; }
+
+#elifdef CASE_4
+
+template <typename T, template <typename> typename U> class A {
+public:
+  A(void *) {}
+  T(A<T, U<T>>) {} // expected-error{{member 'A' cannot have template arguments}}\
+  // expected-error{{expected ';' at end of declaration list}}\
+  // expected-error{{member 'A' has the same name as its class}}
+};
+
+template <typename T> struct S {};
+
+A<int, S> foo() { return A<int, S>(nullptr); }
+
+#endif