Mark lambda decl as invalid if a captured variable has an invalid type.

[jgorbe]

Summary

This causes the compiler to crash when trying to compute a layout for
the lambda closure type (see included test).

[rsmith]

Thanks!

clang/lib/Sema/SemaExpr.cpp

14946

You'll need to use FieldType->getBaseElementTypeUnsafe() or similar here to handle the case where the field type is an array whose element type is a class. (That can happen in a lambda if you capture a local array variable by value.)

14948

For consistency with the class case, I think we should mark the field invalid in this case too.

[jgorbe]

Fixed some issues pointed out in review comments:

• call to getBaseElementType before checking type validity.
• when the type is incomplete, mark not only the lambda closure type as invalid but also the field

Also, added a couple of checks to resemble more closely the code that checks user-declared fields in classes.

[jgorbe]

clang/lib/Sema/SemaExpr.cpp

14946

Done, I ended up using Context.getBaseElementType because the call to RequireCompleteType below required a QualType.

14948

Done!

[rsmith]

Thanks! Some simplifications are possible here, but otherwise this looks good.

clang/lib/Sema/SemaExpr.cpp

14946

Hah, OK, now you've switched to using RequireCompleteType this has become redundant again (it'll walk to the base element type for you). =)

14952–14958

I believe the else case here is redundant: if RequireCompleteType returns false, the type is complete and there's nothing more you need to do.

clang/test/SemaCXX/lambda-invalid-capture.cpp

9–13

Can you also add a test for the "capturing an array of incomplete type" case please?

[jgorbe]

Added a test for the "capturing an array of incomplete type" case. See also responses to inline comments below.

clang/lib/Sema/SemaExpr.cpp

14946

Removed call to getBaseElementType(), changed appearances of EltTy to FieldType instead.

14952–14958

If I just remove the else block, the test case included with the patch keeps crashing. Is there any way a Decl can be invalid even after RequireCompleteType returns false?

[rsmith]

Looks good, thanks! (Though we generally prefer fewer test files with more test per file, because a lot of the overhead of running tests is the setup / teardown time, so can you merge the two added tests into a single file?)

clang/lib/Sema/SemaExpr.cpp

14952–14958

I think I was just wrong before :) Looks like RequireCompleteType doesn't check whether the type is invalid.

[jgorbe]

Folded the two test cases (capturing an invalid type and capturing an invalid array type) into a single file.

clang/lib/Sema/SemaExpr.cpp

static void addAsFieldToClosureType(Sema &S, LambdaScopeInfo *LSI,
                                     bool RefersToCapturedVariable) {
   CXXRecordDecl *Lambda = LSI->Lambda;
 
   // Build the non-static data member.
   FieldDecl *Field
     = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
                         S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
                         nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (const CXXRecordDecl *RD = FieldType->getAsCXXRecordDecl()) {
+    if (RD->isInvalidDecl()) {
+      Lambda->setInvalidDecl();
+    }
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);
 }
 
 /// Capture the given variable in the lambda.
 static bool captureInLambda(LambdaScopeInfo *LSI,
                             VarDecl *Var,
                             SourceLocation Loc,
                             const bool BuildAndDiagnose,
                             QualType &CaptureType,
                             QualType &DeclRefType,
                             const bool RefersToCapturedVariable,
                             const Sema::TryCaptureKind Kind,
                             SourceLocation EllipsisLoc,
                             const bool IsTopScope,

clang/test/SemaCXX/lambda-invalid-capture.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() { 
+  g child;    
+  auto q = [child]{}; 
+  const int n = sizeof(q); 
+}