Allow conditions to be decomposed with structured bindings

[lichray]

Summary

This feature was discussed but not yet proposed. It allows a structured binding to appear as a condition

if (auto [ok, val] = f(...))

So the user can save an extra condition if the statement can test the value to-be-decomposed instead. Formally, it makes the value of the underlying object of the structured binding declaration also the value of a condition that is an initialized declaration.

Considering its logicality which is entirely evident from its trivial implementation, I think it might be acceptable to land it as an extension for now before I write the paper.

[aaron.ballman]

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

[lichray]

In D39284#906450, @aaron.ballman wrote:

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

C++2a the standard itself is under development, so the users can't expect any feature to work on every compiler already.

[aaron.ballman]

In D39284#906800, @lichray wrote:

In D39284#906450, @aaron.ballman wrote:

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

C++2a the standard itself is under development, so the users can't expect any feature to work on every compiler already.

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

[lichray]

In D39284#906860, @aaron.ballman wrote:

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

No. In the first line of the Summary I said this hasn't even been proposed. But on the reflectors it received positive feedback. While I was trying to implement it in order to find out corner cases which I haven't thought of, I found that the implementation is astonishingly simple, defending the change by showing that not introducing the change to the language is merely complicating the language, so I decided to seeking for a possibility to land it first.

[aaron.ballman]

In D39284#906889, @lichray wrote:

In D39284#906860, @aaron.ballman wrote:

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

No. In the first line of the Summary I said this hasn't even been proposed. But on the reflectors it received positive feedback. While I was trying to implement it in order to find out corner cases which I haven't thought of, I found that the implementation is astonishingly simple, defending the change by showing that not introducing the change to the language is merely complicating the language, so I decided to seeking for a possibility to land it first.

Okay, then I didn't misunderstand you and we're on the same page.

We typically diagnose vendor extensions to the language, and I think we should apply that consistently. Otherwise, your code will compile fine in Clang with warning levels cranked all the way up and then fail to compile on every other compiler, which does not do our users any good. I'll let Richard have the final say for this, but my preference is that this is diagnosed as an extension (at least in pedantic mode).

[lichray]

One more test case

[lichray]

In D39284#906899, @aaron.ballman wrote:

We typically diagnose vendor extensions to the language, and I think we should apply that consistently. Otherwise, your code will compile fine in Clang with warning levels cranked all the way up and then fail to compile on every other compiler, which does not do our users any good. I'll let Richard have the final say for this, but my preference is that this is diagnosed as an extension (at least in pedantic mode).

I think we don't have a practice to issue diagnosis for a change that is basically racing with the standard. First, if we issue a diagnosis now, there is a chance for the (coming) proposal to be accepted before we making a release from trunk; second, users are supposed to be cautious using latest -std= in general, as there might be coming fixes. In addition, this change, most of the time, requires the users to opt-in by providing conversion operators, so users are expected to be intentionally trying it out rather than accidentally running into a situation that you describe.

But anyway, ping @rsmith?

[rsmith]

include/clang/Sema/DeclSpec.h

2015

If there's no grammar ambiguities here (and I don't think there are), please accept this unconditionally, with an ExtWarn.

lib/Parse/ParseExprCXX.cpp

1711

This shouldn't be listed as C++2a until it's actually voted into the working draft. Listing it as [Clang] for a clang extension is fine.

lib/Sema/SemaDeclCXX.cpp

699–701

Again, please don't guess what's going to be in C++2a. I would actually expect that we'll vote this in as a DR against C++17. It seems like a wording oversight to me.

[lichray]

Make the feature unconditional with an ExtWarn

[lichray]

lib/Sema/SemaDeclCXX.cpp

699–701

I would say, use DR with parsimony... But OK.

OT: I'm writing a paper on auto(x) and auto{x} while implementing it. Do you expect this to land as a DR? In which form you expect it to appear in Clang?

[rsmith]

lib/Sema/SemaDeclCXX.cpp

699–701

For that one, I don't know. The status quo is inconsistent (particularly comparing auto to class template argument deduction), but I don't think the inconsistency is a wording oversight. If this were to land in Clang prior to being voted into C++2a, I'd expect it to produce an ExtWarn that's not tied to any particular C++ version.

[lichray]

Tweak coding style

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

414

Phrase this as "ISO C++17 does not permit structured binding declaration in a condition"

lib/Sema/SemaDeclCXX.cpp

712–720

Using a lambda here seems like unwarranted complexity. A three-way ternary of the form

!getLangOpts().CPlusPlus1z ? diag::ext_decomp_decl :
D.getContext() == Declarator::ConditionContext ? diag::ext_decomp_decl_cond :
diag::warn_cxx14_compat_decomp_decl

would be fine. Feel free to ignore clang-format if it wants to format it stupidly.

test/Misc/warning-flags.c

19

Please read and respect this rule :)

test/Parser/cxx1z-decomposition.cpp

35–36

You have removed test coverage here, at least for the if init-statement case. Please just update the comments on these tests rather than removing them.

[lichray]

Rephrase warning message.

[lichray]

lib/Sema/SemaDeclCXX.cpp

712–720

The clang-formatted code actually looks good.

test/Misc/warning-flags.c

19

Do you know of some categories which can cover this kind of extensions?

[lichray]

Rebased.

[rsmith]

test/Misc/warning-flags.c

19

Add a new warning flag for it. Maybe -Wbinding-in-condition?

[lichray]

test/Misc/warning-flags.c

19

I wish it's still alive after Clang 6.0 released :)

[lichray]

Added -Wbinding-in-condition.

[rsmith]

Thanks, LGTM!

[lichray]

Can someone commit this please? One more patch then I'll go get a commit bit.

include/clang/Basic/DiagnosticSemaKinds.td

 // C++ decomposition declarations
 def err_decomp_decl_context : Error<
   "decomposition declaration not permitted in this context">;
 def warn_cxx14_compat_decomp_decl : Warning<
   "decomposition declarations are incompatible with "
   "C++ standards before C++17">, DefaultIgnore, InGroup<CXXPre17Compat>;
 def ext_decomp_decl : ExtWarn<
   "decomposition declarations are a C++17 extension">, InGroup<CXX17>;
+def ext_decomp_decl_cond : ExtWarn<
+  "decomposed condition is a Clang extension">;
 def err_decomp_decl_spec : Error<
   "decomposition declaration cannot be declared "
   "%plural{1:'%1'|:with '%1' specifiers}0">;
 def err_decomp_decl_type : Error<
   "decomposition declaration cannot be declared with type %0; "
   "declared type must be 'auto' or reference to 'auto'">;
 def err_decomp_decl_parens : Error<
   "decomposition declaration cannot be declared with parentheses">;

include/clang/Sema/DeclSpec.h

public:
   bool mayHaveDecompositionDeclarator() const {
     switch (Context) {
     case FileContext:
       // FIXME: It's not clear that the proposal meant to allow file-scope
       // structured bindings, but it does.
     case BlockContext:
     case ForContext:
     case InitStmtContext:
+    case ConditionContext:
       return true;
 
-    case ConditionContext:
     case MemberContext:
     case PrototypeContext:
     case TemplateParamContext:
       // Maybe one day...
       return false;
 
     // These contexts don't allow any kind of non-abstract declarator.
     case KNRTypeListContext:
     case TypeNameContext:

lib/Parse/ParseExprCXX.cpp

 
 /// ParseCXXCondition - if/switch/while condition expression.
 ///
 ///       condition:
 ///         expression
 ///         type-specifier-seq declarator '=' assignment-expression
 /// [C++11] type-specifier-seq declarator '=' initializer-clause
 /// [C++11] type-specifier-seq declarator braced-init-list
+/// [Clang] type-specifier-seq ref-qualifier[opt] '[' identifier-list ']'
+///             brace-or-equal-initializer
 /// [GNU]   type-specifier-seq declarator simple-asm-expr[opt] attributes[opt]
 ///             '=' assignment-expression
 ///
 /// In C++1z, a condition may in some contexts be preceded by an
 /// optional init-statement. This function will parse that too.
 ///
 /// \param InitStmt If non-null, an init-statement is permitted, and if present
 /// will be parsed and stored here.

lib/Sema/SemaDeclCXX.cpp

 }
 
 NamedDecl *
 Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D,
                                    MultiTemplateParamsArg TemplateParamLists) {
   assert(D.isDecompositionDeclarator());
   const DecompositionDeclarator &Decomp = D.getDecompositionDeclarator();
 
-  // The syntax only allows a decomposition declarator as a simple-declaration
-  // or a for-range-declaration, but we parse it in more cases than that.
+  // The syntax only allows a decomposition declarator as a simple-declaration,
+  // a for-range-declaration, or a condition in Clang, but we parse it in more
+  // cases than that.
   if (!D.mayHaveDecompositionDeclarator()) {
     Diag(Decomp.getLSquareLoc(), diag::err_decomp_decl_context)
       << Decomp.getSourceRange();
     return nullptr;
   }
 
   if (!TemplateParamLists.empty()) {
     // FIXME: There's no rule against this, but there are also no rules that
     // would actually make it usable, so we reject it for now.
     Diag(TemplateParamLists.front()->getTemplateLoc(),
          diag::err_decomp_decl_template);
     return nullptr;
   }
 
-  Diag(Decomp.getLSquareLoc(), getLangOpts().CPlusPlus1z
-                                   ? diag::warn_cxx14_compat_decomp_decl
-                                   : diag::ext_decomp_decl)
-      << Decomp.getSourceRange();
+  Diag(Decomp.getLSquareLoc(), [&] {
+    if (getLangOpts().CPlusPlus1z) {
+      if (D.getContext() == Declarator::ConditionContext)
+        return diag::ext_decomp_decl_cond;
+      else
+        return diag::warn_cxx14_compat_decomp_decl;
+    } else
+      return diag::ext_decomp_decl;
+  }()) << Decomp.getSourceRange();
 
   // The semantic context is always just the current context.
   DeclContext *const DC = CurContext;
 
   // C++1z [dcl.dcl]/8:
   //   The decl-specifier-seq shall contain only the type-specifier auto
   //   and cv-qualifiers.
   auto &DS = D.getDeclSpec();

test/Misc/warning-flags.c

 
 (2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (77):
+CHECK: Warnings without flags (78):
+CHECK-NEXT:   ext_decomp_decl_cond
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_initializer_string_for_char_array_too_long
 CHECK-NEXT:   ext_missing_declspec
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst

test/Parser/cxx1z-decomposition.cpp

 
 // Other kinds of declaration cannot.
 namespace OtherDecl {
   // A parameter-declaration is not a simple-declaration.
   // This parses as an array declaration.
   void f(auto [a, b, c]); // expected-error {{'auto' not allowed in function prototype}} expected-error {{'a'}}
 
   void g() {
-    // A condition is not a simple-declaration.
-    for (; auto [a, b, c] = S(); ) {} // expected-error {{not permitted in this context}}
-    if (auto [a, b, c] = S()) {} // expected-error {{not permitted in this context}}
-    if (int n; auto [a, b, c] = S()) {} // expected-error {{not permitted in this context}}
-    switch (auto [a, b, c] = S()) {} // expected-error {{not permitted in this context}}
-    switch (int n; auto [a, b, c] = S()) {} // expected-error {{not permitted in this context}}
-    while (auto [a, b, c] = S()) {} // expected-error {{not permitted in this context}}
+    // A condition is allowed as a Clang extension.
+    // See commentary in test/Parser/decomposed-condition.cpp
 
     // An exception-declaration is not a simple-declaration.
     try {}
     catch (auto [a, b, c]) {} // expected-error {{'auto' not allowed in exception declaration}} expected-error {{'a'}}
   }
 
   // A member-declaration is not a simple-declaration.
   class A {

test/Parser/decomposed-condition.cpp

+// RUN: %clang_cc1 -std=c++1z %s -verify
+
+struct Na {
+  bool flag;
+  float data;
+};
+
+struct Rst {
+  bool flag;
+  float data;
+  explicit operator bool() const {
+    return flag;
+  }
+};
+
+Rst f();
+Na g();
+
+namespace CondInIf {
+void h() {
+  if (auto [ok, d] = f()) // expected-warning {{decomposed condition is a Clang extension}}
+    ;
+  if (auto [ok, d] = g()) // expected-warning {{decomposed condition is a Clang extension}} expected-error {{value of type 'Na' is not contextually convertible to 'bool'}}
+    ;
+}
+} // namespace CondInIf
+
+namespace CondInWhile {
+void h() {
+  while (auto [ok, d] = f()) // expected-warning {{decomposed condition is a Clang extension}}
+    ;
+  while (auto [ok, d] = g()) // expected-warning {{decomposed condition is a Clang extension}} expected-error {{value of type 'Na' is not contextually convertible to 'bool'}}
+    ;
+}
+} // namespace CondInWhile
+
+namespace CondInFor {
+void h() {
+  for (; auto [ok, d] = f();) // expected-warning {{decomposed condition is a Clang extension}}
+    ;
+  for (; auto [ok, d] = g();) // expected-warning {{decomposed condition is a Clang extension}} expected-error {{value of type 'Na' is not contextually convertible to 'bool'}}
+    ;
+}
+} // namespace CondInFor
+
+struct IntegerLike {
+  bool flag;
+  float data;
+  operator int() const {
+    return int(data);
+  }
+};
+
+namespace CondInSwitch {
+void h(IntegerLike x) {
+  switch (auto [ok, d] = x) // expected-warning {{decomposed condition is a Clang extension}}
+    ;
+  switch (auto [ok, d] = g()) // expected-warning {{decomposed condition is a Clang extension}} expected-error {{statement requires expression of integer type ('Na' invalid)}}
+    ;
+}
+} // namespace CondInSwitch

test/SemaCXX/decomposed-condition.cpp

+// RUN: %clang_cc1 -std=c++1z -w -verify %s
+
+struct X {
+  bool flag;
+  int data;
+  constexpr explicit operator bool() const {
+    return flag;
+  }
+  constexpr operator int() const {
+    return data;
+  }
+};
+
+namespace CondInIf {
+constexpr int f(X x) {
+  if (auto [ok, d] = x)
+    return d + int(ok);
+  else
+    return d * int(ok);
+  ok = {}; // expected-error {{use of undeclared identifier 'ok'}}
+  d = {};  // expected-error {{use of undeclared identifier 'd'}}
+}
+
+static_assert(f({true, 2}) == 3);
+static_assert(f({false, 2}) == 0);
+
+constexpr char g(char const (&x)[2]) {
+  if (auto &[a, b] = x)
+    return a;
+  else
+    return b;
+
+  if (auto [a, b] = x) // expected-error {{an array type is not allowed here}}
+    ;
+}
+
+static_assert(g("x") == 'x');
+} // namespace CondInIf
+
+namespace CondInSwitch {
+constexpr int f(int n) {
+  switch (X s = {true, n}; auto [ok, d] = s) {
+    s = {};
+  case 0:
+    return int(ok);
+  case 1:
+    return d * 10;
+  case 2:
+    return d * 40;
+  default:
+    return 0;
+  }
+  ok = {}; // expected-error {{use of undeclared identifier 'ok'}}
+  d = {};  // expected-error {{use of undeclared identifier 'd'}}
+  s = {};  // expected-error {{use of undeclared identifier 's'}}
+}
+
+static_assert(f(0) == 1);
+static_assert(f(1) == 10);
+static_assert(f(2) == 80);
+} // namespace CondInSwitch
+
+namespace CondInWhile {
+constexpr int f(int n) {
+  int m = 1;
+  while (auto [ok, d] = X{n > 1, n}) {
+    m *= d;
+    --n;
+  }
+  return m;
+  return ok; // expected-error {{use of undeclared identifier 'ok'}}
+}
+
+static_assert(f(0) == 1);
+static_assert(f(1) == 1);
+static_assert(f(4) == 24);
+} // namespace CondInWhile
+
+namespace CondInFor {
+constexpr int f(int n) {
+  int a = 1, b = 1;
+  for (X x = {true, n}; auto &[ok, d] = x; --d) {
+    if (d < 2)
+      ok = false;
+    else {
+      int x = b;
+      b += a;
+      a = x;
+    }
+  }
+  return b;
+  return d; // expected-error {{use of undeclared identifier 'd'}}
+}
+
+static_assert(f(0) == 1);
+static_assert(f(1) == 1);
+static_assert(f(2) == 2);
+static_assert(f(5) == 8);
+} // namespace CondInFor