[Clang][P1061] Improve visiting HoldingVars; Improve parsing diagnostics of binding packs

ricejasonf · ricejasonf · commit 116aff1e444b · 2025-01-02T14:13:49.000-08:00
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
@@ -4166,7 +4166,6 @@ class BindingDecl : public ValueDecl {
   void setDecomposedDecl(ValueDecl *Decomposed) { Decomp = Decomposed; }
 
   VarDecl *getHoldingVar() const;
-  static VarDecl *getHoldingVar(Expr *E);
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Decl::Binding; }
@@ -4224,7 +4223,7 @@ class DecompositionDecl final
   /// binding. Only present for user-defined bindings for tuple-like types.
   void VisitHoldingVars(llvm::function_ref<void(VarDecl *)> F) const;
 
-  // Visit the concrete bindings. (workaround)
+  // Visit the concrete bindings.
   void VisitBindings(llvm::function_ref<void(BindingDecl *)> F) const;
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
@@ -5321,6 +5321,8 @@ class BuiltinBitCastExpr final
   }
 };
 
+// Represents an unexpanded pack where the list of expressions are
+// known. These are used when structured bindings introduce a pack.
 class ResolvedUnexpandedPackExpr final
     : public Expr,
       private llvm::TrailingObjects<ResolvedUnexpandedPackExpr, Stmt *> {
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1101,9 +1101,11 @@ def err_lambda_capture_multiple_ellipses : Error<
   "multiple ellipses in pack capture">;
 def err_binding_multiple_ellipses : Error<
   "multiple ellipses in structured binding declaration">;
-def warn_cxx2c_binding_pack : Warning<
+def note_previous_ellipsis : Note<
+  "previous ellipsis specified here">;
+def ext_cxx_binding_pack : ExtWarn<
   "structured binding pack is incompatible with C++ standards before C++2c">,
-  DefaultIgnore, InGroup<CXXPre26Compat>;
+  InGroup<CXX26>;
 def err_capture_default_first : Error<
   "capture default must be first">;
 def ext_decl_attrs_on_lambda : ExtWarn<
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
@@ -237,12 +237,10 @@ bool Decl::isTemplateParameterPack() const {
 }
 
 bool Decl::isParameterPack() const {
-  if (isTemplateParameterPack())
-    return true;
   if (const auto *Var = dyn_cast<ValueDecl>(this))
     return Var->isParameterPack();
 
-  return false;
+  return isTemplateParameterPack();
 }
 
 FunctionDecl *Decl::getAsFunction() {
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
@@ -3395,7 +3395,7 @@ VarDecl *ValueDecl::getPotentiallyDecomposedVarDecl() {
   if (auto *Var = llvm::dyn_cast<VarDecl>(this))
     return Var;
   if (auto *BD = llvm::dyn_cast<BindingDecl>(this))
-    return llvm::dyn_cast_or_null<VarDecl>(BD->getDecomposedDecl());
+    return llvm::dyn_cast_if_present<VarDecl>(BD->getDecomposedDecl());
   return nullptr;
 }
 
@@ -3416,16 +3416,7 @@ VarDecl *BindingDecl::getHoldingVar() const {
   Expr *B = getBinding();
   if (!B)
     return nullptr;
-  return getHoldingVar(B);
-}
-
-VarDecl *BindingDecl::getHoldingVar(Expr *E) {
-  auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreImplicit());
-  if (!DRE)
-    return nullptr;
-  if (auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) {
-    DRE = dyn_cast<DeclRefExpr>(BD->getBinding());
-  }
+  auto *DRE = dyn_cast<DeclRefExpr>(B->IgnoreImplicit());
   if (!DRE)
     return nullptr;
 
@@ -3434,27 +3425,13 @@ VarDecl *BindingDecl::getHoldingVar(Expr *E) {
   return VD;
 }
 
+
 void DecompositionDecl::VisitHoldingVars(
     llvm::function_ref<void(VarDecl *)> F) const {
-  for (BindingDecl *B : bindings()) {
-    Expr *BE = B->getBinding();
-    // All BindingDecls will contain holding vars or none will
-    if (!BE)
-      return;
-
-    llvm::ArrayRef<Expr *> Exprs;
-    if (auto *RP = dyn_cast<ResolvedUnexpandedPackExpr>(BE))
-      Exprs = llvm::ArrayRef(RP->getExprs(), RP->getNumExprs());
-    else
-      Exprs = BE;
-
-    for (Expr *E : Exprs) {
-      VarDecl *VD = BindingDecl::getHoldingVar(E);
-      if (!VD)
-        return;
+  VisitBindings([&](BindingDecl* BD) {
+    if (VarDecl* VD = BD->getHoldingVar())
       F(VD);
-    }
-  }
+  });
 }
 
 void DecompositionDecl::VisitBindings(
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
@@ -3653,12 +3653,8 @@ bool Expr::HasSideEffects(const ASTContext &Ctx,
   case PackIndexingExprClass:
   case HLSLOutArgExprClass:
   case OpenACCAsteriskSizeExprClass:
-    // These never have a side-effect.
-    return false;
-
-  // ResolvedUnexpandedPackExpr is currently only used for
-  // structed bindings which have no side effects
   case ResolvedUnexpandedPackExprClass:
+    // These never have a side-effect.
     return false;
 
   case ConstantExprClass:
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
@@ -1971,6 +1971,9 @@ ResolvedUnexpandedPackExpr::ResolvedUnexpandedPackExpr(SourceLocation BL,
                                                        unsigned NumExprs)
     : Expr(ResolvedUnexpandedPackExprClass, QT, VK_PRValue, OK_Ordinary),
       BeginLoc(BL), NumExprs(NumExprs) {
+  // C++ [temp.dep.expr]p3
+  // An id-expression is type-dependent if it is
+  //    - associated by name lookup with a pack
   setDependence(ExprDependence::TypeValueInstantiation |
                 ExprDependence::UnexpandedPack);
 }
@@ -2007,9 +2010,7 @@ ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL,
 }
 
 ResolvedUnexpandedPackExpr *ResolvedUnexpandedPackExpr::getFromDecl(Decl *D) {
-  // TODO P1858: Extend to VarDecls for P1858
-  if (auto *BD = dyn_cast<BindingDecl>(D)) {
-    return dyn_cast_or_null<ResolvedUnexpandedPackExpr>(BD->getBinding());
-  }
+  if (auto *BD = dyn_cast<BindingDecl>(D))
+    return dyn_cast_if_present<ResolvedUnexpandedPackExpr>(BD->getBinding());
   return nullptr;
 }
diff --git a/clang/lib/AST/ExprClassification.cpp b/clang/lib/AST/ExprClassification.cpp
@@ -455,7 +455,7 @@ static Cl::Kinds ClassifyInternal(ASTContext &Ctx, const Expr *E) {
     if (cast<ResolvedUnexpandedPackExpr>(E)->getNumExprs() > 0)
       return ClassifyInternal(
           Ctx, cast<ResolvedUnexpandedPackExpr>(E)->getExpansion(0));
-    return Cl::CL_PRValue;
+    return Cl::CL_LValue;
   }
 
   case Expr::MaterializeTemporaryExprClass:
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
@@ -7310,14 +7310,14 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
   // array declarator.
   if (!(Tok.isOneOf(tok::identifier, tok::ellipsis) &&
         NextToken().isOneOf(tok::comma, tok::r_square, tok::kw_alignas,
-                            tok::identifier, tok::l_square)) &&
+                            tok::identifier, tok::l_square, tok::ellipsis)) &&
       !(Tok.is(tok::r_square) &&
         NextToken().isOneOf(tok::equal, tok::l_brace))) {
     PA.Revert();
     return ParseMisplacedBracketDeclarator(D);
   }
 
-  bool HasEllipsis = false;
+  SourceLocation PrevEllipsisLoc;
   SmallVector<DecompositionDeclarator::Binding, 32> Bindings;
   while (Tok.isNot(tok::r_square)) {
     if (!Bindings.empty()) {
@@ -7332,9 +7332,8 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
           Diag(Tok, diag::err_expected_comma_or_rsquare);
         }
 
-        // I don't know why this skipping was here
-        // SkipUntil(tok::r_square, tok::comma, tok::identifier,
-        //          StopAtSemi | StopBeforeMatch);
+        SkipUntil({tok::r_square, tok::comma, tok::identifier, tok::ellipsis},
+                  StopAtSemi | StopBeforeMatch);
         if (Tok.is(tok::comma))
           ConsumeToken();
         else if (Tok.is(tok::r_square))
@@ -7345,17 +7344,18 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
     if (isCXX11AttributeSpecifier())
       DiagnoseAndSkipCXX11Attributes();
 
-    SourceLocation EllipsisLoc = {};
+    SourceLocation EllipsisLoc;
 
     if (Tok.is(tok::ellipsis)) {
       if (!getLangOpts().CPlusPlus26)
-        Diag(Tok, diag::warn_cxx2c_binding_pack);
-      if (HasEllipsis) {
+        Diag(Tok, diag::ext_cxx_binding_pack);
+      if (PrevEllipsisLoc.isValid()) {
         Diag(Tok, diag::err_binding_multiple_ellipses);
+        Diag(PrevEllipsisLoc, diag::note_previous_ellipsis);
         break;
       }
-      HasEllipsis = true;
       EllipsisLoc = Tok.getLocation();
+      PrevEllipsisLoc = EllipsisLoc;
       ConsumeToken();
     }
 
@@ -7368,6 +7368,13 @@ void Parser::ParseDecompositionDeclarator(Declarator &D) {
     SourceLocation Loc = Tok.getLocation();
     ConsumeToken();
 
+    if (Tok.is(tok::ellipsis) && !PrevEllipsisLoc.isValid()) {
+      DiagnoseMisplacedEllipsis(Tok.getLocation(), Loc,
+                                EllipsisLoc.isValid(), true);
+      EllipsisLoc = Tok.getLocation();
+      ConsumeToken();
+    }
+
     ParsedAttributes Attrs(AttrFactory);
     if (isCXX11AttributeSpecifier()) {
       Diag(Tok, getLangOpts().CPlusPlus26
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
@@ -9780,7 +9780,6 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   QualType R = TInfo->getType();
 
   assert(R->isFunctionType());
-  bool NeedsExpansion = false;
 
   if (R.getCanonicalType()->castAs<FunctionType>()->getCmseNSCallAttr())
     Diag(D.getIdentifierLoc(), diag::err_function_decl_cmse_ns_call);
@@ -10966,14 +10965,6 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
       D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration)
     ExternalDeclarations.push_back(NewFD);
 
-  if (NeedsExpansion) {
-    CodeSynthesisContext SynthCtx{};
-    pushCodeSynthesisContext(SynthCtx);
-    NewFD = dyn_cast_or_null<FunctionDecl>(
-        SubstDecl(NewFD, DC, MultiLevelTemplateArgumentList{}));
-    popCodeSynthesisContext();
-  }
-
   return NewFD;
 }
 
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2491,7 +2491,7 @@ TemplateInstantiator::TransformDeclRefExpr(DeclRefExpr *E) {
   if (BindingDecl *BD = dyn_cast<BindingDecl>(D); BD && BD->isParameterPack()) {
     BD = cast<BindingDecl>(TransformDecl(BD->getLocation(), BD));
     if (auto *RP =
-            dyn_cast_or_null<ResolvedUnexpandedPackExpr>(BD->getBinding())) {
+            dyn_cast_if_present<ResolvedUnexpandedPackExpr>(BD->getBinding())) {
       return TransformResolvedUnexpandedPackExpr(RP);
     }
   }
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1180,13 +1180,13 @@ Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
   ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
   for (auto *OldBD : D->bindings()) {
     Expr *BindingExpr = OldBD->getBinding();
-    if (auto *RP = dyn_cast_or_null<ResolvedUnexpandedPackExpr>(BindingExpr))
+    if (auto *RP = dyn_cast_if_present<ResolvedUnexpandedPackExpr>(BindingExpr))
       OldResolvedPack = RP;
     NewBindings.push_back(cast<BindingDecl>(VisitBindingDecl(OldBD)));
   }
   ArrayRef<BindingDecl*> NewBindingArray = NewBindings;
 
-  auto *NewDD = cast_or_null<DecompositionDecl>(
+  auto *NewDD = cast_if_present<DecompositionDecl>(
       VisitVarDecl(D, /*InstantiatingVarTemplate=*/false, &NewBindingArray));
 
   if (!NewDD || NewDD->isInvalidDecl())
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -56,7 +56,7 @@ class CollectUnexpandedParameterPacksVisitor
         return;
       } else if (auto *BD = dyn_cast<BindingDecl>(ND)) {
         Expr *E = BD->getBinding();
-        if (auto *RP = dyn_cast_or_null<ResolvedUnexpandedPackExpr>(E)) {
+        if (auto *RP = dyn_cast_if_present<ResolvedUnexpandedPackExpr>(E)) {
           addUnexpanded(RP);
           return;
         }
diff --git a/clang/test/Parser/cxx2c-binding-pack.cpp b/clang/test/Parser/cxx2c-binding-pack.cpp
@@ -3,5 +3,9 @@
 template <unsigned N>
 void decompose_array() {
   int arr[4] = {1, 2, 3, 5};
-  auto [x, ...rest, ...more_rest] = arr; // expected-error{{multiple ellipses in structured binding declaration}}
+  auto [x, ... // #1
+    rest, ...more_rest] = arr; // expected-error{{multiple ellipses in structured binding declaration}}
+                               // expected-note@#1{{previous ellipsis specified here}}
+                               //
+  auto [y...] = arr; // expected-error{{'...' must immediately precede declared identifier}}
 }
diff --git a/clang/test/SemaCXX/cxx2c-binding-pack-nontemplate.cpp b/clang/test/SemaCXX/cxx2c-binding-pack-nontemplate.cpp
@@ -1,12 +1,8 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++2c %s -verify
+// RUN: %clang_cc1 -fsyntax-only -std=c++26 %s -verify=nontemplate
+// RUN: %clang_cc1 -fsyntax-only %s -verify=nontemplate,compat
 
 void decompose_array() {
   int arr[4] = {1, 2, 3, 6};
-  auto [x, ...rest, y] = arr; // expected-error{{pack declaration outside of template}}
-}
-
-template <unsigned N>
-void decompose_array_2() {
-  int arr[4] = {1, 2, 3, N};
-  auto [x, ...rest, ...y] = arr; // expected-error{{multiple ellipses in structured binding declaration}}
+  auto [x, ...rest, y] = arr; // nontemplate-error{{pack declaration outside of template}} \
+  // compat-warning{{structured binding pack is incompatible with C++ standards before C++2c}}
 }
diff --git a/clang/test/SemaCXX/cxx2c-binding-pack.cpp b/clang/test/SemaCXX/cxx2c-binding-pack.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++2b %s -verify
+// RUN: %clang_cc1 -fsyntax-only -std=c++26 %s -verify
 // expected-no-diagnostics
 
 template <typename T>
@@ -64,8 +64,6 @@ void decompose_struct() {
 
 template <typename T>
 void decompose_array() {
-  // previously unable to use non-dependent array here
-  // Fixes https://bugs.llvm.org/show_bug.cgi?id=45964
   int arr[4] = {1, 2, 3, 6};
   auto [x, ...rest, y] = arr;
 

Original file line number	Diff line number	Diff line change
`@@ -5321,6 +5321,8 @@ class BuiltinBitCastExpr final`
`5321`	`5321`	`}`
`5322`	`5322`	`};`
`5323`	`5323`
	`5324`	`+// Represents an unexpanded pack where the list of expressions are`
	`5325`	`+// known. These are used when structured bindings introduce a pack.`
`5324`	`5326`	`class ResolvedUnexpandedPackExpr final`
`5325`	`5327`	`: public Expr,`
`5326`	`5328`	`private llvm::TrailingObjects<ResolvedUnexpandedPackExpr, Stmt *> {`
Original file line number	Diff line number	Diff line change
`@@ -237,12 +237,10 @@ bool Decl::isTemplateParameterPack() const {`
`237`	`237`	`}`
`238`	`238`
`239`	`239`	`bool Decl::isParameterPack() const {`
`240`		`- if (isTemplateParameterPack())`
`241`		`- return true;`
`242`	`240`	`if (const auto *Var = dyn_cast<ValueDecl>(this))`
`243`	`241`	`return Var->isParameterPack();`
`244`	`242`
`245`		`- return false;`
	`243`	`+ return isTemplateParameterPack();`
`246`	`244`	`}`
`247`	`245`
`248`	`246`	`FunctionDecl *Decl::getAsFunction() {`
Original file line number	Diff line number	Diff line change
`@@ -455,7 +455,7 @@ static Cl::Kinds ClassifyInternal(ASTContext &Ctx, const Expr *E) {`
`455`	`455`	`if (cast<ResolvedUnexpandedPackExpr>(E)->getNumExprs() > 0)`
`456`	`456`	`return ClassifyInternal(`
`457`	`457`	`Ctx, cast<ResolvedUnexpandedPackExpr>(E)->getExpansion(0));`
`458`		`- return Cl::CL_PRValue;`
	`458`	`+ return Cl::CL_LValue;`
`459`	`459`	`}`
`460`	`460`
`461`	`461`	`case Expr::MaterializeTemporaryExprClass:`