[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 ↗

(On Diff #60477)

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 ↗

(On Diff #60622)

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!

cfe/trunk/lib/Sema/SemaExprCXX.cpp

if (isLambdaCallOperator(CurDC)) {
     }
   }
   return ASTCtx.getPointerType(ClassType);
 }
 
 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()) {
+
+    assert(isa<CXXRecordDecl>(DC) &&
+           "Trying to get 'this' type from static method?");
+
+    // This is a lambda call operator that is being instantiated as a default
+    // initializer. DC must point to the enclosing class type, so we can recover
+    // the 'this' type from it.
+
+    QualType ClassTy = Context.getTypeDeclType(cast<CXXRecordDecl>(DC));
+    // There are no cv-qualifiers for 'this' within default initializers,
 
   // 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);
   return ThisTy;

cfe/trunk/test/SemaCXX/lambda-expressions.cpp

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

 
 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;
+}
+}