Skip to content

[Clang] Defer the instantiation of explicit-specifier until constraint checking completes #70548

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

Conversation

LYP951018
Copy link
Contributor

@LYP951018 LYP951018 commented Oct 28, 2023

Modifications:

  • Skip the instantiation of the explicit-specifier during Decl substitution if we are deducing template arguments and the explicit-specifier is value dependent.

  • Instantiate the explicit-specifier after the constraint checking completes.

  • Make instantiateExplicitSpecifier a member function in order to instantiate the explicit-specifier in different stages.

This PR doesn’t defer the instantiation of the explicit specifier for deduction guides, because I’m not familiar with deduction guides yet. I’ll dig into it after this PR.

According to my local test, GCC 13 tuple works with this PR.

Fixes #59827.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 28, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2023

@llvm/pr-subscribers-clang

Author: 刘雨培 (LYP951018)

Changes

Modifications:

  • Skipped the instantiation of the explicit-specifier during Decl substitution if we were deducing template arguments and the explicit-specifier was value dependent.

  • Instantiated the explicit-specifier after the constraint checking completed.

  • Made instantiateExplicitSpecifier a member function in order to instantiate the explicit-specifier in different stages.

This PR doesn’t defer the instantiation of the explicit specifier for deduction guides, because I’m not familiar with deduction guides yet. I’ll dig into it after this PR.

According to my local test, GCC 13 tuple works with this PR.

Fixes #59827.


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

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4)
  • (modified) clang/include/clang/Sema/Sema.h (+3)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+56)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+26-14)
  • (added) clang/test/SemaCXX/cxx2a-explicit-bool-deferred.cpp (+31)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2595737e8b3b143..ad41a831477f66c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -670,6 +670,10 @@ Bug Fixes to C++ Support
   default initializing a base class in a constant expression context. Fixes:
   (`#69890 <https://github.com/llvm/llvm-project/issues/69890>`_)
 
+- Clang now defers the instantiation of explicit specifier until constraint checking
+  completes (except deduction guides). Fixes: 
+  (`#59827 <https://github.com/llvm/llvm-project/issues/59827>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1e9752345ffd173..93fd6507db3d78a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10430,6 +10430,9 @@ class Sema final {
                                   const CXXConstructorDecl *Tmpl,
                             const MultiLevelTemplateArgumentList &TemplateArgs);
 
+  ExplicitSpecifier instantiateExplicitSpecifier(
+      const MultiLevelTemplateArgumentList &TemplateArgs, ExplicitSpecifier ES);
+
   NamedDecl *FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D,
                           const MultiLevelTemplateArgumentList &TemplateArgs,
                           bool FindingInstantiatedContext = false);
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 0b3f0247ea3bee3..e75e46aa5228812 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -3553,6 +3553,53 @@ static unsigned getPackIndexForParam(Sema &S,
   llvm_unreachable("parameter index would not be produced from template");
 }
 
+// if `Specialization` is a `CXXConstructorDecl` or `CXXConversionDecl`
+// we try to instantiate and update its explicit specifier after constraint
+// checking.
+static Sema::TemplateDeductionResult
+tryInstantiateExplicitSpecifier(Sema &S, FunctionDecl *Specialization,
+                                const MultiLevelTemplateArgumentList &SubstArgs,
+                                TemplateDeductionInfo &Info,
+                                FunctionTemplateDecl *FunctionTemplate,
+                                ArrayRef<TemplateArgument> DeducedArgs) {
+
+  const auto TryInstantiateExplicitSpecifierForSingleDecl =
+      [&](auto *ExplicitDecl) {
+        ExplicitSpecifier ExplicitSpecifier =
+            ExplicitDecl->getExplicitSpecifier();
+        Expr *const Expr = ExplicitSpecifier.getExpr();
+        if (!Expr) {
+          return Sema::TDK_Success;
+        }
+        if (!Expr->isValueDependent()) {
+          return Sema::TDK_Success;
+        }
+        Sema::InstantiatingTemplate Inst(
+            S, Info.getLocation(), FunctionTemplate, DeducedArgs,
+            Sema::CodeSynthesisContext::DeducedTemplateArgumentSubstitution,
+            Info);
+        const auto Instantiated =
+            S.instantiateExplicitSpecifier(SubstArgs, ExplicitSpecifier);
+        if (Instantiated.isInvalid()) {
+          ExplicitDecl->setInvalidDecl(true);
+          return clang::Sema::TDK_SubstitutionFailure;
+        }
+        ExplicitDecl->setExplicitSpecifier(Instantiated);
+        return clang::Sema::TDK_Success;
+      };
+  Sema::TemplateDeductionResult DeductionResult = clang::Sema::TDK_Success;
+  if (CXXConstructorDecl *ConstructorDecl =
+          dyn_cast_or_null<CXXConstructorDecl>(Specialization)) {
+    DeductionResult =
+        TryInstantiateExplicitSpecifierForSingleDecl(ConstructorDecl);
+  } else if (CXXConversionDecl *ConversionDecl =
+                 dyn_cast_or_null<CXXConversionDecl>(Specialization)) {
+    DeductionResult =
+        TryInstantiateExplicitSpecifierForSingleDecl(ConversionDecl);
+  }
+  return DeductionResult;
+}
+
 /// Finish template argument deduction for a function template,
 /// checking the deduced template arguments for completeness and forming
 /// the function template specialization.
@@ -3682,6 +3729,15 @@ Sema::TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
     }
   }
 
+  // We skipped the instantiation of the explicit-specifier during subst the
+  // decl before. Now, we try to instantiate it back if the Specialization is a
+  // constructor or a conversion.
+  if (TDK_Success !=
+      tryInstantiateExplicitSpecifier(*this, Specialization, SubstArgs, Info,
+                                      FunctionTemplate, DeducedArgs)) {
+    return TDK_SubstitutionFailure;
+  }
+
   if (OriginalCallArgs) {
     // C++ [temp.deduct.call]p4:
     //   In general, the deduction process attempts to find template argument
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 78a7892a35a320b..163dbef03e80a4d 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -563,18 +563,16 @@ static void instantiateDependentAMDGPUFlatWorkGroupSizeAttr(
   S.addAMDGPUFlatWorkGroupSizeAttr(New, Attr, MinExpr, MaxExpr);
 }
 
-static ExplicitSpecifier
-instantiateExplicitSpecifier(Sema &S,
-                             const MultiLevelTemplateArgumentList &TemplateArgs,
-                             ExplicitSpecifier ES, FunctionDecl *New) {
+ExplicitSpecifier Sema::instantiateExplicitSpecifier(
+    const MultiLevelTemplateArgumentList &TemplateArgs, ExplicitSpecifier ES) {
   if (!ES.getExpr())
     return ES;
   Expr *OldCond = ES.getExpr();
   Expr *Cond = nullptr;
   {
     EnterExpressionEvaluationContext Unevaluated(
-        S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
-    ExprResult SubstResult = S.SubstExpr(OldCond, TemplateArgs);
+        *this, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+    ExprResult SubstResult = SubstExpr(OldCond, TemplateArgs);
     if (SubstResult.isInvalid()) {
       return ExplicitSpecifier::Invalid();
     }
@@ -582,7 +580,7 @@ instantiateExplicitSpecifier(Sema &S,
   }
   ExplicitSpecifier Result(Cond, ES.getKind());
   if (!Cond->isTypeDependent())
-    S.tryResolveExplicitSpecifier(Result);
+    tryResolveExplicitSpecifier(Result);
   return Result;
 }
 
@@ -2073,8 +2071,8 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
 
   ExplicitSpecifier InstantiatedExplicitSpecifier;
   if (auto *DGuide = dyn_cast<CXXDeductionGuideDecl>(D)) {
-    InstantiatedExplicitSpecifier = instantiateExplicitSpecifier(
-        SemaRef, TemplateArgs, DGuide->getExplicitSpecifier(), DGuide);
+    InstantiatedExplicitSpecifier = SemaRef.instantiateExplicitSpecifier(
+        TemplateArgs, DGuide->getExplicitSpecifier());
     if (InstantiatedExplicitSpecifier.isInvalid())
       return nullptr;
   }
@@ -2453,11 +2451,25 @@ Decl *TemplateDeclInstantiator::VisitCXXMethodDecl(
     }
   }
 
-  ExplicitSpecifier InstantiatedExplicitSpecifier =
-      instantiateExplicitSpecifier(SemaRef, TemplateArgs,
-                                   ExplicitSpecifier::getFromDecl(D), D);
-  if (InstantiatedExplicitSpecifier.isInvalid())
-    return nullptr;
+  auto InstantiatedExplicitSpecifier = ExplicitSpecifier::getFromDecl(D);
+  // deduction guides need this
+  const bool CouldInstantiate =
+      InstantiatedExplicitSpecifier.getExpr() == nullptr ||
+      !InstantiatedExplicitSpecifier.getExpr()->isValueDependent();
+
+  // Delay the instantiation of the explicit-specifier until after the
+  // constraints are checked during template argument deduction.
+  if (CouldInstantiate ||
+      SemaRef.CodeSynthesisContexts.back().Kind !=
+          Sema::CodeSynthesisContext::DeducedTemplateArgumentSubstitution) {
+    InstantiatedExplicitSpecifier = SemaRef.instantiateExplicitSpecifier(
+        TemplateArgs, InstantiatedExplicitSpecifier);
+
+    if (InstantiatedExplicitSpecifier.isInvalid())
+      return nullptr;
+  } else {
+    InstantiatedExplicitSpecifier.setKind(ExplicitSpecKind::Unresolved);
+  }
 
   // Implicit destructors/constructors created for local classes in
   // DeclareImplicit* (see SemaDeclCXX.cpp) might not have an associated TSI.
diff --git a/clang/test/SemaCXX/cxx2a-explicit-bool-deferred.cpp b/clang/test/SemaCXX/cxx2a-explicit-bool-deferred.cpp
new file mode 100644
index 000000000000000..4d667008f2e2763
--- /dev/null
+++ b/clang/test/SemaCXX/cxx2a-explicit-bool-deferred.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+
+template <typename T1, typename T2> struct is_same {
+  static constexpr bool value = false;
+};
+
+template <typename T> struct is_same<T, T> {
+  static constexpr bool value = true;
+};
+
+template <class T, class U>
+concept SameHelper = is_same<T, U>::value;
+template <class T, class U>
+concept same_as = SameHelper<T, U> && SameHelper<U, T>;
+
+namespace deferred_instantiation {
+template <class X> constexpr X do_not_instantiate() { return nullptr; }
+
+struct T {
+  template <same_as<float> X> explicit(do_not_instantiate<X>()) T(X) {}
+
+  T(int) {}
+};
+
+T t(5);
+// expected-error@17{{cannot initialize}}
+// expected-note@20{{in instantiation of function template specialization}}
+// expected-note@30{{while substituting deduced template arguments}}
+// expected-note@30{{in instantiation of function template specialization}}
+T t2(5.0f);
+} // namespace deferred_instantiation

@LYP951018 LYP951018 changed the title Defer the instantiation of explicit-specifier until constraint checking completes [Clang] Defer the instantiation of explicit-specifier until constraint checking completes Oct 28, 2023
@LYP951018 LYP951018 force-pushed the deferred_explicit_specifier_instantiation branch 2 times, most recently from 4f8111b to a5929dd Compare October 28, 2023 11:23
Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this! Some nit comments, hope you don't mind.
(Also invited some clang folks to have a detailed look at this. :=)

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to me, please give other reviewers another day to take a look before merging though.

@LYP951018 LYP951018 force-pushed the deferred_explicit_specifier_instantiation branch from 9912122 to 522998e Compare October 31, 2023 16:02
Copy link

github-actions bot commented Oct 31, 2023

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

@LYP951018 LYP951018 force-pushed the deferred_explicit_specifier_instantiation branch 2 times, most recently from 961ab6c to e6ed716 Compare October 31, 2023 16:23
@LYP951018 LYP951018 force-pushed the deferred_explicit_specifier_instantiation branch from bba3006 to 0ef00fd Compare October 31, 2023 16:55
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a better run-through again, so these are all thats left for me. Thanks for the patience!

@LYP951018 LYP951018 requested a review from shafik October 31, 2023 17:17
Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to make sure we have enough testing in place for this change but overall I think I am happy.

Sema::SFINAETrap Trap(S);
const ExplicitSpecifier InstantiatedES =
S.instantiateExplicitSpecifier(SubstArgs, ES);
if (InstantiatedES.isInvalid() || Trap.hasErrorOccurred()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test that checks this case at all? Same for the other failures, if we know how to test them we should write a test to verify them.

@LYP951018 LYP951018 force-pushed the deferred_explicit_specifier_instantiation branch from 35d0d68 to aaa687e Compare November 1, 2023 01:35
@erichkeane erichkeane merged commit 128b3b6 into llvm:main Nov 1, 2023
krwalker pushed a commit to krwalker/llvm-project that referenced this pull request Nov 14, 2023
…t checking completes (llvm#70548)

Modifications:

- Skip the instantiation of the explicit-specifier during Decl
substitution if we are deducing template arguments and the
explicit-specifier is value dependent.

- Instantiate the explicit-specifier after the constraint checking
completes.

- Make `instantiateExplicitSpecifier` a member function in order to
instantiate the explicit-specifier in different stages.

This PR doesn’t defer the instantiation of the explicit specifier for
deduction guides, because I’m not familiar with deduction guides yet.
I’ll dig into it after this PR.

According to my local test, GCC 13 tuple works with this PR.

Fixes llvm#59827.

---------

Co-authored-by: Erich Keane <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang][concepts] Conditional explicit specifier is checked too early in constrained constructor
5 participants