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

696–698

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

696–698

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

696–698

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 ↗

(On Diff #120922)

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

lib/Sema/SemaDeclCXX.cpp

715–716

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 ↗

(On Diff #120922)

Please read and respect this rule :)

test/Parser/cxx1z-decomposition.cpp

35–36 ↗

(On Diff #120922)

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

715–716

The clang-formatted code actually looks good.

test/Misc/warning-flags.c

19 ↗

(On Diff #120922)

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

[lichray]

Rebased.

[rsmith]

test/Misc/warning-flags.c

19 ↗

(On Diff #120922)

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

[lichray]

test/Misc/warning-flags.c

19 ↗

(On Diff #120922)

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/Sema/DeclSpec.h

bool mayHaveIdentifier() const {
     case TemplateTypeArgContext:
     case TrailingReturnContext:
       return false;
     }
     llvm_unreachable("unknown context kind!");
   }
 
   /// Return true if the context permits a C++17 decomposition declarator.
-  bool mayHaveDecompositionDeclarator() const {
+  bool mayHaveDecompositionDeclarator(const LangOptions &Lang) 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:
       return true;
 
     case ConditionContext:
+      return Lang.CPlusPlus2a;
     case MemberContext:
     case PrototypeContext:
     case TemplateParamContext:
       // Maybe one day...
       return false;
 
     // These contexts don't allow any kind of non-abstract declarator.
     case KNRTypeListContext:

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
+/// [C++2a] 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.
-  if (!D.mayHaveDecompositionDeclarator()) {
+  // The syntax only allows a decomposition declarator as a simple-declaration,
+  // a for-range-declaration, or a condition in C++2a, but we parse it in more
+  // cases than that.
+  if (!D.mayHaveDecompositionDeclarator(getLangOpts())) {
     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();
 
   // 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/Parser/cxx2a-decomposition.cpp

+// RUN: %clang_cc1 -std=c++2a %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())
+    ;
+  if (auto [ok, d] = g()) // expected-error {{value of type 'Na' is not contextually convertible to 'bool'}}
+    ;
+}
+} // namespace CondInIf
+
+namespace CondInWhile {
+void h() {
+  while (auto [ok, d] = f())
+    ;
+  while (auto [ok, d] = g()) // expected-error {{value of type 'Na' is not contextually convertible to 'bool'}}
+    ;
+}
+} // namespace CondInWhile
+
+namespace CondInFor {
+void h() {
+  for (; auto [ok, d] = f();)
+    ;
+  for (; auto [ok, d] = g();) // 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)
+    ;
+  switch (auto [ok, d] = g()) // expected-error {{statement requires expression of integer type ('Na' invalid)}}
+    ;
+}
+} // namespace CondInSwitch

test/SemaCXX/cxx2a-decomposition.cpp

+// RUN: %clang_cc1 -std=c++2a -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;
+}
+
+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