[Clang] Fix finding instantiated decls for class template specializations during instantiation

[yuxuanchen1997]

This change aims to fix #70375

It appears to me that the logic here should be handling specializations in general, not just partial specialization. It also seems that both the comment before the block and the isInstantiationOf(ClassTemplate, SpecTemplate) below agree with my judgement.

The issue might just be a mistake that someone mistaken specialization as a special case of partial specializations, while it's actually the other way around.

Needs some experts to comment here if this is the right fix.

The code that caused clang ICE is added as a test case.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Yuxuan Chen (yuxuanchen1997)

Changes

This change aims to fix #70375

It appears to me that the logic here should be handling specializations in general. Not just partial specialization. It also seems that both the comment before the block and the isInstantiationOf(ClassTemplate, SpecTemplate) below agree with my judgement.

The issue might just be a mistake that someone mistaken specialization as a special case of partial specializations, while it's actually the other way around.

Needs some experts to comment here if this is the right fix.

The code that caused clang ICE is added as a test case.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+3-3)
• (added) clang/test/SemaCXX/member-template-specialization.cpp (+33)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 011356e08a04297..07b3488a21e670b 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -6211,9 +6211,9 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D,
     ClassTemplateDecl *ClassTemplate = Record->getDescribedClassTemplate();
     if (ClassTemplate)
       ClassTemplate = ClassTemplate->getCanonicalDecl();
-    else if (ClassTemplatePartialSpecializationDecl *PartialSpec
-               = dyn_cast<ClassTemplatePartialSpecializationDecl>(Record))
-      ClassTemplate = PartialSpec->getSpecializedTemplate()->getCanonicalDecl();
+    else if (ClassTemplateSpecializationDecl *Spec =
+                 dyn_cast<ClassTemplateSpecializationDecl>(Record))
+      ClassTemplate = Spec->getSpecializedTemplate()->getCanonicalDecl();
 
     // Walk the current context to find either the record or an instantiation of
     // it.
diff --git a/clang/test/SemaCXX/member-template-specialization.cpp b/clang/test/SemaCXX/member-template-specialization.cpp
new file mode 100644
index 000000000000000..29d46ec9c1e44fc
--- /dev/null
+++ b/clang/test/SemaCXX/member-template-specialization.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+// expected-no-diagnostics
+
+// Verify that the inner template specialization can be found
+
+template <typename Ty>
+struct S {
+  static void bar() {
+    Ty t;
+    t.foo();
+  }
+
+  static void take(Ty&) {}
+};
+
+template <typename P>
+struct Outer {
+  template <typename C>
+  struct Inner;
+
+  using U = S<Inner<P>>;
+
+  template <>
+  struct Inner<void> {
+    void foo() {
+      U::take(*this);
+    }
+  };
+};
+
+int main() {
+  Outer<void>::U::bar();
+}

[yuxuanchen1997]

@AaronBallman , mind providing some feedback on this patch? I think this can solve #70375

[shafik]

This makes some sense but I am not familiar enough with this area.

[shafik]

Normally we put lone tests for a specific bug report in its own file e.g. GH70735.cpp for this case or we find a test that covers a similar area and put the test in a namespace named after the issue e.g. GH70735.

[yuxuanchen1997]

Can it be more elaborate like GH70735-member-template-specialization.cpp?

[erichkeane]

Typically we find a similar test file and put it there, just wrapped in a namespace for the github issue.

[yuxuanchen1997]

gotcha, will do.

[erichkeane]

I don't have a great feel if this is the right fix, but if it doesn't break anything in the tests, and does fix something, this is likely acceptable for now.

This DOES need a release note, and as Shafik says: this should likely be placed in an existing/related test file in a namespace GH70735.

[yuxuanchen1997]

Thanks for the review, @erichkeane. I am wondering what you mean by this needs "release note"?

[erichkeane]

Thanks for the review, @erichkeane. I am wondering what you mean by this needs "release note"?

Ah, yes! See docs/ReleaseNotes.rst. For each new bug fix (and most patches!) we require an entry into our release notes document.

[bcardosolopes]

I'm not 100% confident here but the fix makes sense and seems good (nice testcase!).

[yuxuanchen1997]

I have made the suggested changes and this is ready for another round of review.