[Clang] Correctly construct template arguments for template template parameters #76811

zyn0217 · 2024-01-03T12:19:23Z

This fixes the bug introduced by
6db007a.

We construct placeholder template arguments for template-template parameters to avoid mismatching argument substitution since they have different depths with their corresponding template arguments. In this case,

template <template <Concept C> class T> void foo(T<int>);

T lies at the depth 0, and C lies at 1. The corresponding argument, of which there is exactly one, int, is at depth 0. If we consider the argument as the outermost one, then we would end up substituting 'int' into the wrong parameter T.

We used to perform such placeholder construction during the context walk-up. In the previous patch, we slipped through that inadvertently because we would walk up to the parent, which is precisely a FileContext for template-template parameters, after adding innermost arguments.

Besides, this patch moves the sanity check up to the context switch. That way, we avoid dereferencing null pointers if ND is unspecified.

Closes #57410.
Closes #76604. (The case is slightly different than that in #57410. We should not assume the surrounding context to be a file-scope one.)

llvmbot · 2024-01-03T12:19:57Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

This fixes the bug introduced by
6db007a.

We construct placeholder template arguments for template-template parameters to avoid mismatching argument substitution since they have different depths with their corresponding template arguments. In this case,

template &lt;template &lt;Concept C&gt; class T&gt; void foo(T&lt;int&gt;);

T lies at the depth 0, and C lies at 1. The corresponding argument, of which there is exactly one, int, is at depth 0. If we consider the argument as the outermost one, then we would end up substituting 'int' into the wrong parameter T.

We used to perform such placeholder construction during the context walk-up. In the previous patch, we slipped through that inadvertently because we would walk up to the parent, which is precisely a FileContext for template-template parameters, after adding innermost arguments.

Besides, this patch moves the sanity check up to the context switch. That way, we avoid dereferencing null pointers if ND is unspecified.

Closes #57410.

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+8-4)
(modified) clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp (+25)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0c8fec691bf3c9..7193d711333780 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -843,6 +843,9 @@ Bug Fixes to C++ Support
 - Fix crash when parsing nested requirement. Fixes:
   (`#73112 <https://github.com/llvm/llvm-project/issues/73112>`_)
 
+- Fix a regression where clang forgets how to substitute into constraints on template-template
+  parameters. Fixes: (`#57410 <https://github.com/llvm/llvm-project/issues/57410>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index df6b40999e645c..4420280efebb86 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -345,15 +345,19 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
 
   using namespace TemplateInstArgsHelpers;
   const Decl *CurDecl = ND;
+
+  if (!ND)
+    CurDecl = Decl::castFromDeclContext(DC);
+
   if (Innermost) {
     Result.addOuterTemplateArguments(const_cast<NamedDecl *>(ND),
                                      Innermost->asArray(), Final);
-    CurDecl = Response::UseNextDecl(ND).NextDecl;
+    if (CurDecl->getDeclContext()->isFileContext())
+      if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(CurDecl))
+        HandleDefaultTempArgIntoTempTempParam(TTP, Result);
+    CurDecl = Response::UseNextDecl(CurDecl).NextDecl;
   }
 
-  if (!ND)
-    CurDecl = Decl::castFromDeclContext(DC);
-
   while (!CurDecl->isFileContextDecl()) {
     Response R;
     if (const auto *VarTemplSpec =
diff --git a/clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp b/clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
index 449b6232542e24..277935f6b3b2f0 100644
--- a/clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
+++ b/clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
@@ -59,3 +59,28 @@ struct Nothing {};
 
 // FIXME: Wait the standard to clarify the intent.
 template<> template<> Z<Nothing> S5<Z>::V<Nothing>;
+
+namespace GH57410 {
+
+template<typename T>
+concept True = true;
+
+template<typename T>
+concept False = false; // #False
+
+template<template<True T> typename Wrapper>
+using Test = Wrapper<int>;
+
+template<template<False T> typename Wrapper> // #TTP-Wrapper
+using Test = Wrapper<int>; // expected-error {{constraints not satisfied for template template parameter 'Wrapper' [with T = int]}}
+
+// expected-note@#TTP-Wrapper {{'int' does not satisfy 'False'}}
+// expected-note@#False {{evaluated to false}}
+
+template <template<False> typename T> // #TTP-foo
+void foo(T<int>); // expected-error {{constraints not satisfied for template template parameter 'T' [with $0 = int]}}
+
+// expected-note@#TTP-foo {{'int' does not satisfy 'False'}}
+// expected-note@#False {{evaluated to false}}
+
+}

erichkeane

This makes sense to me, I think this is right.

shafik · 2024-01-03T23:20:22Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

@@ -345,15 +345,19 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(

  using namespace TemplateInstArgsHelpers;
  const Decl *CurDecl = ND;
+
+  if (!ND)


Seems less error prone to check !CurDecl here. If the surrounding code get modified or refactored the relationship between ND and CurDecl may break down.

Can we also get a comment here why this makes sense to call Decl::castFromDeclContext(DC) if CurrDecl is not present?

Seems less error prone to check !CurDecl here.

done.

Can we also get a comment here why this makes sense to call Decl::castFromDeclContext(DC) if CurrDecl is not present?

I think this has been explained in the comment above the function.

/// \param DC In the event we don't HAVE a declaration yet, we instead provide /// the decl context where it will be created. In this case, the `Innermost` /// should likely be provided. If ND is non-null, this is ignored.

clang/lib/Sema/SemaTemplateInstantiate.cpp

zyn0217 · 2024-01-04T06:04:17Z

I realized this still crashes the code with NTTP. I will look into this.

template <class T>
concept C = false;

template <typename T, unsigned N> struct S {};

template <unsigned N, template <C, unsigned> typename T>
auto wow(T<int, N> ts) {
  return ts.x;
}

int main() { return wow<3>(S<int, 3>{}); }

zyn0217 · 2024-01-04T12:44:23Z

It is interesting to note that, if we exchange the position of two template parameters, i.e.

template <class T>
concept C = false;

template <typename T, unsigned N> struct S {};

template <template <C, unsigned> typename T, unsigned N> // The template-template parameter now precedes the NTTP
int wow(T<int, N> ts);

int main() { return wow<S, 3>(S<int, 3>{}); }

then the crash disappears.

zyn0217 · 2024-01-05T07:55:02Z

Just get back to this again. So, I have explored the other case and found it particular to explicit template arguments. In fact, we could slightly revise the motivating case to

template <class T>
concept C = false;

template <typename U, template <C> typename T>
int wow(T<U> ts);

template <typename T> struct S {};

int main() { return wow<int>(S<int>{}); }

with an explicit template argument int. This takes us to such the branch in DeduceTemplateArguments,

llvm-project/clang/lib/Sema/SemaTemplateDeduction.cpp

Lines 4222 to 4232 in e0c554a

    
           if (ExplicitTemplateArgs) { 
        
             TemplateDeductionResult Result; 
        
             runWithSufficientStackSpace(Info.getLocation(), [&] { 
        
               Result = SubstituteExplicitTemplateArguments( 
        
                   FunctionTemplate, *ExplicitTemplateArgs, Deduced, ParamTypes, nullptr, 
        
                   Info); 
        
             }); 
        
             if (Result) 
        
               return Result; 
        
             NumExplicitlySpecified = Deduced.size();

which would deduce the remaining template arguments from the given FunctionTemplate and fall into our getTemplateInstantiationArgs at the end of the day. It is also interesting to notice that the given TemplateTemplateParmDecl originates from this FunctionTemplate, and the surrounding DeclContexts for these Decls has already been redirected to its associated FunctionDecl at AdoptTemplateParameterList.

llvm-project/clang/lib/AST/DeclTemplate.cpp

Lines 202 to 206 in 59af659

    
           static bool AdoptTemplateParameterList(TemplateParameterList *Params, 
        
                                                  DeclContext *Owner) { 
        
             bool Invalid = false; 
        
             for (NamedDecl *P : *Params) { 
        
               P->setDeclContext(Owner);

Therefore, I think our assumption that the TemplateTemplateParmDecl always resides in the TranslationUnitDecl is probably wrong. And, it is indeed murky that the DeclContexts where these Decls live depend on the callers to this function...

Removing these checks works for the code above and, fortunately, doesn't fail any tests.

@erichkeane WDYT?

erichkeane · 2024-01-05T14:53:07Z

So these sort of crashes are simply that the generated Multi-level Template Argument List doesn't match what is in the AST. This function (getInstantiationArgs) tries to 'recreate' them, so if it is 'wrong' than we get a crash. I suspect we're just missing some sort of awkward case in this situation. I'd suggest looking to see if you can figure out where the correct arguments are.

One thing that Concepts did was make it so that this function is used in many more cases, and at many more times. I wouldn't be surprised if it is now being used in situations where the decl-context hasn't been appropriately updated (that is, is either not set at all, or is in the uninstantiated version). In those cases, we have to special case them/figure out what they are supposed to be and use those isntead.

zyn0217 · 2024-01-05T16:32:57Z

Thank you for the feedback.

I wouldn't be surprised if it is now being used in situations where the decl-context hasn't been appropriately updated

I think this is not the case where we’re dealing with ill-formed Decl-contexts; it is that the caller site doesn't respect the assumption that “the surrounding Decl for a TTPD shall be a file-scope decl”.

I'd suggest looking to see if you can figure out where the correct arguments are.

I think I’ve already done that? (I have updated the patch to address it and that looks fine then), I’m just unsure if the removal for such assumption is appropriate. (or maybe i’m misreading your points? Should I change the call sites instead?)

erichkeane · 2024-01-05T16:48:19Z

Thank you for the feedback.

I wouldn't be surprised if it is now being used in situations where the decl-context hasn't been appropriately updated

I think this is not the case where we’re dealing with ill-formed Decl-contexts; it is that the caller site doesn't respect the assumption that “the surrounding Decl for a TTPD shall be a file-scope decl”.

I'd suggest looking to see if you can figure out where the correct arguments are.

I think I’ve already done that? (I have updated the patch to address it and that looks fine then), I’m just unsure if the removal for such assumption is appropriate. (or maybe i’m misreading your points? Should I change the call sites instead?)

That TTPD eventually DOES get the correct decl context, right? Or have you found a case where the decl context is NEVER set right? In some cases, we start forming a declaration without properly setting its decl context, then come along afterwards and 'fix' it up, so if this is THAT case, we can either change the caller to set it 'earlier' (if possible?), or make this function tolerant of it.

If it is NEVER set, then yes, we SHOULD be making sure the declaration context is set correctly. That said, even in a template-template parameter, we shouldn't really NEED the decl context, since it should be the decl context of hte declaration it is associated with.

zyn0217 · 2024-01-05T18:03:21Z

That TTPD eventually DOES get the correct decl context, right? Or have you found a case where the decl context is NEVER set right?

I believe so, yes
Not yet, but I would keep an eye on issues.

So I presume the removal for declcontexts is correct (at least) at the moment, and I’d like to leave the “fix-up declcontexts from callers” work in the future patch if I could find out some cases.

Anyway, thank you again for the review and explanation!

…e template parameters This fixes the bug introduced by llvm@6db007a. We construct placeholder template arguments for template-template parameters to avoid mismatching argument substitution since they have different depths with their corresponding template arguments. In this case, ```cpp template <template <Concept C> class T> void foo(T<int>); ``` T lies at the depth 0, and C lies at 1. The corresponding argument, of which there is exactly one, int, is at depth 0. If we consider the argument as the outermost one, then we would end up substituting 'int' into the wrong parameter T. We used to perform such placeholder construction during the context walk-up. In the previous patch, we slipped through that inadvertently because we would walk up to the parent, which is precisely a FileContext for template-template parameters, after adding innermost arguments. Besides, this patch moves the sanity check up to the context switch. That way, we avoid dereferencing null pointers if ND is unspecified. Closes llvm#57410.

github-actions · 2024-01-06T13:06:05Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…parameters (llvm#76811) This fixes the bug introduced by llvm@6db007a. We construct placeholder template arguments for template-template parameters to avoid mismatching argument substitution since they have different depths with their corresponding template arguments. In this case, ```cpp template <template <Concept C> class T> void foo(T<int>); ``` T lies at the depth 0, and C lies at 1. The corresponding argument, of which there is exactly one, int, is at depth 0. If we consider the argument as the outermost one, then we would end up substituting 'int' into the wrong parameter T. We used to perform such placeholder construction during the context walk-up. In the previous patch, we slipped through that inadvertently because we would walk up to the parent, which is precisely a FileContext for template-template parameters, after adding innermost arguments. Besides, this patch moves the sanity check up to the context switch. That way, we avoid dereferencing null pointers if ND is unspecified. Closes llvm#57410. Closes llvm#76604. (The case is slightly different than that in llvm#57410. We should *not* assume the surrounding context to be a file-scope one.)

zyn0217 requested review from cor3ntin, shafik, alexander-shaposhnikov and erichkeane January 3, 2024 12:19

zyn0217 added the clang Clang issues not falling into any other category label Jan 3, 2024

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jan 3, 2024

erichkeane approved these changes Jan 3, 2024

View reviewed changes

shafik reviewed Jan 3, 2024

View reviewed changes

zyn0217 changed the title ~~[Clang] Correctly construct template arguments for file-scope template template parameters~~ [Clang] Correctly construct template arguments for template template parameters Jan 5, 2024

zyn0217 force-pushed the pr57410 branch from 617d427 to 8aa0e22 Compare January 5, 2024 08:58

zyn0217 added 3 commits January 6, 2024 21:02

Address comments and poke the CI

5aa6a92

fixup

323a3ec

zyn0217 force-pushed the pr57410 branch from 8aa0e22 to 323a3ec Compare January 6, 2024 13:03

Format

43b7d9c

zyn0217 merged commit 0c7d46a into llvm:main Jan 6, 2024
4 of 5 checks passed

zyn0217 mentioned this pull request Feb 11, 2024

ICE when using a concept in template parameter of a template template parameter #50596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Correctly construct template arguments for template template parameters #76811

[Clang] Correctly construct template arguments for template template parameters #76811

zyn0217 commented Jan 3, 2024 •

edited

llvmbot commented Jan 3, 2024

erichkeane left a comment

shafik Jan 3, 2024

shafik Jan 3, 2024

zyn0217 Jan 5, 2024 •

edited

zyn0217 commented Jan 4, 2024

zyn0217 commented Jan 4, 2024

zyn0217 commented Jan 5, 2024

erichkeane commented Jan 5, 2024

zyn0217 commented Jan 5, 2024

erichkeane commented Jan 5, 2024

zyn0217 commented Jan 5, 2024 •

edited

github-actions bot commented Jan 6, 2024 •

edited

[Clang] Correctly construct template arguments for template template parameters #76811

[Clang] Correctly construct template arguments for template template parameters #76811

Conversation

zyn0217 commented Jan 3, 2024 • edited

llvmbot commented Jan 3, 2024

erichkeane left a comment

Choose a reason for hiding this comment

shafik Jan 3, 2024

Choose a reason for hiding this comment

shafik Jan 3, 2024

Choose a reason for hiding this comment

zyn0217 Jan 5, 2024 • edited

Choose a reason for hiding this comment

zyn0217 commented Jan 4, 2024

zyn0217 commented Jan 4, 2024

zyn0217 commented Jan 5, 2024

erichkeane commented Jan 5, 2024

zyn0217 commented Jan 5, 2024

erichkeane commented Jan 5, 2024

zyn0217 commented Jan 5, 2024 • edited

github-actions bot commented Jan 6, 2024 • edited

zyn0217 commented Jan 3, 2024 •

edited

zyn0217 Jan 5, 2024 •

edited

zyn0217 commented Jan 5, 2024 •

edited

github-actions bot commented Jan 6, 2024 •

edited