[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

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

[riccibruno]

clang/include/clang/AST/DeclCXX.h

3843

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

114

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.

147

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

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.

147

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

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/include/clang/AST/DeclCXX.h

 ///   int n[3];
 ///   auto &[a, b, c] = n;
 ///
 /// a, b, and c are BindingDecls, whose bindings are the expressions
 /// x[0], x[1], and x[2] respectively, where x is the implicit
 /// DecompositionDecl of type 'int (&)[3]'.
 class BindingDecl : public ValueDecl {
   /// The declaration that this binding binds to part of.
+  // FIXME: Currently not set during deserialization of the BindingDecl;
+  // only set when the corresponding DecompositionDecl is visited.
   LazyDeclPtr Decomp;
   /// The binding represented by this declaration. References to this
   /// declaration are effectively equivalent to this expression (except
   /// that it is only evaluated once at the point of declaration of the
   /// binding).
   Expr *Binding = nullptr;
 
   BindingDecl(DeclContext *DC, SourceLocation IdLoc, IdentifierInfo *Id)

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_param_default_argument_redefinition : ExtWarn<
   InGroup<MicrosoftDefaultArgRedefinition>;
 def err_param_default_argument_missing : Error<
   "missing default argument on parameter">;
 def err_param_default_argument_missing_name : Error<
   "missing default argument on parameter %0">;
 def err_param_default_argument_references_param : Error<
   "default argument references parameter %0">;
 def err_param_default_argument_references_local : Error<
-  "default argument references local variable %0 of enclosing function">;
+  "default argument references local %select{variable|binding}0 %1 "
+  "of enclosing function">;
 def err_param_default_argument_references_this : Error<
   "default argument references 'this'">;
 def err_param_default_argument_nonfunc : Error<
   "default arguments can only be specified for parameters in a function "
   "declaration">;
 def err_param_default_argument_template_redecl : Error<
   "default arguments cannot be added to a function template that has already "
   "been declared">;

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();
+
+  auto CheckAndDiagnoseLocalEntity = [&](const VarDecl *VD, unsigned DiagID,
+                                         const auto &... DiagArgs) -> bool {
+    if (VD->isLocalVarDecl() && !DRE->isNonOdrUse()) {
+      auto DB = S.Diag(DRE->getBeginLoc(), DiagID);
+      // FIXME: Replace with a fold once we can use C++17.
+      int dummy[] = {(DB << DiagArgs, 0)...};
+      (void)dummy;
+
+      DB << DefaultArg->getSourceRange();
+      return true;
+    }
+
+    return false;
+  };
+
   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.
     //
     // C++17 [dcl.fct.default]p7 (by CWG 2082):
     //   A local variable shall not appear as a potentially-evaluated
     //   expression in a default argument.
     //
     // C++20 [dcl.fct.default]p7 (DR as part of P0588R1, see also CWG 2346):
     //   Note: A local variable cannot be odr-used (6.3) in a default argument.
     //
-    if (VDecl->isLocalVarDecl() && !DRE->isNonOdrUse())
-      return S.Diag(DRE->getBeginLoc(),
-                    diag::err_param_default_argument_references_local)
-             << VDecl << DefaultArg->getSourceRange();
+    if (CheckAndDiagnoseLocalEntity(
+            VDecl, diag::err_param_default_argument_references_local,
+            /*variable*/ 0, VDecl))
+      return true;
+
+  } else if (const auto *Binding = dyn_cast<BindingDecl>(Decl)) {
+    // 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).
+    //
+    const auto *VD = cast_or_null<VarDecl>(Binding->getDecomposedDecl());
+    if (VD && CheckAndDiagnoseLocalEntity(
+                  VD, diag::err_param_default_argument_references_local,
+                  /*binding*/ 1, Binding))
+      return true;
   }
 
   return false;
 }
 
 /// VisitCXXThisExpr - Visit a C++ "this" expression.
 bool CheckDefaultArgumentVisitor::VisitCXXThisExpr(const CXXThisExpr *ThisE) {
   // C++ [dcl.fct.default]p8:

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 binding 'x'}}
 
-  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 binding '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 binding 'a1'}}
 }