[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

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!

lib/Sema/SemaTemplateInstantiateDecl.cpp

 //===--- SemaTemplateInstantiateDecl.cpp - C++ Template Decl Instantiation ===/
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //===----------------------------------------------------------------------===/
 //
 //  This file implements C++ template instantiation for declarations.
 //
 //===----------------------------------------------------------------------===/
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/DependentDiagnostic.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Sema/Lookup.h"

       PushExpressionEvaluationContext(Sema::ConstantEvaluated, OldVar);
     else
       PushExpressionEvaluationContext(Sema::PotentiallyEvaluated, OldVar);
 
     // Instantiate the initializer.
     ExprResult Init;
 
     {
-      ContextRAII SwitchContext(*this, Var->getDeclContext());
+      ContextRAII SwitchContext(
+          *this, Var->getDeclContext(),
+          /*NewThisContext=*/!isLambdaCallOperator(getCurLexicalContext()));
       Init = SubstInitializer(OldVar->getInit(), TemplateArgs,
                               OldVar->getInitStyle() == VarDecl::CallInit);
     }
 
     if (!Init.isInvalid()) {
       bool TypeMayContainAuto = true;
       Expr *InitExpr = Init.get();
 

test/SemaCXX/lambda-expressions.cpp

 
 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; };
+};
+
+B<int> b;
+}