[clang] Also consider structured bindings when checking whether a local entity is odr-used in a default argument.

[riccibruno]

Summary

Currently clang accepts a default argument containing an odr-used structured binding which is not odr-usable here:

void test() {
  struct S { int i, j; };
  auto [x, y] = S();
  extern void h(int = x); // Should be rejected.
}

It was not entirely clear in C++17 that this was forbidden since [dcl.fct.default]p7 only used the term "local variable". However this is clearly forbidden in C++20 with the new wording in term of odr-usable local entities.

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

101–105

Is there a reason to separate these two cases? (Could we just use the decomposed decl unconditionally?) Generally treating tuple-like decompositions differently from other kinds seems error-prone.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp

51

The diagnostic in this case is inconsistent with the non-tuple-like cases. I think this diagnostic is better; we should use the original Decl when producing the diagnostic, not the decomposed variable.

[riccibruno]

Refer to the binding in the diagnostic.

[riccibruno]

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp

51

Right, let's just refer to the binding in all cases.

[rsmith]

clang/include/clang/AST/DeclCXX.h

3843 ↗

(On Diff #284364)

!! This seems pretty bad; would it be hard to fix?

[riccibruno]

clang/include/clang/AST/DeclCXX.h

3843 ↗

(On Diff #284364)

On further examination, I believe that`Decomp` is set but this is subtle, and it is likely that I am missing a case/wrong somehow.

The expression for the binding (Binding) will be deserialized when visiting the BindingDecl. This expression when non-null will always (as far as I can tell) contain a reference to the decomposition declaration so the decomposition will be deserialized, which will set Decomp.

The binding expression is null when the initializer is type-dependent. But then, since variable template decompositions are not allowed, the decomposition must occurs at block scope (mayHaveDecompositionDeclarator). This means that the DecompositionDecl will be read first.

[riccibruno]

clang/lib/Sema/SemaDeclCXX.cpp

126

This can use DiagnoseIfOdrUse as soon as the inconsistency between parameters and local variables is removed.

[riccibruno]

Remove the now-unused const VarDecl * parameter to DiagnoseIfOdrUse.

[aaronpuchert]

clang/lib/Sema/SemaDeclCXX.cpp

91

Since you're emitting a specific warning, I think you can just hardcode the expected types here instead of using const auto&.... Alternatively you could guess the integer argument from the (dynamic) type of the VarDecl argument.

162

Have you seen a case there the _or_null is relevant? To me it seems like this shouldn't happen, at least not when we get here.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp

40–47

Better use a relative line number: [[@LINE-5]] or something like that.

[riccibruno]

clang/lib/Sema/SemaDeclCXX.cpp

91

I am using a pack here (changed a bit in the latest revision) to make it easy to re-use this lambda when the inconsistency between parameters and local variables is removed.

162

I have removed it in the latest revision since we should fix the (de)serialization instead if this is possible.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp

40–47

I wish I could. Unfortunately [[@LINE-5]] is a CHECK line construct. Is it possible to refer to a relative line in a regex verify line?

[aaronpuchert]

Ah, I guess Ianded on an older version of this through a link and didn't notice. Nevermind.

clang/lib/Sema/SemaDeclCXX.cpp

bool CheckDefaultArgumentVisitor::VisitExpr(const Expr *Node) {
   return IsInvalid;
 }
 
 /// VisitDeclRefExpr - Visit a reference to a declaration, to
 /// determine whether this declaration can be used in the default
 /// argument expression.
 bool CheckDefaultArgumentVisitor::VisitDeclRefExpr(const DeclRefExpr *DRE) {
   const NamedDecl *Decl = DRE->getDecl();
+
+  // C++20 [basic.pre]p7:
+  //   A local entity is [...] a structured binding whose corresponding variable
+  //   is such an entity [...]
+  //
+  // C++20 [basic.def.odr]p9:
+  //   A local entity (6.1) is odr-usable in a declarative region if [...]
+  //
+  // Note that this was not entirely clear in C++17 since [dcl.fct.default]p7
+  // only prohibited local variables (a structured binding declaration
+  // introduces identifiers as names).
+  if (const auto *Binding = dyn_cast<BindingDecl>(Decl)) {
+    if (const VarDecl *HoldingVar = Binding->getHoldingVar()) {
+      // C++20 [dcl.struct.bind]p4:
+      //   Each vi is the name [...] that refers to the object bound to ri [...]
+      Decl = HoldingVar;
+    } else {
+      // C++20 [dcl.struct.bind]p3:
+      //   Each vi is the name of an lvalue that refers to the element i of
+      //   the array [...]
+      //
+      // C++20 [dcl.struct.bind]p5:
+      //   [...] each vi is the name of an lvalue that refers to the member mi
+      //   of e [...]
+      Decl = Binding->getDecomposedDecl();
+    }
+  }
+
   if (const auto *Param = dyn_cast<ParmVarDecl>(Decl)) {
     // C++ [dcl.fct.default]p9:
     //   [...] parameters of a function shall not be used in default
     //   argument expressions, even if they are not evaluated. [...]
     //
     // C++17 [dcl.fct.default]p9 (by CWG 2082):
     //   [...] A parameter shall not appear as a potentially-evaluated
     //   expression in a default argument. [...]
     //
     if (DRE->isNonOdrUse() != NOUR_Unevaluated)
       return S.Diag(DRE->getBeginLoc(),
                     diag::err_param_default_argument_references_param)
              << Param << DefaultArg->getSourceRange();
   } else if (const auto *VDecl = dyn_cast<VarDecl>(Decl)) {
     // C++ [dcl.fct.default]p7:
     //   Local variables shall not be used in default argument
     //   expressions.
     //
bool CheckDefaultArgumentVisitor::VisitCXXThisExpr(const CXXThisExpr *ThisE) {
   //   The keyword this shall not be used in a default argument of a
   //   member function.
   return S.Diag(ThisE->getBeginLoc(),
                 diag::err_param_default_argument_references_this)
          << ThisE->getSourceRange();
 }
 
 bool CheckDefaultArgumentVisitor::VisitPseudoObjectExpr(
     const PseudoObjectExpr *POE) {
   bool Invalid = false;
   for (const Expr *E : POE->semantics()) {
     // Look through bindings.
     if (const auto *OVE = dyn_cast<OpaqueValueExpr>(E)) {
       E = OVE->getSourceExpr();
       assert(E && "pseudo-object binding without source expression?");
     }
 

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp

 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
 
+namespace std {
+using size_t = decltype(sizeof(int));
+template <typename> struct tuple_size;
+template <size_t, typename> struct tuple_element;
+} // namespace std
+using size_t = std::size_t;
+
+struct E {};
+template <size_t> int get(E);
+
+namespace std {
+template <> struct tuple_size<E> { static constexpr size_t value = 2; };
+template <size_t I> struct tuple_element<I, E> { using type = int; };
+}; // namespace std
+
 void h() {
   int i1 = 0;
   extern void h1(int x = i1);
   // expected-error@-1 {{default argument references local variable 'i1' of enclosing function}}
 
   const int i2 = 0;
   extern void h2a(int x = i2);     // ok, not odr-use
   extern void h2b(int x = i2 + 0); // ok, not odr-use
 
   const int i3 = 0;
   extern void h3(const int *x = &i3);
   // expected-error@-1 {{default argument references local variable 'i3' of enclosing function}}
 
   const int i4 = 0;
   extern void h4(int x = sizeof(i4));         // ok, not odr-use
   extern void h5(int x = decltype(i4 + 4)()); // ok, not odr-use
 
   union {
     int i5;
   };
 
   extern void h6(int = i5);
-  // expected-error-re@-1 {{default argument references local variable '(unnamed variable of type (anonymous union at {{.*}}:20:3))' of enclosing function}}
+  // expected-error-re@-1 {{default argument references local variable '(unnamed variable of type (anonymous union at {{.*}}:35:3))' of enclosing function}}
+
+  struct S {
+    int i, j;
+  };
+  auto [x, y] = S();
 
-  struct S { int i; };
-  auto [x] = S();
+  extern void h7(int = x); // expected-error {{default argument references local variable '[x, y]'}}
 
-  extern void h7(int = x); // FIXME: reject
+  auto [z, w] = E();
+  extern void h8a(int = sizeof(z)); // ok
+  extern void h8b(int = w);         // expected-error {{default argument references local variable 'w'}}
+
+  extern auto get_array()->int(&)[2];
+  auto [a0, a1] = get_array();
+  extern void h9a(int = sizeof(a0));
+  extern void h9b(int = a1); // expected-error {{default argument references local variable '[a0, a1]'}}
 }