[Sema] Fix crash on valid where instantiation of lambda cannot access type of 'this'

[erik.pilkington]

Summary

Clang crashes during the instantiation of a lambda call operator where a VarDecl is initialized with an expression that requires the this type of the enclosing class, because when instantiating the initializing expression we introduce a new context which nulls out the Sema::CXXThisTypeOverride. This patch fixes the crash by retaining the 'this' type when the current lexical context is the body of the lambda.

This is a regression from 3.8 introduced in r267956. Fixes PR27994.

Thanks!

[faisalv]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3922 ↗

(On Diff #60062)

I don't think this is the right fix. In my opinion, the fix should be to teach getCurrentThisType to recognize that we are in a context where a lambda is being 'transformed' and correctly return the type of 'this', regardless of the containing-context of the lambda. Also see the Richard-inspired FIXME to attempt to comprehensively handle such cases.

getCurrentThisType is broken though in some other ways currently (handling the cv qualification of lambda 'this' captures by value) - and i'm going to commit my patch to fix that later today. Let me know if you'd like me to work on this next, or would prefer to take a crack at it yourself.

Thanks for looking into this - apologies for the delay in feedback.

[erik.pilkington]

This new patch changes Sema::getCurrentThisType to recover the this type when a lambda initializer is being transformed. This is done with the same code as when instantiating a generic lambda initializer. I also added a loop that finds the enclosing record, in the case where we need to access the this type through a nested lambda.

Thanks for the feedback!

[faisalv]

Hmm - after having given this some more thought - I'm not as confident about the best approach. Should we leverage the CXXThisTypeOverride mechanism (as you had done in your initial patch) - and remove the computation entirely from getCurrentThisType - or just remove CXXThisTypeOverride entirely and have getCurrentThisType be smart enough to always figure out the 'this' type? Admittedly, my slight bias is to smarty-pantify getCurrentThisType (and just remove CXXThisTypeOverride if we can do it for all cases) - but I'd be interested in the counterarguments against that. Does it really make sense to leverage and maintain both approaches within the code-base?

Richard you have any thoughts on this (pun ;)?

Apologies Erik, for the waffle here - and thanks again for putting time into it.

lib/Sema/SemaExprCXX.cpp

971

I wonder if we could just check that 'DC' is a CXXRecordDecl and just use it (so avoid computing it again below)?

[erik.pilkington]

This patch removes the handmade loop from Sema::getCurrentThisType instead just using the result of Sema::getFunctionLevelDeclContext. I believe that this will always give us the nearest CXXRecordDecl that we're looking for (right?).

For what it's worth, I agree with (Saturday) you that Sema::getCurrentThisType should be able to handle this case. That being said it seems at the very least quirky that int x; x = y; and int x = y; take an entirely different path through getCurrentThisType for y in a lambda initializer (the first using the CXXThisTypeOverride path).

@faisalv: Waffles are definitely not a problem, if that's what it takes to get the right fix! Thanks again for helping.

[faisalv]

With the assert, I would feel comfortable committing the patch. It always helps to get Richard's approval too - so feel free to ping him aggressively to see if he's interested in stopping it from being committed.

Also, I spoke with Richard & Hubert briefly at the Standards meeting in Oulu about dropping CXXThisTypeOverride - they're not sure whether we can entirely drop it (because of noexcept operands and trailing return types) without some other ugliness - but I'll try and investigate that at some point (unless you're interested in looking into it).

Thanks again Erik!

lib/Sema/SemaExprCXX.cpp

971

I wouldn't mind an assertion here that 'DC' is a CXXRecordDecl if you think this branch can only be triggered if that's the case. Like you, I can't readily think of a case where getCurrentThisType would need to be called while 'transforming' or 'parsing' a lambda and getFunctionLevelDeclContext would not return either a CXXMethorDecl or a CXXRecordDecl (assuming ThisTy is null).

[erik.pilkington]

Okay, thanks! I'll add the assert in.
@rsmith: Any thoughts on this?

[erik.pilkington]

Ping!

[erik.pilkington]

Pong!

[erik.pilkington]

Ping!

lib/Sema/SemaExprCXX.cpp

 
 QualType Sema::getCurrentThisType() {
   DeclContext *DC = getFunctionLevelDeclContext();
   QualType ThisTy = CXXThisTypeOverride;
   if (CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(DC)) {
     if (method && method->isInstance())
       ThisTy = method->getThisType(Context);
   }
-  if (ThisTy.isNull()) {
-    if (isGenericLambdaCallOperatorSpecialization(CurContext) &&
-        CurContext->getParent()->getParent()->isRecord()) {
-      // This is a generic lambda call operator that is being instantiated
-      // within a default initializer - so use the enclosing class as 'this'.
-      // There is no enclosing member function to retrieve the 'this' pointer
-      // from.
-
-      // FIXME: This looks wrong. If we're in a lambda within a lambda within a
-      // default member initializer, we need to recurse up more parents to find
-      // the right context. Looks like we should be walking up to the parent of
-      // the closure type, checking whether that is itself a lambda, and if so,
-      // recursing, until we reach a class or a function that isn't a lambda
-      // call operator. And we should accumulate the constness of *this on the
-      // way.
-
-      QualType ClassTy = Context.getTypeDeclType(
-          cast<CXXRecordDecl>(CurContext->getParent()->getParent()));
-      // There are no cv-qualifiers for 'this' within default initializers, 
-      // per [expr.prim.general]p4.
-      ThisTy = Context.getPointerType(ClassTy);
+
+  if (ThisTy.isNull() && isLambdaCallOperator(CurContext) &&
+      !ActiveTemplateInstantiations.empty()) {
+
+    // This is a lambda call operator that is being instantiated as a default
+    // initializer. We need to iterate through the potential list of parent
+    // lambda contexts to reach the enclosing class, from which we can recover
+    // the 'this' pointer type, as there is no enclosing member function.
+
+    DeclContext *IterCtx = CurContext;
+    for (;;) {
+      // Jump over both the lambda call operator and the lambda class.
+      IterCtx = IterCtx->getParent()->getParent();
+
+      if (IterCtx->isRecord())
+        break;
+
+      // FIXME: Is this possible?
+      if (!(isa<CXXMethodDecl>(IterCtx) &&
+            cast<CXXRecordDecl>(IterCtx->getParent())->isLambda()))
+        return QualType();
     }
+
+    QualType ClassTy = Context.getTypeDeclType(cast<CXXRecordDecl>(IterCtx));
+    // There are no cv-qualifiers for 'this' within default initializers,
+    // per [expr.prim.general]p4.
+    ThisTy = Context.getPointerType(ClassTy);
   }
 
   // If we are within a lambda's call operator, the cv-qualifiers of 'this'
   // might need to be adjusted if the lambda or any of its enclosing lambda's
   // captures '*this' by copy.
   if (!ThisTy.isNull() && isLambdaCallOperator(CurContext))
     return adjustCVQualifiersForCXXThisWithinLambda(FunctionScopes, ThisTy,
                                                     CurContext, Context);

test/SemaCXX/lambda-expressions.cpp

-// RUN: %clang_cc1 -std=c++11 -Wno-unused-value -fsyntax-only -verify -fblocks %s
+// RUN: %clang_cc1 -std=c++14 -Wno-unused-value -fsyntax-only -verify -fblocks %s
 // RUN: %clang_cc1 -std=c++1y -Wno-unused-value -fsyntax-only -verify -fblocks %s
 
 namespace std { class type_info; };
 
 namespace ExplicitCapture {
   class C {
     int Member;
 

 
 void foo() {
   const int num = 18;  // expected-note {{'num' declared here}}
   auto outer = []() {
     auto inner = [](S<num> &X) {};  // expected-error {{variable 'num' cannot be implicitly captured in a lambda with no capture-default specified}}
   };
 }
 }
+
+namespace PR27994 {
+struct A { template <class T> A(T); };
+
+template <class T>
+struct B {
+  int x;
+  A a = [&] { int y = x; };
+  A b = [&] { [&] { [&] { int y = x; }; }; };
+  A d = [&](auto param) { int y = x; };
+  A e = [&](auto param) { [&] { [&](auto param2) { int y = x; }; }; };
+};
+
+B<int> b;
+
+template <class T> struct C {
+  struct D {
+    int x;
+    A f = [&] { int y = x; };
+  };
+};
+
+int func() {
+  C<int> a;
+  decltype(a)::D b;
+}
+}