[Clang] Do not change the type of captured vars when checking lambda constraints

[cor3ntin]

Summary

When checking the constraint of a lambda, we need to respect the constness
of the call operator when establishing the type of capture variables.

In D124351, this was done by adding const to the captured variable...
However, that would change the type of the variable outside of the scope
of the lambda, which is clearly not the desired outcome.

Instead, to ensure const-correctness, we need to populate
a LambdaScopeInfo with the capture variables before checking the
constraints of a generic lambda.

There is no changelog as I'd like to tentatively propose we backport
this change to RC3 as it is a regression introduced in the Clang 17
cycle.

Fixes #61267

[aaron.ballman]

Adding a few more reviewers for extra sets of eyes given that we'd like this to land in 17.x. The changes look correct to me, but a confirmation would be appreciated.

[shafik]

clang/lib/Sema/SemaConcept.cpp

722

This comment does not really explain to me the effect of LSI->AfterParameterList = false;

clang/lib/Sema/SemaExpr.cpp

19753

Why move this block of code for?

[cor3ntin]

clang/lib/Sema/SemaConcept.cpp

722

AfterParameterList affects in which context we look for captured variables. I will try to think of a better comment.

clang/lib/Sema/SemaExpr.cpp

19753

isVariableAlreadyCapturedInScopeInfo is kinda badly name because it will adjust DeclRefType by adding const as necessary.

if we capture a parameter, and then use it in a concept - which are not checked from within the scope of the lambda, we need to add const to it, which we can only do by reordering these paths. It's a bit subtle, and could do with some improvement as I'm not sure parameters will always be const correct in attributes currently.
But I tried to do a minimal fix

[cor3ntin]

Improve comment

[cor3ntin]

@aaron.ballman Should we land that? it already missed rc3...

[erichkeane]

I agree with this approach, and think we should backport this if at all possible. The original solution is a bit problematic, and I don't believe this solution is particularly risky.

clang/lib/Sema/SemaConcept.cpp

727

curleys required here.

923–924

here too

clang/lib/Sema/SemaExpr.cpp

19753

Yeah, this function is unfortunately doing a lot of work and is a bit of a mess unfortunately. I think I see the logic of the reorder, but its a sensitive enough function that a minimal fix is prudent.

[cor3ntin]

Missing curlies

[aaron.ballman]

LGTM!

clang/include/clang/Sema/Sema.h

   void DefineImplicitLambdaToBlockPointerConversion(SourceLocation CurrentLoc,
                                                     CXXConversionDecl *Conv);
 
   ExprResult BuildBlockForLambdaConversion(SourceLocation CurrentLocation,
                                            SourceLocation ConvLocation,
                                            CXXConversionDecl *Conv,
                                            Expr *Src);
 
+  sema::LambdaScopeInfo *RebuildLambdaScopeInfo(CXXMethodDecl *CallOperator);
+
   /// Check whether the given expression is a valid constraint expression.
   /// A diagnostic is emitted if it is not, false is returned, and
   /// PossibleNonPrimary will be set to true if the failure might be due to a
   /// non-primary expression being used as an atomic constraint.
   bool CheckConstraintExpression(const Expr *CE, Token NextToken = Token(),
                                  bool *PossibleNonPrimary = nullptr,
                                  bool IsTrailingRequiresClause = false);
 

clang/lib/Sema/SemaConcept.cpp

 //===-- SemaConcept.cpp - Semantic Analysis for Constraints and Concepts --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements semantic analysis for C++ constraints and concepts.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Sema/SemaConcept.h"
 #include "TreeTransform.h"
 #include "clang/AST/ASTLambda.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprConcepts.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/OperatorPrecedence.h"
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Overload.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/StringExtras.h"
bool Sema::addInstantiatedCapturesToScope(
   const auto *LambdaClass = cast<CXXMethodDecl>(Function)->getParent();
   const auto *LambdaPattern = cast<CXXMethodDecl>(PatternDecl)->getParent();
 
   unsigned Instantiated = 0;
 
   auto AddSingleCapture = [&](const ValueDecl *CapturedPattern,
                               unsigned Index) {
     ValueDecl *CapturedVar = LambdaClass->getCapture(Index)->getCapturedVar();
-    if (cast<CXXMethodDecl>(Function)->isConst()) {
-      QualType T = CapturedVar->getType();
-      T.addConst();
-      CapturedVar->setType(T);
-    }
     if (CapturedVar->isInitCapture())
       Scope.InstantiatedLocal(CapturedPattern, CapturedVar);
   };
 
   for (const LambdaCapture &CapturePattern : LambdaPattern->captures()) {
     if (!CapturePattern.capturesVariable()) {
       Instantiated++;
       continue;
bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
 
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast<CXXMethodDecl>(FD)) {
     ThisQuals = Method->getMethodQualifiers();
     Record = const_cast<CXXRecordDecl *>(Method->getParent());
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
+
+  // When checking the constraints of a lambda, we need to restore a
+  // LambdaScopeInfo populated with correct capture information so that the type
+  // of a variable referring to a capture is correctly const-adjusted.
+  FunctionScopeRAII FuncScope(*this);
+  if (isLambdaCallOperator(FD)) {
+    LambdaScopeInfo *LSI = RebuildLambdaScopeInfo(
+        const_cast<CXXMethodDecl *>(cast<CXXMethodDecl>(FD)));
+    // Constraints are checked from the parent context of the lambda, so we set
+    // AfterParameterList to false, so that `tryCaptureVariable` finds
+    // explicit captures in the appropriate context.
+    LSI->AfterParameterList = false;
+  } else {
+    FuncScope.disable();
+  }
+
   return CheckConstraintSatisfaction(
       FD, {FD->getTrailingRequiresClause()}, *MLTAL,
       SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
       Satisfaction);
 }
 
 
 // Figure out the to-translation-unit depth for this function declaration for
bool Sema::CheckInstantiatedFunctionTemplateConstraints(
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
     ThisQuals = Method->getMethodQualifiers();
     Record = Method->getParent();
   }
   CXXThisScopeRAII ThisScope(*this, Record, ThisQuals, Record != nullptr);
   FunctionScopeRAII FuncScope(*this);
-  if (isLambdaCallOperator(Decl))
-    PushLambdaScope();
-  else
+
+  if (isLambdaCallOperator(Decl)) {
+    LambdaScopeInfo *LSI = RebuildLambdaScopeInfo(cast<CXXMethodDecl>(Decl));
+    LSI->AfterParameterList = false;
+  } else {
     FuncScope.disable();
+  }
 
   llvm::SmallVector<Expr *, 1> Converted;
   return CheckConstraintSatisfaction(Template, TemplateAC, Converted, *MLTAL,
                                      PointOfInstantiation, Satisfaction);
 }
 
 static void diagnoseUnsatisfiedRequirement(Sema &S,
                                            concepts::ExprRequirement *Req,

clang/lib/Sema/SemaDecl.cpp

Diag(FD->getLocation(), diag::err_redefinition_extern_inline)
         << FD << getLangOpts().CPlusPlus;
   else
     Diag(FD->getLocation(), diag::err_redefinition) << FD;
 
   Diag(Definition->getLocation(), diag::note_previous_definition);
   FD->setInvalidDecl();
 }
 
-static void RebuildLambdaScopeInfo(CXXMethodDecl *CallOperator,
-                                   Sema &S) {
-  CXXRecordDecl *const LambdaClass = CallOperator->getParent();
+LambdaScopeInfo *Sema::RebuildLambdaScopeInfo(CXXMethodDecl *CallOperator) {
+  CXXRecordDecl *LambdaClass = CallOperator->getParent();
 
-  LambdaScopeInfo *LSI = S.PushLambdaScope();
+  LambdaScopeInfo *LSI = PushLambdaScope();
   LSI->CallOperator = CallOperator;
   LSI->Lambda = LambdaClass;
   LSI->ReturnType = CallOperator->getReturnType();
   const LambdaCaptureDefault LCD = LambdaClass->getLambdaCaptureDefault();
 
   if (LCD == LCD_None)
     LSI->ImpCaptureStyle = CapturingScopeInfo::ImpCap_None;
   else if (LCD == LCD_ByCopy)
     LSI->ImpCaptureStyle = CapturingScopeInfo::ImpCap_LambdaByval;
   else if (LCD == LCD_ByRef)
     LSI->ImpCaptureStyle = CapturingScopeInfo::ImpCap_LambdaByref;
   DeclarationNameInfo DNI = CallOperator->getNameInfo();
 
   LSI->IntroducerRange = DNI.getCXXOperatorNameRange();
   LSI->Mutable = !CallOperator->isConst();
 
   // Add the captures to the LSI so they can be noted as already
   // captured within tryCaptureVar.
   auto I = LambdaClass->field_begin();
   for (const auto &C : LambdaClass->captures()) {
     if (C.capturesVariable()) {
       ValueDecl *VD = C.getCapturedVar();
       if (VD->isInitCapture())
-        S.CurrentInstantiationScope->InstantiatedLocal(VD, VD);
+        CurrentInstantiationScope->InstantiatedLocal(VD, VD);
       const bool ByRef = C.getCaptureKind() == LCK_ByRef;
       LSI->addCapture(VD, /*IsBlock*/false, ByRef,
           /*RefersToEnclosingVariableOrCapture*/true, C.getLocation(),
           /*EllipsisLoc*/C.isPackExpansion()
                          ? C.getEllipsisLoc() : SourceLocation(),
           I->getType(), /*Invalid*/false);
 
     } else if (C.capturesThis()) {
       LSI->addThisCapture(/*Nested*/ false, C.getLocation(), I->getType(),
                           C.getCaptureKind() == LCK_StarThis);
     } else {
       LSI->addVLATypeCapture(C.getLocation(), I->getCapturedVLAType(),
                              I->getType());
     }
     ++I;
   }
+  return LSI;
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
                                     SkipBodyInfo *SkipBody,
                                     FnBodyKind BodyKind) {
   if (!D) {
     // Parsing the function declaration failed in some way. Push on a fake scope
     // anyway so we can try to parse the function body.
Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   // has to be properly restored so that tryCaptureVariable doesn't try
   // and capture any new variables. In addition when calculating potential
   // captures during transformation of nested lambdas, it is necessary to
   // have the LSI properly restored.
   if (isGenericLambdaCallOperatorSpecialization(FD)) {
     assert(inTemplateInstantiation() &&
            "There should be an active template instantiation on the stack "
            "when instantiating a generic lambda!");
-    RebuildLambdaScopeInfo(cast<CXXMethodDecl>(D), *this);
+    RebuildLambdaScopeInfo(cast<CXXMethodDecl>(D));
   } else {
     // Enter a new function scope
     PushFunctionScope();
   }
 
   // Builtin functions cannot be defined.
   if (unsigned BuiltinID = FD->getBuiltinID()) {
     if (!Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) &&

clang/lib/Sema/SemaExpr.cpp

if (LSI && !LSI->AfterParameterList) {
         return true;
     }
     // If the variable is declared in the current context, there is no need to
     // capture it.
     if (IsInScopeDeclarationContext &&
         FunctionScopesIndex == MaxFunctionScopesIndex && VarDC == DC)
       return true;
 
-    // When evaluating some attributes (like enable_if) we might refer to a
-    // function parameter appertaining to the same declaration as that
-    // attribute.
-    if (const auto *Parm = dyn_cast<ParmVarDecl>(Var);
-        Parm && Parm->getDeclContext() == DC)
-      return true;
-
     // Only block literals, captured statements, and lambda expressions can
     // capture; other scopes don't work.
     DeclContext *ParentDC =
         !IsInScopeDeclarationContext
             ? DC->getParent()
             : getParentOfCapturingContextOrNull(DC, Var, ExprLoc,
                                                 BuildAndDiagnose, *this);
     // We need to check for the parent *first* because, if we *have*
do {
     CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FSI);
 
     // Check whether we've already captured it.
     if (isVariableAlreadyCapturedInScopeInfo(CSI, Var, Nested, CaptureType,
                                              DeclRefType)) {
       CSI->getCapture(Var).markUsed(BuildAndDiagnose);
       break;
     }
+
+    // When evaluating some attributes (like enable_if) we might refer to a
+    // function parameter appertaining to the same declaration as that
+    // attribute.
+    if (const auto *Parm = dyn_cast<ParmVarDecl>(Var);
+        Parm && Parm->getDeclContext() == DC)
+      return true;
+
     // If we are instantiating a generic lambda call operator body,
     // we do not want to capture new variables.  What was captured
     // during either a lambdas transformation or initial parsing
     // should be used.
     if (isGenericLambdaCallOperatorSpecialization(DC)) {
       if (BuildAndDiagnose) {
         LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(CSI);
         if (LSI->ImpCaptureStyle == CapturingScopeInfo::ImpCap_None) {

clang/test/SemaCXX/lambda-capture-type-deduction.cpp

(void)[a = 0](decltype((a)) ap) mutable {
     static_assert(is_same<int &, decltype(ap)>);
   };
   (void)[a = 0](decltype((a)) ap) {
     static_assert(is_same<const int &, decltype((a))>);
     static_assert(is_same<int, decltype(a)>);
     static_assert(is_same<int&, decltype((ap))>);
   };
 }
+
+namespace GH61267 {
+template <typename> concept C = true;
+
+template<typename>
+void f(int) {
+  int i;
+  [i]<C P>(P) {}(0);
+  i = 4;
+}
+
+void test() { f<int>(0);  }
+
+}