[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() #102922

zyn0217 · 2024-08-12T15:54:42Z

getTemplateInstantiationArgs() takes two parameters ND and DC to determine the starting point of a traversal. Previously, DC would be ignored in the presence of ND, and the next declaration context would almost always be the semantic context of DC if Innermost is specified.

Actually, we could harness DC to describe where we want to go after placing an Innermost template argument list, because DC was never used if ND is present, but anyhow we still have passed in DC with the semantic declaration context at the caller sides.

This is in preparation for CWG 2369.

llvmbot · 2024-08-13T04:36:20Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

getTemplateInstantiationArgs() takes two parameters ND and DC to determine the starting point of a traversal. Previously, DC would be ignored in the presence of ND, and the next declaration context would almost always be the semantic context of DC if Innermost is specified.

Actually, we could harness DC to describe where we want to go after placing an Innermost template argument list, because DC was never used if ND is present, but anyhow we still have passed in DC with the semantic declaration context at the caller sides.

This is in preparation for CWG 2369.

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

4 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+9-3)
(modified) clang/lib/Sema/SemaConcept.cpp (+1-1)
(modified) clang/lib/Sema/SemaTemplate.cpp (+6-5)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+7-8)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 25cb6c8fbf6104..c1912343cf43b8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13033,11 +13033,14 @@ class Sema final : public SemaBase {
   /// instantiation arguments.
   ///
   /// \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.
+  ///  the decl context where it will be created.  In this case, the \p
+  ///  Innermost should likely be provided.  If \p ND is non-null and \p
+  ///  Innermost is NULL, this is ignored.
   ///
   /// \param Innermost if non-NULL, specifies a template argument list for the
-  /// template declaration passed as ND.
+  /// template declaration passed as \p ND. The next declaration context would
+  /// be switched to \p DC if present; otherwise, it would be the semantic
+  /// declaration context of \p ND.
   ///
   /// \param RelativeToPrimary true if we should get the template
   /// arguments relative to the primary template, even when we're
@@ -13053,6 +13056,9 @@ class Sema final : public SemaBase {
   /// ForConstraintInstantiation indicates we should continue looking when
   /// encountering a lambda generic call operator, and continue looking for
   /// arguments on an enclosing class template.
+  ///
+  /// \param SkipForSpecialization when specified, any template specializations
+  /// in a traversal would be ignored.
   MultiLevelTemplateArgumentList getTemplateInstantiationArgs(
       const NamedDecl *D, const DeclContext *DC = nullptr, bool Final = false,
       std::optional<ArrayRef<TemplateArgument>> Innermost = std::nullopt,
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index de24bbe7eb99ce..f4dd0648781b61 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -1487,7 +1487,7 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N,
 static bool substituteParameterMappings(Sema &S, NormalizedConstraint &N,
                                         const ConceptSpecializationExpr *CSE) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      CSE->getNamedConcept(), CSE->getNamedConcept()->getLexicalDeclContext(),
+      CSE->getNamedConcept(), CSE->getNamedConcept()->getDeclContext(),
       /*Final=*/false, CSE->getTemplateArguments(),
       /*RelativeToPrimary=*/true,
       /*Pattern=*/nullptr,
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 29e7978ba5b1f8..d2b1bc0e463ade 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -5582,11 +5582,12 @@ bool Sema::CheckTemplateArgumentList(
     ContextRAII Context(*this, NewContext);
     CXXThisScopeRAII(*this, RD, ThisQuals, RD != nullptr);
 
-    MultiLevelTemplateArgumentList MLTAL = getTemplateInstantiationArgs(
-        Template, NewContext, /*Final=*/false, CanonicalConverted,
-        /*RelativeToPrimary=*/true,
-        /*Pattern=*/nullptr,
-        /*ForConceptInstantiation=*/true);
+    MultiLevelTemplateArgumentList MLTAL =
+        getTemplateInstantiationArgs(Template, Template->getDeclContext(),
+                                     /*Final=*/false, CanonicalConverted,
+                                     /*RelativeToPrimary=*/true,
+                                     /*Pattern=*/nullptr,
+                                     /*ForConceptInstantiation=*/true);
     if (EnsureTemplateArgumentListConstraints(
             Template, MLTAL,
             SourceRange(TemplateLoc, TemplateArgs.getRAngleLoc()))) {
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 9a6cd2cd0ab751..efae547b21c715 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -492,7 +492,8 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
     // has a depth of 0.
     if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(CurDecl))
       HandleDefaultTempArgIntoTempTempParam(TTP, Result);
-    CurDecl = Response::UseNextDecl(CurDecl).NextDecl;
+    CurDecl = DC ? Decl::castFromDeclContext(DC)
+                 : Response::UseNextDecl(CurDecl).NextDecl;
   }
 
   while (!CurDecl->isFileContextDecl()) {
@@ -3240,15 +3241,13 @@ bool Sema::SubstDefaultArgument(
           /*ForDefinition*/ false);
       if (addInstantiatedParametersToScope(FD, PatternFD, *LIS, TemplateArgs))
         return true;
-      const FunctionTemplateDecl *PrimaryTemplate = FD->getPrimaryTemplate();
-      if (PrimaryTemplate && PrimaryTemplate->isOutOfLine()) {
-        TemplateArgumentList *CurrentTemplateArgumentList =
-            TemplateArgumentList::CreateCopy(getASTContext(),
-                                             TemplateArgs.getInnermost());
+      // FIXME: Investigate if we shall validate every FunctionTemplateDecl
+      // along the getInstantiatedFromMemberTemplate() chain.
+      if (auto *PrimaryTemplate = FD->getPrimaryTemplate();
+          PrimaryTemplate && PrimaryTemplate->isOutOfLine())
         NewTemplateArgs = getTemplateInstantiationArgs(
             FD, FD->getDeclContext(), /*Final=*/false,
-            CurrentTemplateArgumentList->asArray(), /*RelativeToPrimary=*/true);
-      }
+            TemplateArgs.getInnermost(), /*RelativeToPrimary=*/true);
     }
 
     runWithSufficientStackSpace(Loc, [&] {

mizvekov

I think I'd need to see how you would use it, to say if this would be the best approach.

It's not clear to me why we would want to navigate to a DC that is unrelated to the passed ND.

zyn0217 · 2024-08-14T01:56:06Z

I think I'd need to see how you would use it, to say if this would be the best approach.

(I thought I have explained somewhere, but it seems not. Sorry!)

So the basic idea of CWG2369 is to swap the order of the function template substitution and its constraint checking. We previously relied on the substituted function to collect the entire template argument lists for the constraint evaluation to work, while this is no longer the case because we're now required to check the constraint before the function declaration substitution.

The situation is then slightly subtle since we don't have a complete function specialization to collect the complete arguments in one go. So it would end up with two pieces: one is the instantiating template arguments that we have (CanonicalBuilder / SugaredBuilder) for the current function template; the other part consisted of the template arguments of the parent DeclContexts. My idea is to splice these template arguments together through the parameter Innermost of the getTemplateInstantiationArgs(), and in that case we have to switch the next DC to its lexical declaration context, similar to what we're going to visit after a FunctionTemplateDecl in HandleFunctionTemplateDecl().

(The current implementation would always drive us to the semantic declaration context if Innermost is present.)

mizvekov · 2024-08-14T02:43:35Z

I think we could instead simplify the function to the following signature:

MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
    const Decl *D, bool LexicalParent, bool Final, .....)

Ie we take a single decl, which we will assume is a NamedDecl in case Innermost is provided, and in that case continue to either it's semantic or lexical parent, depending on LexicalParent parameter.

As an improvement on that, we can basically reuse the logic we already have for deciding which parent to visit next, which is already used in the normal case. Either refactor that, or just add an internal 'bool' parameter that controls if the implementation will add the template parameters, and call that from the 'Innermost' path with adding the arguments disabled.
That way, we can do away with the LexicalParent parameter as well.

Does that make sense? Would that work for you?

zyn0217 · 2024-08-14T03:36:22Z

simplify the function to the following signature:

I'm afraid we couldn't. We have one reliance on the DC parameter in constraint expression comparison, where we're unable to obtain a Decl as that is not formed yet. So dropping the DC parameter doesn't work here.

If you probably meant to add another overload, I think that might work; although it looks the same way we add another bool flag.

(Maybe we should promote the null checking to SubstituteConstraintExpressionWithoutSatisfaction, and that way we could remove the DC thing - that is the only place where we could pass in a null ND)

As an improvement on that, we can basically reuse the logic we already have for deciding which parent to visit next, which is already used in the normal case [...] or just add an internal 'bool' parameter that controls if the implementation will add the template parameters, and call that from the 'Innermost' path with adding the arguments disabled

Yeah, this is tempting; I was also thinking about adding an additional parameter, but that somehow makes the function more complicated and I do feel like ending up with many bool flags results in a heavier maintenance burden..

zyn0217 · 2024-08-14T03:42:35Z

Either refactor that, or just add an internal 'bool' parameter that controls if the implementation will add the template parameters, and call that from the 'Innermost' path with adding the arguments disabled.

Thinking twice, I think this is probably feasible to go. As an internal flag wouldn't be exposed to users, but we might have to refactor the function a lot more.

mizvekov · 2024-08-14T04:12:47Z

Thinking twice, I think this is probably feasible to go. As an internal flag wouldn't be exposed to users, but we might have to refactor the function a lot more.

Yep. Just don't forget to assert in case the template arguments wouldn't get added, to diagnose users passing Innermost with a declaration which doesn't take template arguments :)

…ion (#104911) (This is one step towards tweaking `getTemplateInstantiationArgs()` as discussed in #102922) We don't always substitute into default arguments while transforming a function parameter. In that case, we would preserve the uninstantiated expression until after, e.g. building up a CXXDefaultArgExpr and instantiate the expression there. For member function instantiation, this algorithm used to cause a problem in that the default argument of an out-of-line member function specialization couldn't get properly instantiated. This is because, in `getTemplateInstantiationArgs()`, we would give up visiting a function's declaration context if the function is a specialization of a member template. For example, ```cpp template <class T> struct S { template <class U> void f(T = sizeof(T)); }; template <> template <class U> void S<int>::f(int) {} ``` The default argument `sizeof(U)` that lexically appears inside the declaration would be copied to the function declaration in the class template specialization `S<int>`, as well as to the function's out-of-line definition. We use template arguments collected from the out-of-line function definition when substituting into the default arguments. We would therefore give up the traversal after the function, resulting in a single-level template argument of the `f` itself. However the default argument here could still reference the template parameters of the primary template, hence the error. In fact, this is similar to constraint checking in some respects: we actually want the "whole" template arguments relative to the primary template, not those relative to the function definition. So this patch adds another flag to indicate `getTemplateInstantiationArgs()` for that. This patch also consolidates the tests for default arguments and removes some unnecessary tests.

…ion (llvm#104911) (This is one step towards tweaking `getTemplateInstantiationArgs()` as discussed in llvm#102922) We don't always substitute into default arguments while transforming a function parameter. In that case, we would preserve the uninstantiated expression until after, e.g. building up a CXXDefaultArgExpr and instantiate the expression there. For member function instantiation, this algorithm used to cause a problem in that the default argument of an out-of-line member function specialization couldn't get properly instantiated. This is because, in `getTemplateInstantiationArgs()`, we would give up visiting a function's declaration context if the function is a specialization of a member template. For example, ```cpp template <class T> struct S { template <class U> void f(T = sizeof(T)); }; template <> template <class U> void S<int>::f(int) {} ``` The default argument `sizeof(U)` that lexically appears inside the declaration would be copied to the function declaration in the class template specialization `S<int>`, as well as to the function's out-of-line definition. We use template arguments collected from the out-of-line function definition when substituting into the default arguments. We would therefore give up the traversal after the function, resulting in a single-level template argument of the `f` itself. However the default argument here could still reference the template parameters of the primary template, hence the error. In fact, this is similar to constraint checking in some respects: we actually want the "whole" template arguments relative to the primary template, not those relative to the function definition. So this patch adds another flag to indicate `getTemplateInstantiationArgs()` for that. This patch also consolidates the tests for default arguments and removes some unnecessary tests.

zyn0217 · 2024-08-31T08:12:36Z

Superseded by #106585.

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs()

1119f0a

zyn0217 force-pushed the cwg-2369-2 branch from 401c6a9 to 1119f0a Compare August 13, 2024 01:33

zyn0217 added 2 commits August 13, 2024 09:39

Merge branch 'main' into cwg-2369-2

ca00718

Resolve a merge conflict

2dd772d

zyn0217 changed the title ~~[Clang][NFCI] Refactor getTemplateInstantiationArgs()~~ [Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() Aug 13, 2024

zyn0217 requested review from mizvekov and cor3ntin August 13, 2024 04:35

zyn0217 marked this pull request as ready for review August 13, 2024 04:35

zyn0217 requested a review from Endilll as a code owner August 13, 2024 04:35

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 13, 2024

mizvekov reviewed Aug 13, 2024

View reviewed changes

zyn0217 mentioned this pull request Aug 20, 2024

[Clang][NFCI] Cleanup the fix for default function argument substitution #104911

Merged

mizvekov mentioned this pull request Aug 29, 2024

[Clang][Sema] Refactor collection of multi-level template argument lists #106585

Merged

zyn0217 closed this Aug 31, 2024

zyn0217 deleted the cwg-2369-2 branch August 31, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() #102922

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() #102922

Uh oh!

zyn0217 commented Aug 12, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 13, 2024

Uh oh!

mizvekov left a comment •

edited

Loading

Uh oh!

zyn0217 commented Aug 14, 2024 •

edited

Loading

Uh oh!

mizvekov commented Aug 14, 2024

Uh oh!

zyn0217 commented Aug 14, 2024 •

edited

Loading

Uh oh!

zyn0217 commented Aug 14, 2024

Uh oh!

mizvekov commented Aug 14, 2024

Uh oh!

zyn0217 commented Aug 31, 2024

Uh oh!

Uh oh!

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() #102922

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs() #102922

Uh oh!

Conversation

zyn0217 commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 13, 2024

Uh oh!

mizvekov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mizvekov commented Aug 14, 2024

Uh oh!

zyn0217 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Aug 14, 2024

Uh oh!

mizvekov commented Aug 14, 2024

Uh oh!

zyn0217 commented Aug 31, 2024

Uh oh!

Uh oh!

zyn0217 commented Aug 12, 2024 •

edited

Loading

mizvekov left a comment •

edited

Loading

zyn0217 commented Aug 14, 2024 •

edited

Loading

zyn0217 commented Aug 14, 2024 •

edited

Loading