Implement P2173 for attributes on lambdas

[aaron.ballman]

Summary

https://wg21.link/P2173 is making its way through WG21 currently and has not been formally adopted yet. This feature provides very useful functionality in that you can specify attributes on the various function *declarations* generated by a lambda expression, where the current C++ grammar only allows attributes which apply to the various function *types* so generated.

This patch implements P2173 on the assumption that it will be adopted by WG21 with this syntax because there are no other syntactic locations to put an attribute on a lambda that will unambiguously appertain to the declaration rather than the type. Given the large number of function declaration attributes compared to function type attributes, such a construct is needed. The proposal has been approved in principle by EWG and is scheduled to go through Core wording review, so there is a strong likelihood it will be adopted, at least with this syntax. When WG21 adopts the proposal, the diagnostic can be changed as appropriate. In the unlikely event that WG21 decides not to adopt the proposal, this syntax is still a conforming extension.

I need this functionality in a downstream fork of Clang that is currently sliding some attributes written in the type position to instead apply to the declaration.

[rjmccall]

The patch seems technically okay to me. Do we need to recognize lambdas in any tentative-parse situations, or is it always the reverse (e.g. recognizing ObjC message sends as not-a-lambda)?

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

[rsmith]

In D95691#2540450, @rjmccall wrote:

The patch seems technically okay to me. Do we need to recognize lambdas in any tentative-parse situations, or is it always the reverse (e.g. recognizing ObjC message sends as not-a-lambda)?

Disambiguation with ObjC message sends never looks past the ] of the lambda introducer, so I think that's fine. And the disambiguation for [[ won't kick in here, because we've already committed to the [...] [[ introducing a lambda before we parse the attributes. The disambiguation for int a[] = { [n] = 0 }; also looks OK: it looks for an = after the ], which we can still use to disambiguate as an array designator rather than a lambda.

I think the error recovery disambiguation logic for delete [] { return p; } (); won't work if the lambda has attributes, but that seems fine: that's a best-effort recovery anyway.

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

[rjmccall]

In D95691#2540619, @rsmith wrote:

In D95691#2540450, @rjmccall wrote:

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

SGTM, then.

[aaron.ballman]

Updating based on review feedback.

[aaron.ballman]

In D95691#2540667, @rjmccall wrote:

In D95691#2540619, @rsmith wrote:

In D95691#2540450, @rjmccall wrote:

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

SGTM, then.

That works for me as well -- I'd also be very surprised if the committee rejected the feature at this point. When I originally worked on the patch, the paper hadn't started its polling in Evolution yet and so it was less clear how it would be received.

[rjmccall]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

Uh, I think we're a couple standard releases past the point at which we should have reconsidered this schema. I guess the problem is that we can't say -Wpre-c++23-compat without jumping the gun. Is there a problem with -Wc++20-compat and then having the earlier warning groups imply the later ones? That seems to be what we do with -Wc++98-compat; did we abandon that approach intentionally?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

@rsmith may have more background here. I was following the pattern already in the file, but I tend to agree that this pattern is not leading us somewhere good. FWIW, I ran into a similar situation with this on the C side of things in D95396, so we should probably be consistent there too.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

My understanding is that the command-line user is expected to pass

• clang++ -std=c++20 -Wc++11-compat to indicate "I want actually to compile in C++20 mode, but give me warnings about anything that would prevent compiling in C++11 mode"
• clang++ -std=c++17 -Wc++14-compat to indicate "I want actually to compile in C++17 mode, but give me warnings about anything that would prevent compiling in C++14 mode"
• clang++ -std=c++14 -Wc++20-compat to indicate "I want actually to compile in C++14 mode, but give me warnings about anything that would prevent compiling in C++20 mode" — EXCEPT that I think this is not supported. My impression is that forward-compatibility warnings are generally just rolled into -Wall and not handled separately beyond that?

I don't think any human user is expected to pass -Wc++98-c++11-c++14-c++17-c++20-compat by hand; it's just an internal name for a particular subset of -Wc++98-compat.

IOW, we could choose a new naming scheme for it, but that would be a purely internal change that won't affect how command-line users interact with Clang at all (for better and for worse).

[rjmccall]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

Diagnostic groups can both directly contain diagnostics and imply other diagnostic groups, so I don't think there's any reason to make a dedicated group just to contain the new diagnostics in e.g. -Wc++14-compat except to allow someone turn on those warnings separately. And it does show up to users as the warning group under -fdiagnostics-show-option (which is the default).

[rsmith]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

@Quuxplusone's comment describes the intent. -std=c++20 -Wc++14-compat should give a more or less complete list of reasons why the code would not compile in C++14 (at least on the language side; we don't check for stdlib compatibility). The other direction -- -std=c++11 -Wc++14-compat -- is more of a best-effort check for things that we've seen cause problems in practice and can easily detect. (As a consequence, I don't think there's any subset/superset relation between -Wc++X-compat and -Wc++Y-compat.)

I'd be happy to see these groups renamed to -Wpre-c++20-compat or similar. Warning group synonyms are relatively cheap, so I wouldn't be worried about adding a -Wpre-c++2b-compat now and renaming it to -Wpre-c++23-compat flag later.

(As an aside, it'd be handy if there were some way to mark a DiagGroup as existing only for grouping purposes, so that we could avoid exposing a -W flag for cases where groups are added for internal reasons.)

1023 ↗

(On Diff #321465)

I think we generally use a lowercase letter here, so CXX2b. (For example, see the C2x group.)

[rjmccall]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

Okay. It looks like -Wc++X-compat is consistently (1) all the this-feature-used-not-to-exist diagnostics from C++X and later plus (2) warnings about deprecation and semantic changes introduced by exactly version X. This seems like an unfortunate pairing, basically caused by the option names not being very clear about what kind of compatibility they mean. If we want @Quuxplusone's interpretation, which I agree is a natural human interpretation of those command lines, we'll need special support for it in diagnostic-option handling, so that we include specific diagnostics based on the relationship between the option and the language version.

There is a natural subset relationship between the this-feature-used-not-to-exist groups; we're just not taking advantage of it at all.

[rsmith]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

(2) sounds like a bug. Maybe we should add CXXPostXYCompat groups, symmetric to the CXXPreXYCompat groups, to better handle that?

I'm not sure about the need for special support in diagnostic option handling -- we don't ever produce a "you're using a feature that wasn't in an older standard mode" warning unless we're in the newer mode, and we don't ever produce a "you're using a feature that will change / go away in a newer standard mode" warning unless we're in the older mode.

I think it'd be reasonable to take advantage of the subset relationships. Back when there were only a couple of C++ language standards we cared about, the difference between linear and quadratic growth didn't really matter, but we're past that point now.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

In terms of what's reasonable for this patch, what's our path forward? It sounds like we'd like to see CXXPre2bCompat that's spelled -Wpre-c++2b-compat (and same for pedantic), and then we'll add aliases for the other language standard modes in a follow-up?

[aaron.ballman]

Updated based on review feedback.

[aaron.ballman]

In D95691#2545808, @aaron.ballman wrote:

Updated based on review feedback.

Ping

[aaron.ballman]

In D95691#2572114, @aaron.ballman wrote:

In D95691#2545808, @aaron.ballman wrote:

Updated based on review feedback.

Ping

Ping

[rjmccall]

Sorry. I have no problem with continuing the existing pattern, I guess.

[rsmith]

clang/include/clang/Basic/DiagnosticGroups.td

269 ↗

(On Diff #321465)

I'm happy with what you have here, so long as the cleanup is actually done. I don't think this inconsistent state is OK for the longer term.

[aaron.ballman]

I've commit in b2bc0a32545f9b066fed1631c6fba92a2a5a6d84 and will work on a patch for the remainder of the diagnostic groups. Thanks for the reviews!

clang/include/clang/Basic/DiagnosticParseKinds.td

def err_lambda_decl_specifier_repeated : Error<
   "%select{'mutable'|'constexpr'|'consteval'}0 cannot appear multiple times in a lambda declarator">;
 def err_lambda_capture_misplaced_ellipsis : Error<
   "ellipsis in pack %select{|init-}0capture must appear %select{after|before}0 "
   "the name of the capture">;
 def err_lambda_capture_multiple_ellipses : Error<
   "multiple ellipses in pack capture">;
 def err_capture_default_first : Error<
   "capture default must be first">;
+def ext_decl_attrs_on_lambda : ExtWarn<
+  "an attribute specifier sequence in this position is a Clang extension">,
+  InGroup<DiagGroup<"attributes-preceding-lambda-declarator">>;
+
 // C++17 lambda expressions
 def err_expected_star_this_capture : Error<
   "expected 'this' following '*' in lambda capture list">;
 
 // C++17 constexpr lambda expressions
 def warn_cxx14_compat_constexpr_on_lambda : Warning<
   "constexpr on lambda expressions is incompatible with C++ standards before C++17">,
   InGroup<CXXPre17Compat>, DefaultIgnore;

clang/lib/Parse/ParseExprCXX.cpp

       }
 
       Actions.ActOnLambdaExplicitTemplateParameterList(
           LAngleLoc, TemplateParams, RAngleLoc, RequiresClause);
       ++CurTemplateDepthTracker;
     }
   }
 
+  // Implement WG21 P2173, which allows attributes immediately before the
+  // lambda declarator and applies them to the corresponding function operator
+  // or operator template declaration. We accept this as a conforming extension
+  // in all language modes that support lambdas.
+  if (isCXX11AttributeSpecifier()) {
+    // FIXME: if the paper is adopted by WG21, this diagnostic should be
+    // replaced with one about it being an extension in older standard modes.
+    Diag(Tok.getLocation(), diag::ext_decl_attrs_on_lambda);
+    MaybeParseCXX11Attributes(D);
+  }
+
   TypeResult TrailingReturnType;
   SourceLocation TrailingReturnTypeLoc;
   if (Tok.is(tok::l_paren)) {
     ParseScope PrototypeScope(this,
                               Scope::FunctionPrototypeScope |
                               Scope::FunctionDeclarationScope |
                               Scope::DeclScope);
 

clang/test/AST/ast-dump-lambda.cpp

template <typename... Ts> void test(Ts... a) {
   [&] {};
   [&] { return c; };
   [b, &c] { return b + c; };
   [a..., x = 12] {};
   []() constexpr {};
   []() mutable {};
   []() noexcept {};
   []() -> int { return 0; };
+  [] [[noreturn]] () {};
 }
 // CHECK:Dumping test:
-// CHECK-NEXT:FunctionTemplateDecl {{.*}} <{{.*}}ast-dump-lambda.cpp:15:1, line:36:1> line:15:32{{( imported)?}} test
+// CHECK-NEXT:FunctionTemplateDecl {{.*}} <{{.*}}ast-dump-lambda.cpp:15:1, line:37:1> line:15:32{{( imported)?}} test
 // CHECK-NEXT:|-TemplateTypeParmDecl {{.*}} <col:11, col:23> col:23{{( imported)?}} referenced typename depth 0 index 0 ... Ts
-// CHECK-NEXT:`-FunctionDecl {{.*}} <col:27, line:36:1> line:15:32{{( imported)?}} test 'void (Ts...)'
+// CHECK-NEXT:`-FunctionDecl {{.*}} <col:27, line:37:1> line:15:32{{( imported)?}} test 'void (Ts...)'
 // CHECK-NEXT:  |-ParmVarDecl {{.*}} <col:37, col:43> col:43{{( imported)?}} referenced a 'Ts...' pack
-// CHECK-NEXT:  `-CompoundStmt {{.*}} <col:46, line:36:1>
+// CHECK-NEXT:  `-CompoundStmt {{.*}} <col:46, line:37:1>
 // CHECK-NEXT:    |-DeclStmt {{.*}} <line:16:3, line:21:4>
 // CHECK-NEXT:    | `-CXXRecordDecl {{.*}} <line:16:3, line:21:3> line:16:10{{( imported)?}}{{( <undeserialized declarations>)?}} struct V definition
 // CHECK-NEXT:    |   |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
 // CHECK-NEXT:    |   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
 // CHECK-NEXT:    |   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    |   | |-MoveConstructor exists simple trivial needs_implicit
 // CHECK-NEXT:    |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    |   | |-MoveAssignment exists simple trivial needs_implicit

 // CHECK-NEXT:    | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    | | | |-MoveAssignment
 // CHECK-NEXT:    | | | `-Destructor simple irrelevant trivial needs_implicit
 // CHECK-NEXT:    | | |-CXXMethodDecl {{.*}} <col:8, col:18> col:3{{( imported)?}} operator() 'auto () const noexcept' inline
 // CHECK-NEXT:    | | | `-CompoundStmt {{.*}} <col:17, col:18>
 // CHECK-NEXT:    | | |-CXXConversionDecl {{.*}} <col:3, col:18> col:3{{( imported)?}} implicit constexpr operator auto (*)() noexcept 'auto (*() const noexcept)() noexcept' inline
 // CHECK-NEXT:    | | `-CXXMethodDecl {{.*}} <col:3, col:18> col:3{{( imported)?}} implicit __invoke 'auto () noexcept' static inline
 // CHECK-NEXT:    | `-CompoundStmt {{.*}} <col:17, col:18>
-// CHECK-NEXT:    `-LambdaExpr {{.*}} <line:35:3, col:27> '(lambda at {{.*}}ast-dump-lambda.cpp:35:3)'
+// CHECK-NEXT:    |-LambdaExpr {{.*}} <line:35:3, col:27> '(lambda at {{.*}}ast-dump-lambda.cpp:35:3)'
+// CHECK-NEXT:    | |-CXXRecordDecl {{.*}} <col:3> col:3{{( imported)?}} implicit{{( <undeserialized declarations>)?}} class definition
+// CHECK-NEXT:    | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init
+// CHECK-NEXT:    | | | |-DefaultConstructor defaulted_is_constexpr
+// CHECK-NEXT:    | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+// CHECK-NEXT:    | | | |-MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT:    | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
+// CHECK-NEXT:    | | | |-MoveAssignment
+// CHECK-NEXT:    | | | `-Destructor simple irrelevant trivial needs_implicit
+// CHECK-NEXT:    | | |-CXXMethodDecl {{.*}} <col:11, col:27> col:3{{( imported)?}} operator() 'auto () const -> int' inline
+// CHECK-NEXT:    | | | `-CompoundStmt {{.*}} <col:15, col:27>
+// CHECK-NEXT:    | | |   `-ReturnStmt {{.*}} <col:17, col:24>
+// CHECK-NEXT:    | | |     `-IntegerLiteral {{.*}} <col:24> 'int' 0
+// CHECK-NEXT:    | | |-CXXConversionDecl {{.*}} <col:3, col:27> col:3{{( imported)?}} implicit constexpr operator int (*)() 'auto (*() const noexcept)() -> int' inline
+// CHECK-NEXT:    | | `-CXXMethodDecl {{.*}} <col:3, col:27> col:3{{( imported)?}} implicit __invoke 'auto () -> int' static inline
+// CHECK-NEXT:    | `-CompoundStmt {{.*}} <col:15, col:27>
+// CHECK-NEXT:    |   `-ReturnStmt {{.*}} <col:17, col:24>
+// CHECK-NEXT:    |     `-IntegerLiteral {{.*}} <col:24> 'int' 0
+// CHECK-NEXT:    `-LambdaExpr {{.*}} <line:36:3, col:23> '(lambda at {{.*}}ast-dump-lambda.cpp:36:3)'
 // CHECK-NEXT:      |-CXXRecordDecl {{.*}} <col:3> col:3{{( imported)?}} implicit{{( <undeserialized declarations>)?}} class definition
 // CHECK-NEXT:      | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init
 // CHECK-NEXT:      | | |-DefaultConstructor defaulted_is_constexpr
 // CHECK-NEXT:      | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:      | | |-MoveConstructor exists simple trivial needs_implicit
 // CHECK-NEXT:      | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:      | | |-MoveAssignment
 // CHECK-NEXT:      | | `-Destructor simple irrelevant trivial needs_implicit
-// CHECK-NEXT:      | |-CXXMethodDecl {{.*}} <col:11, col:27> col:3{{( imported)?}} operator() 'auto () const -> int' inline
-// CHECK-NEXT:      | | `-CompoundStmt {{.*}} <col:15, col:27>
-// CHECK-NEXT:      | |   `-ReturnStmt {{.*}} <col:17, col:24>
-// CHECK-NEXT:      | |     `-IntegerLiteral {{.*}} <col:24> 'int' 0
-// CHECK-NEXT:      | |-CXXConversionDecl {{.*}} <col:3, col:27> col:3{{( imported)?}} implicit constexpr operator int (*)() 'auto (*() const noexcept)() -> int' inline
-// CHECK-NEXT:      | `-CXXMethodDecl {{.*}} <col:3, col:27> col:3{{( imported)?}} implicit __invoke 'auto () -> int' static inline
-// CHECK-NEXT:      `-CompoundStmt {{.*}} <col:15, col:27>
-// CHECK-NEXT:        `-ReturnStmt {{.*}} <col:17, col:24>
-// CHECK-NEXT:          `-IntegerLiteral {{.*}} <col:24> 'int' 0
+// CHECK-NEXT:      | |-CXXMethodDecl {{.*}} <col:20, col:23> col:3{{( imported)?}} operator() 'auto () const' inline
+// CHECK-NEXT:      | | |-CompoundStmt {{.*}} <col:22, col:23>
+// CHECK-NEXT:      | | `-CXX11NoReturnAttr {{.*}} <col:8>
+// CHECK-NEXT:      | |-CXXConversionDecl {{.*}} <col:3, col:23> col:3{{( imported)?}} implicit constexpr operator auto (*)() 'auto (*() const noexcept)()' inline
+// CHECK-NEXT:      | `-CXXMethodDecl {{.*}} <col:3, col:23> col:3{{( imported)?}} implicit __invoke 'auto ()' static inline
+// CHECK-NEXT:      `-CompoundStmt {{.*}} <col:22, col:23>

clang/test/Parser/cxx0x-lambda-expressions.cpp

#if __cplusplus <= 201103L
       // expected-warning@-3{{extension}}
 #endif
       r += 2;
       return x + 2;
     } ();
   }
 
   void attributes() {
-    [] [[]] {}; // expected-error {{lambda requires '()' before attribute specifier}}
     [] __attribute__((noreturn)) {}; // expected-error {{lambda requires '()' before attribute specifier}}
     []() [[]]
       mutable {}; // expected-error {{expected body of lambda expression}}
 
     []() [[]] {};
     []() [[]] -> void {};
     []() mutable [[]] -> void {};
     []() mutable noexcept [[]] -> void {};
 
     // Testing GNU-style attributes on lambdas -- the attribute is specified
     // before the mutable specifier instead of after (unlike C++11).
     []() __attribute__((noreturn)) mutable { while(1); };
     []() mutable
       __attribute__((noreturn)) { while(1); }; // expected-error {{expected body of lambda expression}}
+
+    // Testing support for P2173 on adding attributes to the declaration
+    // rather than the type.
+    [] [[]] () {}; // expected-warning {{an attribute specifier sequence in this position is a Clang extension}}
+#if __cplusplus > 201703L
+    [] <typename> [[]] () {};  // expected-warning {{an attribute specifier sequence in this position is a Clang extension}}
+#endif
+    [] [[]] {}; // expected-warning {{an attribute specifier sequence in this position is a Clang extension}}
   }
 };
 
 template <typename>
 void PR22122() {
   [](int) -> {}; // expected-error {{expected a type}}
 }