[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

6490–6493

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

3398

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 ↗

(On Diff #417504)

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 ↗

(On Diff #417504)

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 ↗

(On Diff #417504)

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

3398

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 ↗

(On Diff #417504)

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

16 ↗

(On Diff #417504)

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

3398

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

clang/test/SemaCXX/PR28101.cpp

8 ↗

(On Diff #417504)

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

16 ↗

(On Diff #417504)

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/Parse/ParseDecl.cpp

if (D.getCXXScopeSpec().isSet()) {
       // scope may have been entered before. Don't do it again.
       if (!D.isInvalidType() &&
           Actions.ShouldEnterDeclaratorScope(getCurScope(),
                                              D.getCXXScopeSpec()))
         // Change the declaration context for name lookup, until this function
         // is exited (and the declarator has been parsed).
         DeclScopeObj.EnterDeclaratorScope();
     }
+    if (D.hasName() && !D.getIdentifier())
+      Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
+           diag::err_expected_member_name_or_semi)
+          << (D.getDeclSpec().isEmpty() ? SourceRange()
+                                        : D.getDeclSpec().getSourceRange());
   } else if (D.mayOmitIdentifier()) {
     // This could be something simple like "int" (in which case the declarator
     // portion is empty), if an abstract-declarator is allowed.
     D.SetIdentifier(nullptr, Tok.getLocation());
 
     // The grammar for abstract-pack-declarator does not allow grouping parens.
     // FIXME: Revisit this once core issue 1488 is resolved.
     if (D.hasEllipsis() && D.hasGroupingParens())
D.AddTypeInfo(
         std::move(attrs), T.getCloseLocation());
 
     D.setGroupingParens(hadGroupingParens);
 
     // An ellipsis cannot be placed outside parentheses.
     if (EllipsisLoc.isValid())
       DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, D);
 
     return;
   }
 
   // Okay, if this wasn't a grouping paren, it must be the start of a function
   // argument list.  Recognize that this declarator will never have an
   // identifier (and remember where it would have been), then call into
   // ParseFunctionDeclarator to handle of argument list.
   D.SetIdentifier(nullptr, Tok.getLocation());
 
   // Enter function-declaration scope, limiting any declarators to the
   // function prototype scope, including parameter declarators.
   ParseScope PrototypeScope(this,

clang/lib/Sema/SemaDeclCXX.cpp

if (TemplateParameterLists.size()) {
             diag::err_template_member_noparams)
             << II
             << SourceRange(TemplateParams->getTemplateLoc(),
                 TemplateParams->getRAngleLoc());
       }
       return nullptr;
     }
 
     if (SS.isSet() && !SS.isInvalid()) {
       // The user provided a superfluous scope specifier inside a class
       // definition:
       //
       // class X {
       //   int X::member;
       // };
       if (DeclContext *DC = computeDeclContext(SS, false))
         diagnoseQualifiedDeclaration(SS, DC, Name, D.getIdentifierLoc(),
if (isTemplateInstantiation(ParentRD->getTemplateSpecializationKind())) {
 
     FieldDecl *Pattern = nullptr;
     for (auto L : Lookup) {
       if (isa<FieldDecl>(L)) {
         Pattern = cast<FieldDecl>(L);
         break;
       }
     }
+    if (LLVM_UNLIKELY(!Pattern))
+      for (const auto *FD : ClassPattern->fields())
+        if (Diags.hasErrorOccurred() && !FD->getIdentifier()) {
+          return ExprError();
+        }
     assert(Pattern && "We must have set the Pattern!");
 
     if (!Pattern->hasInClassInitializer() ||
         InstantiateInClassInitializer(Loc, Field, Pattern,
                                       getTemplateInstantiationArgs(Field))) {
       // Don't diagnose this again.
       Field->setInvalidDecl();
       return ExprError();