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] Fix CTAD not work for C++ explicit type conversion (functional annotation). #75779

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Dec 18, 2023

This fixes #64347.

The CTAD for an aggregate class is missing to handle the explicit type conversion case, e.g. TemplateFooClass(1, 2);. Per C++ expr.type.conv p1, the deduced type is the return type of the deduction guide selected by the CTAD for the reminder.

In the deduction implementation DeduceTemplateSpecializationFromInitializer, the parenthesized express-list case relies on the ParenListExpr parameter (default is nullptr), the AST ParenListExpr node is not built for all variant initializer cases (BuildCXXTypeConstructorExpr, BuildCXXNew etc), thus the deduction doesn't perform for these cases. This patch fixes it by removing the ParenListExpr and using the Inits instead (which also simplifies the interface and implementation).

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

llvmbot commented Dec 18, 2023

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fixes #64347


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

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaInit.cpp (+7-4)
  • (added) clang/test/SemaCXX/ctad.cpp (+19)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f5ae6bb8925202..0bb614c2237794 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -685,6 +685,8 @@ Bug Fixes in This Version
   (`#62157 <https://github.com/llvm/llvm-project/issues/62157>`_) and
   (`#64885 <https://github.com/llvm/llvm-project/issues/64885>`_) and
   (`#65568 <https://github.com/llvm/llvm-project/issues/65568>`_)
+- Fix an issue where CTAD fails for explicit type conversion. Fixes
+  (#64347 <https://github.com/llvm/llvm-project/issues/64347>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index be6a136ef37bc4..85e1d28b0e3fdd 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -12957,7 +12957,7 @@ QualType Sema::deduceVarTypeFromInitializer(VarDecl *VDecl,
     // FIXME: Initialization should not be taking a mutable list of inits.
     SmallVector<Expr*, 8> InitsCopy(DeduceInits.begin(), DeduceInits.end());
     return DeduceTemplateSpecializationFromInitializer(TSI, Entity, Kind,
-                                                       InitsCopy, PL);
+                                                       InitsCopy);
   }
 
   if (DirectInit) {
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 4028b2d642b212..7036a88df6b90e 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -10573,7 +10573,7 @@ static bool isOrIsDerivedFromSpecializationOf(CXXRecordDecl *RD,
 
 QualType Sema::DeduceTemplateSpecializationFromInitializer(
     TypeSourceInfo *TSInfo, const InitializedEntity &Entity,
-    const InitializationKind &Kind, MultiExprArg Inits, ParenListExpr *PL) {
+    const InitializationKind &Kind, MultiExprArg Inits) {
   auto *DeducedTST = dyn_cast<DeducedTemplateSpecializationType>(
       TSInfo->getType()->getContainedDeducedType());
   assert(DeducedTST && "not a deduced template specialization type");
@@ -10804,9 +10804,12 @@ QualType Sema::DeduceTemplateSpecializationFromInitializer(
     if (getLangOpts().CPlusPlus20 && !HasAnyDeductionGuide) {
       if (ListInit && ListInit->getNumInits()) {
         SynthesizeAggrGuide(ListInit);
-      } else if (PL && PL->getNumExprs()) {
-        InitListExpr TempListInit(getASTContext(), PL->getLParenLoc(),
-                                  PL->exprs(), PL->getRParenLoc());
+      } else if (Inits.size()) { // parenthesized expression-list
+        // Inits are expressions inside the parentheses. We don't have
+        // the parentheses source locaitons, use the begin/end of Inits as the
+        // best heuristic.
+        InitListExpr TempListInit(getASTContext(), Inits.front()->getBeginLoc(),
+                                  Inits, Inits.back()->getEndLoc());
         SynthesizeAggrGuide(&TempListInit);
       }
     }
diff --git a/clang/test/SemaCXX/ctad.cpp b/clang/test/SemaCXX/ctad.cpp
new file mode 100644
index 00000000000000..10806f107b4ee9
--- /dev/null
+++ b/clang/test/SemaCXX/ctad.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s
+// expected-no-diagnostics
+
+namespace GH64347 {
+
+template<typename X, typename Y> struct A { X x; Y y;};
+void test() {
+   A(1, 2);
+   new A(1, 2);
+}
+
+template<A a>
+void f() { (void)a; }
+void k() {
+  // Test CTAD works for non-type template arguments.
+  f<A(0, 0)>();
+}
+
+} // namespace GH64347

PL->exprs(), PL->getRParenLoc());
} else if (Inits.size()) { // parenthesized expression-list
// Inits are expressions inside the parentheses. We don't have
// the parentheses source locaitons, use the begin/end of Inits as the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// the parentheses source locaitons, use the begin/end of Inits as the
// the parentheses source locations, use the begin/end of Inits as the

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

Why remove the support for ParenListExpr entirely?
Could you not fallback on looking at the init list expression only when ParenListExpr is null?

@hokein
Copy link
Collaborator Author

hokein commented Dec 18, 2023

Why remove the support for ParenListExpr entirely? Could you not fallback on looking at the init list expression only when ParenListExpr is null?

We could keep ParenListExpr, but I'm not sure the benefit (IMO, removing it seems like an API improvement, and make code simpler). The ParenListExpr is redundant with the existing Inits parameter to some degree, the only benefit to use it is for the precise () location information (as we deduct the template type here, I think it is fine to use a less-precise source location).

Looking at all usages (total 4) of DeduceTemplateSpecializationFromInitializer in clang, only 1 place passes the ParenListExpr.

@shafik
Copy link
Collaborator

shafik commented Dec 18, 2023

Can you please update your summary to explain the problem and how the fix addresses the problem.

This is usually what goes into the git log and we want those to be as descriptive as possible but also it help code reviewers to understand what they are reviewing quicker.

@shafik
Copy link
Collaborator

shafik commented Dec 18, 2023

Why remove the support for ParenListExpr entirely? Could you not fallback on looking at the init list expression only when ParenListExpr is null?

We could keep ParenListExpr, but I'm not sure the benefit (IMO, removing it seems like an API improvement, and make code simpler). The ParenListExpr is redundant with the existing Inits parameter to some degree, the only benefit to use it is for the precise () location information (as we deduct the template type here, I think it is fine to use a less-precise source location).

Looking at all usages (total 4) of DeduceTemplateSpecializationFromInitializer in clang, only 1 place passes the ParenListExpr.

As long as it does not effect the quality of the diagnostics then this makes sense.

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.

LGTM

@hokein
Copy link
Collaborator Author

hokein commented Dec 19, 2023

Can you please update your summary to explain the problem and how the fix addresses the problem.

This is usually what goes into the git log and we want those to be as descriptive as possible but also it help code reviewers to understand what they are reviewing quicker.

Thanks, I have updated the description with more details.

@hokein hokein force-pushed the ctad-explicit-type branch from 073f3d8 to 163a43a Compare December 19, 2023 08:48
@JohelEGP
Copy link

Thanks, I have updated the description with more details.

If it has to appear in the git log, it has to be added to the commit message.
GitHub is separate from Git, so updating the opening comment of the PR won't be visible through git.

@hokein hokein merged commit 32aa7d8 into llvm:main Dec 19, 2023
@hokein
Copy link
Collaborator Author

hokein commented Dec 19, 2023

Thanks, I have updated the description with more details.

If it has to appear in the git log, it has to be added to the commit message. GitHub is separate from Git, so updating the opening comment of the PR won't be visible through git.

GitHub's "squash and merge" button already does this for us.

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.

Aggregate CTAD of parenthesized prvalue fails
5 participants