Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][Sema] Extract ellipsis location from CXXFoldExpr for reattaching constraints on NTTPs #78080

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jan 14, 2024

We build up a CXXFoldExpr for immediately declared constraints as per C++20 [temp.param]/4. This is done by
formImmediatelyDeclaredConstraint where an EllipsisLoc is essential to determine whether this is a pack.

On the other hand, when attempting to instantiate a class template, member templates might not be instantiated immediately, so we leave them intact. For function templates with NTTPs, we reattach constraints if possible so that they can be evaluated later. To properly form that, we attempted to extract an ellipsis location if the param per se was a parameter pack. Unfortunately, for the following NTTP case, we seemingly failed to handle:

template <Constraint auto... Pack>
void member();

The NTTPD Pack is neither an ExpandedParameterPack nor a PackExpansion (its type does not expand anything). As a result, we end up losing track of the constraints on packs, although we have them inside the associated CXXFoldExpr.

This patch fixes that by extracting the ellipsis location out of the previous constraint expression. Closes #63837.

…ing constraints on NTTPs

We build up a CXXFoldExpr for immediately declared constraints as
per C++20 [temp.param]/4. This is done by
`formImmediatelyDeclaredConstraint` where an EllipsisLoc is essential
to determine whether this is a pack.

On the other hand, when attempting to instantiate a class template,
member templates might not be instantiated immediately, so we leave
them intact. For function templates with NTTPs,  we reattach
constraints if possible so that they can be evaluated later. To
properly form that, we attempted to extract an ellipsis location if
the param per se was a parameter pack. Unfortunately, for the following
NTTP case, we seemingly failed to handle:

```cpp
template <Constraint auto... Pack>
void member();
```

The NTTPD Pack is neither an ExpandedParameterPack nor a PackExpansion
(its type does not expand anything). As a result, we end up losing
track of the constraints on packs, although we have them inside the
associated CXXFoldExpr.

This patch fixes that by extracting the ellipsis location out of the
previous constraint expression. This closes
llvm#63837.
@zyn0217 zyn0217 added clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts labels Jan 14, 2024
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 14, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 14, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

We build up a CXXFoldExpr for immediately declared constraints as per C++20 [temp.param]/4. This is done by
formImmediatelyDeclaredConstraint where an EllipsisLoc is essential to determine whether this is a pack.

On the other hand, when attempting to instantiate a class template, member templates might not be instantiated immediately, so we leave them intact. For function templates with NTTPs, we reattach constraints if possible so that they can be evaluated later. To properly form that, we attempted to extract an ellipsis location if the param per se was a parameter pack. Unfortunately, for the following NTTP case, we seemingly failed to handle:

template &lt;Constraint auto... Pack&gt;
void member();

The NTTPD Pack is neither an ExpandedParameterPack nor a PackExpansion (its type does not expand anything). As a result, we end up losing track of the constraints on packs, although we have them inside the associated CXXFoldExpr.

This patch fixes that by extracting the ellipsis location out of the previous constraint expression. Closes #63837.


Full diff: https://github.com/llvm/llvm-project/pull/78080.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+13-8)
  • (modified) clang/test/SemaTemplate/concepts.cpp (+21)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 154412144ef97a..04b2fac2c74713 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -857,6 +857,9 @@ Bug Fixes to C++ Support
   (`#64607 <https://github.com/llvm/llvm-project/issues/64607>`_)
   (`#64086 <https://github.com/llvm/llvm-project/issues/64086>`_)
 
+- Fixed a crash where we lost uninstantiated constraints on placeholder NTTP packs. Fixes:
+  (`#63837 <https://github.com/llvm/llvm-project/issues/63837>`_)
+
 - Fixed a regression where clang forgets how to substitute into constraints on template-template
   parameters. Fixes: 
   (`#57410 <https://github.com/llvm/llvm-project/issues/57410>`_) and
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index d768bb72e07c09..208a7b120c419b 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3056,16 +3056,21 @@ Decl *TemplateDeclInstantiator::VisitNonTypeTemplateParmDecl(
         D->getPosition(), D->getIdentifier(), T, D->isParameterPack(), DI);
 
   if (AutoTypeLoc AutoLoc = DI->getTypeLoc().getContainedAutoTypeLoc())
-    if (AutoLoc.isConstrained())
+    if (AutoLoc.isConstrained()) {
+      SourceLocation EllipsisLoc;
+      if (IsExpandedParameterPack)
+        EllipsisLoc =
+            DI->getTypeLoc().getAs<PackExpansionTypeLoc>().getEllipsisLoc();
+      else if (auto *Constraint = dyn_cast_if_present<CXXFoldExpr>(
+                   D->getPlaceholderTypeConstraint()))
+        EllipsisLoc = Constraint->getEllipsisLoc();
       // Note: We attach the uninstantiated constriant here, so that it can be
-      // instantiated relative to the top level, like all our other constraints.
-      if (SemaRef.AttachTypeConstraint(
-              AutoLoc, Param, D,
-              IsExpandedParameterPack
-                ? DI->getTypeLoc().getAs<PackExpansionTypeLoc>()
-                    .getEllipsisLoc()
-                : SourceLocation()))
+      // instantiated relative to the top level, like all our other
+      // constraints.
+      if (SemaRef.AttachTypeConstraint(AutoLoc, /*NewConstrainedParm=*/Param,
+                                       /*OrigConstrainedParm=*/D, EllipsisLoc))
         Invalid = true;
+    }
 
   Param->setAccess(AS_public);
   Param->setImplicit(D->isImplicit());
diff --git a/clang/test/SemaTemplate/concepts.cpp b/clang/test/SemaTemplate/concepts.cpp
index e98ebcc9203a43..bac209a28da912 100644
--- a/clang/test/SemaTemplate/concepts.cpp
+++ b/clang/test/SemaTemplate/concepts.cpp
@@ -1064,3 +1064,24 @@ void cand(T t)
 
 void test() { cand(42); }
 }
+
+namespace GH63837 {
+
+template<class> concept IsFoo = true;
+
+template<class> struct Struct {
+  template<IsFoo auto... xs>
+  void foo() {}
+
+  template<auto... xs> requires (... && IsFoo<decltype(xs)>)
+  void bar() {}
+
+  template<IsFoo auto... xs>
+  static inline int field = 0;
+};
+
+template void Struct<void>::foo<>();
+template void Struct<void>::bar<>();
+template int Struct<void>::field<1, 2>;
+
+}

@zyn0217 zyn0217 merged commit 29b5f8f into llvm:main Jan 17, 2024
5 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…ing constraints on NTTPs (llvm#78080)

We build up a `CXXFoldExpr` for immediately declared constraints as per
C++20 [temp.param]/4. This is done by
`formImmediatelyDeclaredConstraint` where an `EllipsisLoc` is essential
to determine whether this is a pack.

On the other hand, when attempting to instantiate a class template,
member templates might not be instantiated immediately, so we leave them
intact. For function templates with NTTPs, we reattach constraints if
possible so that they can be evaluated later. To properly form that, we
attempted to extract an ellipsis location if the param per se was a
parameter pack. Unfortunately, for the following NTTP case, we seemingly
failed to handle:

```cpp
template <Constraint auto... Pack>
void member();
```

The NTTPD Pack is neither an `ExpandedParameterPack` nor a
`PackExpansion` (its type does not expand anything). As a result, we end
up losing track of the constraints on packs, although we have them
inside the associated `CXXFoldExpr`.

This patch fixes that by extracting the ellipsis location out of the
previous constraint expression. Closes
llvm#63837.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category concepts C++20 concepts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concept-constrained non-type template parameter pack in a member template of a class template causes a crash
3 participants