Skip to content

Reapply "[Clang][Sema] Refactor collection of multi-level template argument lists (#106585)" #111173

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

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

sdkrystian
Copy link
Member

Reapplies #106585, fixing an issue where non-dependent names of member templates appearing prior to that member template being explicitly specialized for an implicitly instantiated class template specialization would incorrectly use the definition of the explicitly specialized member template.

@sdkrystian sdkrystian requested a review from Endilll as a code owner October 4, 2024 15:26
@sdkrystian sdkrystian force-pushed the reapply-targ-collection branch from e5bd67c to dfa5179 Compare October 4, 2024 15:40
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Oct 4, 2024
@sdkrystian sdkrystian requested a review from mizvekov October 4, 2024 15:40
@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Krystian Stasiowski (sdkrystian)

Changes

Reapplies #106585, fixing an issue where non-dependent names of member templates appearing prior to that member template being explicitly specialized for an implicitly instantiated class template specialization would incorrectly use the definition of the explicitly specialized member template.


Patch is 96.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/111173.diff

18 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/AST/DeclTemplate.h (+29-53)
  • (modified) clang/include/clang/Sema/Sema.h (+6-19)
  • (modified) clang/lib/AST/Decl.cpp (+1-1)
  • (modified) clang/lib/AST/DeclTemplate.cpp (+39-20)
  • (modified) clang/lib/Sema/SemaConcept.cpp (+12-17)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+14-17)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+85-94)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+3-30)
  • (modified) clang/lib/Sema/SemaTemplateDeductionGuide.cpp (+16-29)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+350-362)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+37-9)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+2-1)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+9-9)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+7-10)
  • (modified) clang/test/AST/ast-dump-decl.cpp (+1-1)
  • (added) clang/test/CXX/temp/temp.constr/temp.constr.decl/p4.cpp (+175)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 44d5f348ed2d54..fdb1a5942e4157 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -473,6 +473,9 @@ Bug Fixes to C++ Support
   containing outer unexpanded parameters were not correctly expanded. (#GH101754)
 - Fixed a bug in constraint expression comparison where the ``sizeof...`` expression was not handled properly
   in certain friend declarations. (#GH93099)
+- Clang now uses the correct set of template argument lists when comparing the constraints of
+  out-of-line definitions and member templates explicitly specialized for a given implicit instantiation of
+  a class template. (#GH102320)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index 687715a22e9fd3..58ae7420471a6f 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -781,15 +781,11 @@ class RedeclarableTemplateDecl : public TemplateDecl,
                              EntryType *Entry, void *InsertPos);
 
   struct CommonBase {
-    CommonBase() : InstantiatedFromMember(nullptr, false) {}
+    CommonBase() {}
 
     /// The template from which this was most
     /// directly instantiated (or null).
-    ///
-    /// The boolean value indicates whether this template
-    /// was explicitly specialized.
-    llvm::PointerIntPair<RedeclarableTemplateDecl*, 1, bool>
-      InstantiatedFromMember;
+    RedeclarableTemplateDecl *InstantiatedFromMember = nullptr;
 
     /// If non-null, points to an array of specializations (including
     /// partial specializations) known only by their external declaration IDs.
@@ -809,14 +805,19 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   };
 
   /// Pointer to the common data shared by all declarations of this
-  /// template.
-  mutable CommonBase *Common = nullptr;
+  /// template, and a flag indicating if the template is a member
+  /// specialization.
+  mutable llvm::PointerIntPair<CommonBase *, 1, bool> Common;
+
+  CommonBase *getCommonPtrInternal() const { return Common.getPointer(); }
 
   /// Retrieves the "common" pointer shared by all (re-)declarations of
   /// the same template. Calling this routine may implicitly allocate memory
   /// for the common pointer.
   CommonBase *getCommonPtr() const;
 
+  void setCommonPtr(CommonBase *C) const { Common.setPointer(C); }
+
   virtual CommonBase *newCommon(ASTContext &C) const = 0;
 
   // Construct a template decl with name, parameters, and templated element.
@@ -857,15 +858,12 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   /// template<> template<typename T>
   /// struct X<int>::Inner { /* ... */ };
   /// \endcode
-  bool isMemberSpecialization() const {
-    return getCommonPtr()->InstantiatedFromMember.getInt();
-  }
+  bool isMemberSpecialization() const { return Common.getInt(); }
 
   /// Note that this member template is a specialization.
   void setMemberSpecialization() {
-    assert(getCommonPtr()->InstantiatedFromMember.getPointer() &&
-           "Only member templates can be member template specializations");
-    getCommonPtr()->InstantiatedFromMember.setInt(true);
+    assert(!isMemberSpecialization() && "already a member specialization");
+    Common.setInt(true);
   }
 
   /// Retrieve the member template from which this template was
@@ -905,12 +903,12 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   /// void X<T>::f(T, U);
   /// \endcode
   RedeclarableTemplateDecl *getInstantiatedFromMemberTemplate() const {
-    return getCommonPtr()->InstantiatedFromMember.getPointer();
+    return getCommonPtr()->InstantiatedFromMember;
   }
 
   void setInstantiatedFromMemberTemplate(RedeclarableTemplateDecl *TD) {
-    assert(!getCommonPtr()->InstantiatedFromMember.getPointer());
-    getCommonPtr()->InstantiatedFromMember.setPointer(TD);
+    assert(!getCommonPtr()->InstantiatedFromMember);
+    getCommonPtr()->InstantiatedFromMember = TD;
   }
 
   /// Retrieve the "injected" template arguments that correspond to the
@@ -1957,13 +1955,7 @@ class ClassTemplateSpecializationDecl : public CXXRecordDecl,
   /// specialization which was specialized by this.
   llvm::PointerUnion<ClassTemplateDecl *,
                      ClassTemplatePartialSpecializationDecl *>
-  getSpecializedTemplateOrPartial() const {
-    if (const auto *PartialSpec =
-            SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
-      return PartialSpec->PartialSpecialization;
-
-    return SpecializedTemplate.get<ClassTemplateDecl*>();
-  }
+  getSpecializedTemplateOrPartial() const;
 
   /// Retrieve the set of template arguments that should be used
   /// to instantiate members of the class template or class template partial
@@ -1989,6 +1981,8 @@ class ClassTemplateSpecializationDecl : public CXXRecordDecl,
   /// template arguments have been deduced.
   void setInstantiationOf(ClassTemplatePartialSpecializationDecl *PartialSpec,
                           const TemplateArgumentList *TemplateArgs) {
+    assert(!isa<ClassTemplatePartialSpecializationDecl>(this) &&
+           "A partial specialization cannot be instantiated from a template");
     assert(!SpecializedTemplate.is<SpecializedPartialSpecialization*>() &&
            "Already set to a class template partial specialization!");
     auto *PS = new (getASTContext()) SpecializedPartialSpecialization();
@@ -2000,6 +1994,8 @@ class ClassTemplateSpecializationDecl : public CXXRecordDecl,
   /// Note that this class template specialization is an instantiation
   /// of the given class template.
   void setInstantiationOf(ClassTemplateDecl *TemplDecl) {
+    assert(!isa<ClassTemplatePartialSpecializationDecl>(this) &&
+           "A partial specialization cannot be instantiated from a template");
     assert(!SpecializedTemplate.is<SpecializedPartialSpecialization*>() &&
            "Previously set to a class template partial specialization!");
     SpecializedTemplate = TemplDecl;
@@ -2187,18 +2183,11 @@ class ClassTemplatePartialSpecializationDecl
   /// struct X<int>::Inner<T*> { /* ... */ };
   /// \endcode
   bool isMemberSpecialization() const {
-    const auto *First =
-        cast<ClassTemplatePartialSpecializationDecl>(getFirstDecl());
-    return First->InstantiatedFromMember.getInt();
+    return InstantiatedFromMember.getInt();
   }
 
   /// Note that this member template is a specialization.
-  void setMemberSpecialization() {
-    auto *First = cast<ClassTemplatePartialSpecializationDecl>(getFirstDecl());
-    assert(First->InstantiatedFromMember.getPointer() &&
-           "Only member templates can be member template specializations");
-    return First->InstantiatedFromMember.setInt(true);
-  }
+  void setMemberSpecialization() { return InstantiatedFromMember.setInt(true); }
 
   /// Retrieves the injected specialization type for this partial
   /// specialization.  This is not the same as the type-decl-type for
@@ -2268,10 +2257,6 @@ class ClassTemplateDecl : public RedeclarableTemplateDecl {
     return static_cast<Common *>(RedeclarableTemplateDecl::getCommonPtr());
   }
 
-  void setCommonPtr(Common *C) {
-    RedeclarableTemplateDecl::Common = C;
-  }
-
 public:
 
   friend class ASTDeclReader;
@@ -2722,13 +2707,7 @@ class VarTemplateSpecializationDecl : public VarDecl,
   /// Retrieve the variable template or variable template partial
   /// specialization which was specialized by this.
   llvm::PointerUnion<VarTemplateDecl *, VarTemplatePartialSpecializationDecl *>
-  getSpecializedTemplateOrPartial() const {
-    if (const auto *PartialSpec =
-            SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
-      return PartialSpec->PartialSpecialization;
-
-    return SpecializedTemplate.get<VarTemplateDecl *>();
-  }
+  getSpecializedTemplateOrPartial() const;
 
   /// Retrieve the set of template arguments that should be used
   /// to instantiate the initializer of the variable template or variable
@@ -2754,6 +2733,8 @@ class VarTemplateSpecializationDecl : public VarDecl,
   /// template arguments have been deduced.
   void setInstantiationOf(VarTemplatePartialSpecializationDecl *PartialSpec,
                           const TemplateArgumentList *TemplateArgs) {
+    assert(!isa<VarTemplatePartialSpecializationDecl>(this) &&
+           "A partial specialization cannot be instantiated from a template");
     assert(!SpecializedTemplate.is<SpecializedPartialSpecialization *>() &&
            "Already set to a variable template partial specialization!");
     auto *PS = new (getASTContext()) SpecializedPartialSpecialization();
@@ -2765,6 +2746,8 @@ class VarTemplateSpecializationDecl : public VarDecl,
   /// Note that this variable template specialization is an instantiation
   /// of the given variable template.
   void setInstantiationOf(VarTemplateDecl *TemplDecl) {
+    assert(!isa<VarTemplatePartialSpecializationDecl>(this) &&
+           "A partial specialization cannot be instantiated from a template");
     assert(!SpecializedTemplate.is<SpecializedPartialSpecialization *>() &&
            "Previously set to a variable template partial specialization!");
     SpecializedTemplate = TemplDecl;
@@ -2949,18 +2932,11 @@ class VarTemplatePartialSpecializationDecl
   /// U* X<int>::Inner<T*> = (T*)(0) + 1;
   /// \endcode
   bool isMemberSpecialization() const {
-    const auto *First =
-        cast<VarTemplatePartialSpecializationDecl>(getFirstDecl());
-    return First->InstantiatedFromMember.getInt();
+    return InstantiatedFromMember.getInt();
   }
 
   /// Note that this member template is a specialization.
-  void setMemberSpecialization() {
-    auto *First = cast<VarTemplatePartialSpecializationDecl>(getFirstDecl());
-    assert(First->InstantiatedFromMember.getPointer() &&
-           "Only member templates can be member template specializations");
-    return First->InstantiatedFromMember.setInt(true);
-  }
+  void setMemberSpecialization() { return InstantiatedFromMember.setInt(true); }
 
   SourceRange getSourceRange() const override LLVM_READONLY;
 
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index bede971ce0191b..7ff9c2754a6fe0 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11325,9 +11325,9 @@ class Sema final : public SemaBase {
       CXXScopeSpec &SS, IdentifierInfo *Name, SourceLocation NameLoc,
       const ParsedAttributesView &Attr, TemplateParameterList *TemplateParams,
       AccessSpecifier AS, SourceLocation ModulePrivateLoc,
-      SourceLocation FriendLoc, unsigned NumOuterTemplateParamLists,
-      TemplateParameterList **OuterTemplateParamLists,
-      SkipBodyInfo *SkipBody = nullptr);
+      SourceLocation FriendLoc,
+      ArrayRef<TemplateParameterList *> OuterTemplateParamLists,
+      bool IsMemberSpecialization, SkipBodyInfo *SkipBody = nullptr);
 
   /// Translates template arguments as provided by the parser
   /// into template arguments used by semantic analysis.
@@ -11366,7 +11366,8 @@ class Sema final : public SemaBase {
   DeclResult ActOnVarTemplateSpecialization(
       Scope *S, Declarator &D, TypeSourceInfo *DI, LookupResult &Previous,
       SourceLocation TemplateKWLoc, TemplateParameterList *TemplateParams,
-      StorageClass SC, bool IsPartialSpecialization);
+      StorageClass SC, bool IsPartialSpecialization,
+      bool IsMemberSpecialization);
 
   /// Get the specialization of the given variable template corresponding to
   /// the specified argument list, or a null-but-valid result if the arguments
@@ -13017,28 +13018,14 @@ class Sema final : public SemaBase {
   /// dealing with a specialization. This is only relevant for function
   /// template specializations.
   ///
-  /// \param Pattern If non-NULL, indicates the pattern from which we will be
-  /// instantiating the definition of the given declaration, \p ND. This is
-  /// used to determine the proper set of template instantiation arguments for
-  /// friend function template specializations.
-  ///
   /// \param ForConstraintInstantiation when collecting arguments,
   /// 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.
-  /// \param ForDefaultArgumentSubstitution indicates we should continue looking
-  /// when encountering a specialized member function template, rather than
-  /// returning immediately.
   MultiLevelTemplateArgumentList getTemplateInstantiationArgs(
       const NamedDecl *D, const DeclContext *DC = nullptr, bool Final = false,
       std::optional<ArrayRef<TemplateArgument>> Innermost = std::nullopt,
-      bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr,
-      bool ForConstraintInstantiation = false,
-      bool SkipForSpecialization = false,
-      bool ForDefaultArgumentSubstitution = false);
+      bool RelativeToPrimary = false, bool ForConstraintInstantiation = false);
 
   /// RAII object to handle the state changes required to synthesize
   /// a function body.
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 84ef9f74582ef6..1dbe6ceae97c3a 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4159,7 +4159,7 @@ FunctionTemplateDecl *FunctionDecl::getPrimaryTemplate() const {
   if (FunctionTemplateSpecializationInfo *Info
         = TemplateOrSpecialization
             .dyn_cast<FunctionTemplateSpecializationInfo*>()) {
-    return Info->getTemplate();
+    return Info->getTemplate()->getMostRecentDecl();
   }
   return nullptr;
 }
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 6fe817c5ef1c6b..2781863bfd9610 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -309,16 +309,16 @@ bool TemplateDecl::isTypeAlias() const {
 void RedeclarableTemplateDecl::anchor() {}
 
 RedeclarableTemplateDecl::CommonBase *RedeclarableTemplateDecl::getCommonPtr() const {
-  if (Common)
-    return Common;
+  if (CommonBase *C = getCommonPtrInternal())
+    return C;
 
   // Walk the previous-declaration chain until we either find a declaration
   // with a common pointer or we run out of previous declarations.
   SmallVector<const RedeclarableTemplateDecl *, 2> PrevDecls;
   for (const RedeclarableTemplateDecl *Prev = getPreviousDecl(); Prev;
        Prev = Prev->getPreviousDecl()) {
-    if (Prev->Common) {
-      Common = Prev->Common;
+    if (CommonBase *C = Prev->getCommonPtrInternal()) {
+      setCommonPtr(C);
       break;
     }
 
@@ -326,18 +326,18 @@ RedeclarableTemplateDecl::CommonBase *RedeclarableTemplateDecl::getCommonPtr() c
   }
 
   // If we never found a common pointer, allocate one now.
-  if (!Common) {
+  if (!getCommonPtrInternal()) {
     // FIXME: If any of the declarations is from an AST file, we probably
     // need an update record to add the common data.
 
-    Common = newCommon(getASTContext());
+    setCommonPtr(newCommon(getASTContext()));
   }
 
   // Update any previous declarations we saw with the common pointer.
   for (const RedeclarableTemplateDecl *Prev : PrevDecls)
-    Prev->Common = Common;
+    Prev->setCommonPtr(getCommonPtrInternal());
 
-  return Common;
+  return getCommonPtrInternal();
 }
 
 void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const {
@@ -463,19 +463,17 @@ void FunctionTemplateDecl::addSpecialization(
 }
 
 void FunctionTemplateDecl::mergePrevDecl(FunctionTemplateDecl *Prev) {
-  using Base = RedeclarableTemplateDecl;
-
   // If we haven't created a common pointer yet, then it can just be created
   // with the usual method.
-  if (!Base::Common)
+  if (!getCommonPtrInternal())
     return;
 
-  Common *ThisCommon = static_cast<Common *>(Base::Common);
+  Common *ThisCommon = static_cast<Common *>(getCommonPtrInternal());
   Common *PrevCommon = nullptr;
   SmallVector<FunctionTemplateDecl *, 8> PreviousDecls;
   for (; Prev; Prev = Prev->getPreviousDecl()) {
-    if (Prev->Base::Common) {
-      PrevCommon = static_cast<Common *>(Prev->Base::Common);
+    if (CommonBase *C = Prev->getCommonPtrInternal()) {
+      PrevCommon = static_cast<Common *>(C);
       break;
     }
     PreviousDecls.push_back(Prev);
@@ -485,7 +483,7 @@ void FunctionTemplateDecl::mergePrevDecl(FunctionTemplateDecl *Prev) {
   // use this common pointer.
   if (!PrevCommon) {
     for (auto *D : PreviousDecls)
-      D->Base::Common = ThisCommon;
+      D->setCommonPtr(ThisCommon);
     return;
   }
 
@@ -493,7 +491,7 @@ void FunctionTemplateDecl::mergePrevDecl(FunctionTemplateDecl *Prev) {
   assert(ThisCommon->Specializations.size() == 0 &&
          "Can't merge incompatible declarations!");
 
-  Base::Common = PrevCommon;
+  setCommonPtr(PrevCommon);
 }
 
 //===----------------------------------------------------------------------===//
@@ -999,12 +997,23 @@ void ClassTemplateSpecializationDecl::getNameForDiagnostic(
   }
 }
 
+llvm::PointerUnion<ClassTemplateDecl *,
+                   ClassTemplatePartialSpecializationDecl *>
+ClassTemplateSpecializationDecl::getSpecializedTemplateOrPartial() const {
+  if (const auto *PartialSpec =
+          SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
+    return PartialSpec->PartialSpecialization->getMostRecentDecl();
+
+  return SpecializedTemplate.get<ClassTemplateDecl *>()->getMostRecentDecl();
+}
+
 ClassTemplateDecl *
 ClassTemplateSpecializationDecl::getSpecializedTemplate() const {
   if (const auto *PartialSpec =
           SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization*>())
-    return PartialSpec->PartialSpecialization->getSpecializedTemplate();
-  return SpecializedTemplate.get<ClassTemplateDecl*>();
+    return PartialSpec->PartialSpecialization->getSpecializedTemplate()
+        ->getMostRecentDecl();
+  return SpecializedTemplate.get<ClassTemplateDecl *>()->getMostRecentDecl();
 }
 
 SourceRange
@@ -1412,11 +1421,21 @@ void VarTemplateSpecializationDecl::getNameForDiagnostic(
   }
 }
 
+llvm::PointerUnion<VarTemplateDecl *, VarTemplatePartialSpecializationDecl *>
+VarTemplateSpecializationDecl::getSpecializedTemplateOrPartial() const {
+  if (const auto *PartialSpec =
+          SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
+    return PartialSpec->PartialSpecialization->getMostRecentDecl();
+
+  return SpecializedTemplate.get<VarTemplateDecl *>()->getMostRecentDecl();
+}
+
 VarTemplateDecl *VarTemplateSpecializationDecl::getSpecializedTemplate() const {
   if (const auto *PartialSpec =
           SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
-    return PartialSpec->PartialSpecialization->getSpecializedTemplate();
-  return SpecializedTemplate.get<VarTemplateDecl *>();
+    return PartialSpec->PartialSpecialization->getSpecializedTemplate()
+        ->getMostRecentDecl();
+  return SpecializedTemplate.get<VarTemplateDecl *>()->getMostRecentDecl();
 }
 
 SourceRange VarTemplateSpecializationDecl::getSourceRange() const {
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 998a148a7d24a1..e36ee062213716 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -585,8 +585,8 @@ static bool CheckConstraintSatisfaction(
 
   ArrayRef<TemplateArgument> TemplateArgs =
       TemplateArgsLists.getNumSubstitutedLevels() > 0
-          ? TemplateArgsLists.getOutermost()
-          : ArrayRef<TemplateArgument> {};
+          ? TemplateArgsLists.getInnermost()
+          : ArrayRef<TemplateArgument>{};
   Sema::InstantiatingTemplate Inst(S, TemplateIDRange.getBegin(),
       Sema::InstantiatingTemplate::ConstraintsCheck{},
       const_cast<NamedDecl *>(Template), TemplateArgs, TemplateIDRange);
@@ -834,7 +834,6 @@ Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
       getTemplateInstantiationArgs(FD, FD->getLexicalDeclContext(),
                                    /*Final=*/false, /*Innermost=*/std::nullopt,
                                    /*RelativeToPrimary=*/true,
-                                   /*Pattern=*/nullptr,
                                    /*ForConstraintInstantiation=*/true);
   if (SetupConstraint...
[truncated]

@erichkeane
Copy link
Collaborator

Can you point out the diff from the last review? It isn't clear.

@sdkrystian
Copy link
Member Author

@erichkeane All changes made since the last review are in dfa5179

@zyn0217
Copy link
Contributor

zyn0217 commented Oct 4, 2024

Can you also add the previous regressing case to the test? Thanks
#106585 (comment)

@sdkrystian
Copy link
Member Author

@zyn0217 I have quite a few test cases I'll be adding soon

@sdkrystian sdkrystian force-pushed the reapply-targ-collection branch from e93a98b to d312bd4 Compare October 7, 2024 15:28
@sdkrystian sdkrystian requested a review from mizvekov October 7, 2024 16:04
@sdkrystian sdkrystian force-pushed the reapply-targ-collection branch from 004f24b to 4515c64 Compare October 8, 2024 13:24
@sdkrystian sdkrystian merged commit 4da8ac3 into llvm:main Oct 8, 2024
7 of 8 checks passed
@mizvekov
Copy link
Contributor

mizvekov commented Oct 8, 2024

FYI I just realized the Final parameter is not wired up to the Collecter. It appertains to the Innermost argument.

@sdkrystian
Copy link
Member Author

FYI I just realized the Final parameter is not wired up to the Collecter. It appertains to the Innermost argument.

Oops! I don't think it affects functionality though, so it can probably be addressed in a follow-up patch

@mizvekov
Copy link
Contributor

mizvekov commented Oct 8, 2024

Yeah, I think its only user was reverted a while ago, but should be merged back eventually.

@mstorsjo
Copy link
Member

mstorsjo commented Oct 9, 2024

This still breaks building Qt, in the same source file as before, but with a different error.

Standalone reproducible with https://martin.st/temp/qwindowsystem-preproc.cpp:

$ clang -target i686-w64-mingw32 -c qwindowsystem-preproc.cpp -w
qwindowsystem-preproc.cpp:150631:73: error: cannot initialize a parameter of type 'QWindowSystemInterfacePrivate::WindowSystemEvent *' with an rvalue of type 'QWindowSystemInterface::SynchronousDelivery *'
 150631 |             if (!QWindowSystemInterfacePrivate::eventHandler->sendEvent(&event))
        |                                                                         ^~~~~~
qwindowsystem-preproc.cpp:150621:77: note: in instantiation of function template specialization 'QWindowSystemHelper<QWindowSystemInterface::SynchronousDelivery>::handleEvent<QWindowSystemInterfacePrivate::EnterEvent, QWindow *, QPointF, QPointF>' requested here
 150621 |         ? QWindowSystemHelper<QWindowSystemInterface::SynchronousDelivery>::handleEvent<EventType>(args...)
        |                                                                             ^
qwindowsystem-preproc.cpp:150654:52: note: in instantiation of function template specialization 'QWindowSystemHelper<QWindowSystemInterface::DefaultDelivery>::handleEvent<QWindowSystemInterfacePrivate::EnterEvent, QWindow *, QPointF, QPointF>' requested here
 150654 |     return QWindowSystemHelper<Delivery>::template handleEvent<EventType>(args...);
        |                                                    ^
qwindowsystem-preproc.cpp:150702:9: note: in instantiation of function template specialization 'handleWindowSystemEvent<QWindowSystemInterfacePrivate::EnterEvent, QWindowSystemInterface::DefaultDelivery, QWindow *, QPointF, QPointF>' requested here
 150702 |         handleWindowSystemEvent<QWindowSystemInterfacePrivate::EnterEvent, Delivery>(window,
        |         ^
qwindowsystem-preproc.cpp:150699:66: note: in instantiation of function template specialization 'QWindowSystemInterface::handleEnterEvent<QWindowSystemInterface::DefaultDelivery>' requested here
 150699 | template __attribute__((dllexport)) void QWindowSystemInterface::handleEnterEvent<QWindowSystemInterface::DefaultDelivery>(QWindow *window, const QPointF &local, const QPointF &global); template __attribute__((dllexport)) void QWindowSystemInterface::handleEnterEvent<QWindowSystemInterface::SynchronousDelivery>(QWindow *window, const QPointF &local, const QPointF &global); template __attribute__((dllexport)) void QWindowSystemInterface::handleEnterEvent<QWindowSystemInterface::AsynchronousDelivery>(QWindow *window, const QPointF &local, const QPointF &global); template<typename Delivery> void QWindowSystemInterface::handleEnterEvent(QWindow *window, const QPointF &local, const QPointF &global)
        |                                                                  ^
qwindowsystem-preproc.cpp:146799:78: note: passing argument to parameter 'event' here
 146799 |     virtual bool sendEvent(QWindowSystemInterfacePrivate::WindowSystemEvent *event);
        |                                                                              ^

Also reproducible in a full build (for e.g. linux) with the same instructions as last time:

$ git clone https://github.com/qt/qtbase
$ cd qtbase
$ git checkout v6.8.0
$ mkdir build
$ cd build
$ cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DINPUT_opengl=no
$ ninja

(The same non-reduced standalone reproducer from the last round also shows the exact same issue.)

I'll go ahead and push a revert again.

@mstorsjo
Copy link
Member

mstorsjo commented Oct 9, 2024

This no longer reverts cleanly - 4336f00 (#110387) interferes; can someone more involved in this take it upon themselves to sort this out (revert back to green again)? I can of course revert both too.

@zyn0217
Copy link
Contributor

zyn0217 commented Oct 9, 2024

@mstorsjo I think you can revert both -- @mizvekov's patch could get rid of this reliance in some other way.

@sdkrystian
Copy link
Member Author

sdkrystian commented Oct 9, 2024

Reduced to:

template<int I>
struct A
{
    template<int J>
    static constexpr int f();
};

template<>
template<int J>
constexpr int A<0>::f()
{
    return A<1>::f<J>();
}

template<>
template<int J>
constexpr int A<1>::f()
{
    return J;
}

static_assert(A<0>::f<2>() == 2); // error: static assertion failed due to requirement 'A<0>::f() == 2'

The error does not occur if the explicit specialization of A<1>::f is moved before the explicit specialization of A<0>::f.

@mstorsjo
Copy link
Member

mstorsjo commented Oct 9, 2024

Ah, I never got around to pushing the reverts earlier. Do you think you'll have a fix within a couple of hours, or should we just go ahead with the revert and do a clean reland later, once this issue is sorted out (and you've checked that Qt actually does compile still - there seems to be many issues in this area in that file).

@sdkrystian
Copy link
Member Author

Do you think you'll have a fix within a couple of hours

@mstorsjo I think so. If not, I'll revert.

@mizvekov
Copy link
Contributor

mizvekov commented Oct 9, 2024

FWIW 4336f00 does not really depend on this patch significantly, the same change more or less should work, you just need to adjust for the changes in the function signature.

@sdkrystian
Copy link
Member Author

@mizvekov So, after some experimentation, I think the correct solution is my original change which augments getPrimaryTemplate and related functions to return the most recent declaration. Otherwise, the number of cases where we accidentally use the wrong definition of a function/class/variable template will be innumerable.

@mstorsjo
Copy link
Member

mstorsjo commented Oct 9, 2024

Can we land a fix, or can we get a revert pushed?

@sdkrystian
Copy link
Member Author

@mschessler Unless @erichkeane, @zyn0217, or @mizvekov object, I think we should merge the original embodiment of my reapplication of the patch.

@mschessler
Copy link
Contributor

Not exactly sure how I'm involved here @sdkrystian ?

@sdkrystian
Copy link
Member Author

@mschessler Sorry, I meant @mstorsjo :)

sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Oct 9, 2024
sdkrystian added a commit that referenced this pull request Oct 9, 2024
@mizvekov
Copy link
Contributor

I am not sure I can offer a better option here. The global change seems odd, and I think it may run into other problems.

One option would be to do the big hammer approach as you suggest, but then afterwards back out and then try to nail the places where the most recent declaration is actually required. This way, the patch which needs to be reverted is much smaller, causing less churn.
Another option would be to introduce another helper getPrimaryTemplateAsWritten, and then gradually replace getPrimaryTemplate uses.

@mstorsjo
Copy link
Member

Thanks @sdkrystian for pushing the revert - now my nightly tests this night were back to green :-)

sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Oct 10, 2024
sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Oct 11, 2024
sdkrystian added a commit that referenced this pull request Oct 11, 2024
…gument lists (#106585, #111173)" (#111852)

This patch reapplies #111173, fixing a bug when instantiating dependent
expressions that name a member template that is later explicitly
specialized for a class specialization that is implicitly instantiated.

The bug is addressed by adding the `hasMemberSpecialization` function,
which return `true` if _any_ redeclaration is a member specialization.
This is then used when determining the instantiation pattern for a
specialization of a template, and when collecting template arguments for
a specialization of a template.
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…gument lists (llvm#106585, llvm#111173)" (llvm#111852)

This patch reapplies llvm#111173, fixing a bug when instantiating dependent
expressions that name a member template that is later explicitly
specialized for a class specialization that is implicitly instantiated.

The bug is addressed by adding the `hasMemberSpecialization` function,
which return `true` if _any_ redeclaration is a member specialization.
This is then used when determining the instantiation pattern for a
specialization of a template, and when collecting template arguments for
a specialization of a template.
sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Nov 6, 2024
sdkrystian added a commit that referenced this pull request Nov 6, 2024
sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Jan 22, 2025
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:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants