[Clang] Use of decltype(capture) in parameter-declaration-clause

[cor3ntin]

Summary

Partially implement the proposed resolution to CWG2569.

D119136 broke some libstdc++ code, as P2036R3, implemented as a DR to
C++11 made ill-formed some previously valid and innocuous code.

We resolve this issue to allow decltype(x) - but not decltype((x)
to appear in the parameter list of a lambda that capture x by copy.

Unlike CWG2569, we do not extend that special treatment to
sizeof/noexcept yet, as the resolution has not been approved yet
and keeping the review small allows a quicker fix of impacted code.

[aaron.ballman]

clang/include/clang/Sema/Sema.h

5281

This parameter is always false, so it seems like it can be dropped.

clang/lib/Parse/ParseExprCXX.cpp

688–694

700

This feels surprisingly restrictive to me, but it's sufficient for getting libstdc++ working again.

713–715

In the case we get an error from parsing the unqualified id, I don't think we want to immediately try to parse an expression, right? Seems like a case to return an ExprError

[cor3ntin]

clang/lib/Parse/ParseExprCXX.cpp

700

I've can't think of another kind of unqualified-id that could refer to a capturable entity here.
Do you know what would be missing?
Also, without being very restrictive, I'm not sure how this is implementable

[cor3ntin]

clang/include/clang/Sema/Sema.h

5281

Not if we extend to sizeof id - I guess i can drop it for now?

[cor3ntin]

Address Aaron's feedback

• Typos
• Remove unused parameters
• return ExprError() if ParseUnqualifiedId fails.

[aaron.ballman]

clang/include/clang/Sema/Sema.h

5281

I'd drop for now -- we can add it in later when we get around to doing the full core issue once it's settled.

clang/lib/Parse/ParseExprCXX.cpp

700

What I find restrictive is that it requires an unqualified-id *only* to kick in as being maybe mutable agnostic. e.g., decltype(id) and decltype(+id) (where the unary + may be an overloaded operator that depends on the mutability of the this object).

(I'm not seeing the principle that guides this restriction proposed in the core issue yet, but those can be hashed out later.)

[aaron.ballman]

We should probably add a release note that says we partially implement CWG 2569 (just the decltype portion) and that the core issue has not yet been approved by WG21 but the changes are needed to keep libstdc++ working.

clang/lib/Parse/ParseExprCXX.cpp

710

Sorry, I forgot to mention this before, but there should be some test coverage for this case to make sure the behavior is still reasonable. Something like:

void whatever() {
  [=]<typename T = decltype(b)>(){};
}

(I'd expect we get some sort of lookup error for b.)

[cor3ntin]

clang/lib/Parse/ParseExprCXX.cpp

700

I do not think there is a general answer to "given these captures and this arbitrarily complex expression", would the mutability of the lambda affect the declaration of the parameters.
In this case we know it doesn't and that happen to fix existing code, which make the solution tractable.
If core was keen to add any more complexity here, I think they should consider mandating a look-ahead of the mutable keyword even if that sounds rather complex. I don't think it will come to that, the proposed resolution will be good enough to reduce the blast radius of the paper, and I don't see a strong motivation to add even more complexity here.

[aaron.ballman]

clang/lib/Parse/ParseExprCXX.cpp

700

Eh, I'm less convinced. The changes in P2036R3 were a band-aid to fix mistakes. The core issue "fixing" it is another band-aid over the top of P2036R3. My experience is that band-aids over band-aids rarely leads to a good feature that users can make sense of. The fact that I can't make heads or tails of what the actual language design rule is here also means this is painful for us supporting our own extensions (like alignof on an object instead of a type, __typeof__ instead of decltype, etc).

Personally, I'd prefer the core issue was resolved in a more clean fashion than carving out a few exceptions under the hope that it's "good enough". However, that's something for us to fight about in Core when this comes up. For the moment, I'm happy enough with this because it gets libstdc++ unstuck.

[cor3ntin]

clang/lib/Parse/ParseExprCXX.cpp

700

The changes in P2036R3 were a band-aid to fix mistakes. The core issue "fixing" it is another band-aid over the top of P2036R3.

Agreed, but, the question is how many people will run into scenario not covered by the band-aid in existing code?
And this is why I'm not a fan of generalizing to sizeof/noexcept because it' sounds arbitrary and unmotivated, and if we have more than one bandaid we should consider a general solution.

Basically either that or mutable look-ahead, i don't see an in-between solution as being viable.

[erichkeane]

1 nit, otherwise no comments.

clang/lib/Sema/SemaExpr.cpp

2703

I would probably prefer a RAII object for this bool. It makes it easier later on if we decide this is something that needs to be enabled/disabled in a stack.

[cor3ntin]

• Add a release note
• Add tests for lookup errors in decltype expressions in lambda parameters clause

[cor3ntin]

• Use a RAII class to control InMutableAgnosticContext
• Formatting

clang/lib/Sema/SemaExpr.cpp

2703

Agreed, it's better

[aaron.ballman]

LGTM aside from a nit (forgot to remove a line covered by RAII).

clang/lib/Sema/SemaExpr.cpp

2704–2712

[cor3ntin]

Thanks Aaron, let's pretend this did not happen :)

[cor3ntin]

... and we don't need a local variable anymore either

[cor3ntin]

@aaron.ballman @erichkeane Thanks for the review - I've landed it. Hopefully the remediation should be enough so that libstdc++ is no longer affected but we should still keep an eye out for further issues

[hokein]

Hi, this patch seems to break the following code which was previously compiled:

#include <type_traits>
#include <algorithm>
#include <numeric>

template <typename It, typename MapFn>
auto MapJoin(It first, It last, MapFn map_fn) {
  return std::accumulate(
      first, last, map_fn(*first),
      [=](typename std::result_of<MapFn(decltype(*first))>::type result) {  }); // a new diagnostic: error: captured variable 'first' cannot appear here
}

[aaron.ballman]

In D123909#3461716, @hokein wrote:

Hi, this patch seems to break the following code which was previously compiled:

#include <type_traits>
#include <algorithm>
#include <numeric>

template <typename It, typename MapFn>
auto MapJoin(It first, It last, MapFn map_fn) {
  return std::accumulate(
      first, last, map_fn(*first),
      [=](typename std::result_of<MapFn(decltype(*first))>::type result) {  }); // a new diagnostic: error: captured variable 'first' cannot appear here
}

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

[rsmith]

In D123909#3462560, @aaron.ballman wrote:

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

It sounds to me like this change is causing a substantial amount of breakage for real code, which might suggest that applying it retroactively by default is not in the best interests of our users. I think we need more information here on the scope of the breakages, but it might make sense to suggest that CWG reconsiders treating this as a DR.

[aaron.ballman]

In D123909#3462791, @rsmith wrote:

In D123909#3462560, @aaron.ballman wrote:

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

It sounds to me like this change is causing a substantial amount of breakage for real code, which might suggest that applying it retroactively by default is not in the best interests of our users. I think we need more information here on the scope of the breakages, but it might make sense to suggest that CWG reconsiders treating this as a DR.

Based on the amount of code broken thus far, I tend to agree that P2036R3 should not be a DR (or perhaps retracted entirely for C++23 until these issues are addressed by the paper author). But I agree that we need more information on the scope of the breakages, which is why I was sad to see all of this reverted without prior discussion.

clang/include/clang/Parse/Parser.h

private:
   ExprResult ParseFoldExpression(ExprResult LHS, BalancedDelimiterTracker &T);
 
   //===--------------------------------------------------------------------===//
   // C++ Expressions
   ExprResult tryParseCXXIdExpression(CXXScopeSpec &SS, bool isAddressOfOperand,
                                      Token &Replacement);
   ExprResult ParseCXXIdExpression(bool isAddressOfOperand = false);
 
+  ExprResult ParseCXXMaybeMutableAgnosticExpression();
+
   bool areTokensAdjacent(const Token &A, const Token &B);
 
   void CheckForTemplateAndDigraph(Token &Next, ParsedType ObjectTypePtr,
                                   bool EnteringContext, IdentifierInfo &II,
                                   CXXScopeSpec &SS);
 
   bool ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS,
                                       ParsedType ObjectType,

clang/include/clang/Sema/Sema.h

public:
   /// Stack containing information about each of the nested
   /// function, block, and method scopes that are currently active.
   SmallVector<sema::FunctionScopeInfo *, 4> FunctionScopes;
 
   /// The index of the first FunctionScope that corresponds to the current
   /// context.
   unsigned FunctionScopesStart = 0;
 
+  /// Whether we are currently in the context of a mutable agnostic identifier
+  /// as described by CWG2569.
+  /// We are handling the unqualified-id of a decltype or noexcept expression.
+  bool InMutableAgnosticContext = false;
+
   ArrayRef<sema::FunctionScopeInfo*> getFunctionScopes() const {
     return llvm::makeArrayRef(FunctionScopes.begin() + FunctionScopesStart,
                               FunctionScopes.end());
   }
 
   /// Stack containing information needed when in C++2a an 'auto' is encountered
   /// in a function declaration parameter type specifier in order to invent a
   /// corresponding template parameter in the enclosing abbreviated function
public:
   SourceRange getExprRange(Expr *E) const;
 
   ExprResult ActOnIdExpression(
       Scope *S, CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
       UnqualifiedId &Id, bool HasTrailingLParen, bool IsAddressOfOperand,
       CorrectionCandidateCallback *CCC = nullptr,
       bool IsInlineAsmIdentifier = false, Token *KeywordReplacement = nullptr);
 
+  ExprResult ActOnMutableAgnosticIdExpression(Scope *S, CXXScopeSpec &SS,
+                                              UnqualifiedId &Id);
+
   void DecomposeUnqualifiedId(const UnqualifiedId &Id,
                               TemplateArgumentListInfo &Buffer,
                               DeclarationNameInfo &NameInfo,
                               const TemplateArgumentListInfo *&TemplateArgs);
 
   bool DiagnoseDependentMemberLookup(LookupResult &R);
 
   bool
   DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,

clang/lib/Parse/ParseDeclCXX.cpp

if (Tok.is(tok::kw_auto) && NextToken().is(tok::r_paren)) {
       // Parse the expression
 
       // C++11 [dcl.type.simple]p4:
       //   The operand of the decltype specifier is an unevaluated operand.
       EnterExpressionEvaluationContext Unevaluated(
           Actions, Sema::ExpressionEvaluationContext::Unevaluated, nullptr,
           Sema::ExpressionEvaluationContextRecord::EK_Decltype);
       Result = Actions.CorrectDelayedTyposInExpr(
-          ParseExpression(), /*InitDecl=*/nullptr,
+          ParseCXXMaybeMutableAgnosticExpression(), /*InitDecl=*/nullptr,
           /*RecoverUncorrectedTypos=*/false,
           [](Expr *E) { return E->hasPlaceholderType() ? ExprError() : E; });
       if (Result.isInvalid()) {
         DS.SetTypeSpecError();
         if (SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch)) {
           EndLoc = ConsumeParen();
         } else {
           if (PP.isBacktrackEnabled() && Tok.is(tok::semi)) {

clang/lib/Parse/ParseExprCXX.cpp

if (Result.isUnset()) {
     UnconsumeToken(Replacement);
     Result = tryParseCXXIdExpression(SS, isAddressOfOperand, Replacement);
   }
   assert(!Result.isUnset() && "Typo correction suggested a keyword replacement "
                               "for a previous keyword suggestion");
   return Result;
 }
 
+/// ParseCXXMaybeMutableAgnosticExpression - Handle expressions inside of
+/// sizeof, decltype, noexcept
+/// - unqualified-id
+/// - expression
+/// This serves to silence errors about captured variable referred in lambda
+/// parameter list, if they are used as the unqualified-id of a decltype,
+/// sizeof, or noexcept expression.
+ExprResult Parser::ParseCXXMaybeMutableAgnosticExpression() {
+
+  if (!getLangOpts().CPlusPlus11)
+    return ParseExpression();
+
+  if (Tok.is(tok::identifier) && NextToken().is(tok::r_paren)) {
+    UnqualifiedId Name;
+    CXXScopeSpec SS;
+    if (ParseUnqualifiedId(SS, /*ObjectType=*/nullptr,
+                           /*ObjectHadErrors=*/false,
+                           /*EnteringContext=*/false,
+                           /*AllowDestructorName=*/false,
+                           /*AllowConstructorName=*/false,
+                           /*AllowDeductionGuide=*/false,
+                           /*TemplateKWLoc=*/nullptr, Name))
+      return ExprError();
+    return Actions.ActOnMutableAgnosticIdExpression(getCurScope(), SS, Name);
+  }
+  return ParseExpression();
+}
+
 /// ParseLambdaExpression - Parse a C++11 lambda expression.
 ///
 ///       lambda-expression:
 ///         lambda-introducer lambda-declarator compound-statement
 ///         lambda-introducer '<' template-parameter-list '>'
 ///             requires-clause[opt] lambda-declarator compound-statement
 ///
 ///       lambda-introducer:

clang/lib/Sema/SemaExpr.cpp

if (TemplateArgs || TemplateKWLoc.isValid()) {
     }
 
     return BuildTemplateIdExpr(SS, TemplateKWLoc, R, ADL, TemplateArgs);
   }
 
   return BuildDeclarationNameExpr(SS, R, ADL);
 }
 
+ExprResult Sema::ActOnMutableAgnosticIdExpression(Scope *S, CXXScopeSpec &SS,
+                                                  UnqualifiedId &Id) {
+  InMutableAgnosticContext = true;
+  ExprResult Res = ActOnIdExpression(S, SS, /*TemplateKwLoc*/
+                                     SourceLocation(), Id,
+                                     /*HasTrailingLParen*/ false,
+                                     /*IsAddressOfOperand*/ false,
+                                     /*CorrectionCandidateCallback*/ nullptr,
+                                     /*IsInlineAsmIdentifier*/ false,
+                                     /*KeywordReplacement*/ nullptr);
+  InMutableAgnosticContext = false;
+  return Res;
+}
+
 /// BuildQualifiedDeclarationNameExpr - Build a C++ qualified
 /// declaration name, generally during template instantiation.
 /// There's a large number of things which don't need to be done along
 /// this path.
 ExprResult Sema::BuildQualifiedDeclarationNameExpr(
     CXXScopeSpec &SS, const DeclarationNameInfo &NameInfo,
     bool IsAddressOfOperand, const Scope *S, TypeSourceInfo **RecoveryTSI) {
   DeclContext *DC = computeDeclContext(SS, false);
Sema.Diag(DefaultInsertLoc, diag::note_lambda_default_capture_fixit)
         << /*reference*/ 1
         << FixItHint::CreateInsertion(DefaultInsertLoc, FixBuffer);
   }
 }
 
 static bool CheckCaptureUseBeforeLambdaQualifiers(Sema &S, VarDecl *Var,
                                                   SourceLocation ExprLoc,
                                                   LambdaScopeInfo *LSI) {
+
+  // Allow `[a = 1](decltype(a)) {}` as per CWG2569.
+  if (S.InMutableAgnosticContext)
+    return true;
+
   if (Var->isInvalidDecl())
     return false;
 
   bool ByCopy = LSI->ImpCaptureStyle == LambdaScopeInfo::ImpCap_LambdaByval;
   SourceLocation Loc = LSI->IntroducerRange.getBegin();
   bool Explicitly = false;
   for (auto &&C : LSI->DelayedCaptures) {
     VarDecl *CV = C.second.Var;
bool Sema::tryCaptureVariable(
   do {
     IsInLambdaBeforeQualifiers = false;
 
     LambdaScopeInfo *LSI = nullptr;
     if (!FunctionScopes.empty())
       LSI = dyn_cast_or_null<LambdaScopeInfo>(
           FunctionScopes[FunctionScopesIndex]);
     if (LSI && LSI->BeforeLambdaQualifiersScope) {
-      if (isa<ParmVarDecl>(Var))
+      if (isa<ParmVarDecl>(Var) && !Var->getDeclContext()->isFunctionOrMethod())
         return true;
       IsInLambdaBeforeQualifiers = true;
       if (!CheckCaptureUseBeforeLambdaQualifiers(*this, Var, ExprLoc, LSI)) {
         break;
       }
     }
 
     // If the variable is declared in the current context, there is no need to

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

[x = 1]() mutable requires is_same<const int &, decltype((x))> {} (); // expected-error {{no matching function for call to object of type}} \
                                                                      // expected-note {{candidate function not viable}} \
                                                                      // expected-note {{'is_same<const int &, decltype((x))>' evaluated to false}}
 }
 
 void err() {
   int y, z;                // expected-note 2{{declared here}}
   auto implicit_tpl = [=]( // expected-note {{variable 'y' is captured here}}
                           decltype(
-                              [&]<decltype(y)> { return 0; }) y) { //expected-error{{captured variable 'y' cannot appear here}}
+                              [&]<decltype((y))> { return 0; }) y) { // expected-error{{captured variable 'y' cannot appear here}}
     return y;
   };
 
-  auto init_tpl = [x = 1](                                          // expected-note{{explicitly captured here}}
-                      decltype([&]<decltype(x)> { return 0; }) y) { // expected-error {{captured variable 'x' cannot appear here}}
+  auto init_tpl = [x = 1](                                            // expected-note{{explicitly captured here}}
+                      decltype([&]<decltype((x))> { return 0; }) y) { // expected-error {{captured variable 'x' cannot appear here}}
     return x;
   };
 
   auto implicit = [=]( // expected-note {{variable 'z' is captured here}}
                       decltype(
-                          [&](decltype(z)) { return 0; }) z) { //expected-error{{captured variable 'z' cannot appear here}}
+                          [&](decltype((z))) { return 0; }) z) { // expected-error{{captured variable 'z' cannot appear here}}
     return z;
   };
 
-  auto init = [x = 1](                                          // expected-note{{explicitly captured here}}
-                  decltype([&](decltype(x)) { return 0; }) y) { // expected-error {{captured variable 'x' cannot appear here}}
+  auto init = [x = 1](                                            // expected-note{{explicitly captured here}}
+                  decltype([&](decltype((x))) { return 0; }) y) { // expected-error {{captured variable 'x' cannot appear here}}
     return x;
   };
 
   auto use_before_params = [x = 1]<typename T>  // expected-note {{variable 'x' is explicitly captured here}}
   requires(is_same<const int &, decltype((x))>) // expected-error {{captured variable 'x' cannot appear here}}
   {};
 
   auto use_before_params2 = [x = 1]<typename T =  decltype((x))>  // expected-note {{variable 'x' is explicitly captured here}} \

 }
 
 void nested() {
   int x, y, z; // expected-note {{'x' declared here}} expected-note {{'z' declared here}}
   (void)[&](
       decltype([&](
                    decltype([=]( // expected-note {{variable 'x' is captured here}}
                                 decltype([&](
-                                             decltype([&](decltype(x)) {}) // expected-error{{captured variable 'x' cannot appear here}}
+                                             decltype([&](decltype((x))) {}) // expected-error{{captured variable 'x' cannot appear here}}
                                          ) {})) {})) {})){};
 
   (void)[&](
       decltype([&](
                    decltype([&](
                                 decltype([&](
-                                             decltype([&](decltype(y)) {})) {})) {})) {})){};
+                                             decltype([&](decltype((y))) {})) {})) {})) {})){};
 
   (void)[=](
       decltype([=](
                    decltype([=](
-                                decltype([=](                              // expected-note {{variable 'z' is captured here}}
-                                             decltype([&]<decltype(z)> {}) // expected-error{{captured variable 'z' cannot appear here}}
+                                decltype([=](                                // expected-note {{variable 'z' is captured here}}
+                                             decltype([&]<decltype((z))> {}) // expected-error{{captured variable 'z' cannot appear here}}
                                          ) {})) {})) {})){};
 }
 
 template <typename T, typename U>
 void dependent(U&& u) {
   [&]() requires is_same<decltype(u), T> {}();
 }
 
 void test_dependent() {
   int v   = 0;
   int & r = v;
   const int & cr = v;
   dependent<int&>(v);
   dependent<int&>(r);
   dependent<const int&>(cr);
 }
+
+void test_CWG2569_tpl(auto a) {
+  (void)[=]<typename T = decltype(a)>(decltype(a) b = decltype(a)()){};
+}
+
+void test_CWG2569() {
+  int a = 0;
+  (void)[=]<typename T = decltype(a)>(decltype(a) b = decltype(a)()){};
+  test_CWG2569_tpl(0);
+}