Fix out of line Concept-comparisons of NestedNameSpecifiers #65993

erichkeane · 2023-09-11T18:30:16Z

As reported in GH65810, we don't properly collect ALL of the template parameters in a nested name specifier, and were only doing the 'inner level'.

This patch makes sure we collect from all levels.

Fixes: #65810

As reported in GH65810, we don't properly collect ALL of the template parameters in a nested name specifier, and were only doing the 'inner level'. This patch makes sure we collect from all levels. Fixes: llvm#65810

llvmbot · 2023-09-11T18:31:19Z

@llvm/pr-subscribers-clang

Changes

As reported in GH65810, we don't properly collect ALL of the template parameters in a nested name specifier, and were only doing the 'inner level'.

This patch makes sure we collect from all levels.

Fixes: #65810

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+11-7)
(modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+48)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cc8b2c3808933cb..872c32f59da96ad 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -218,6 +218,9 @@ Bug Fixes in This Version
   (`#65156 `_`)
 - Clang no longer considers the loss of ``__unaligned`` qualifier from objects as
   an invalid conversion during method function overload resolution.
+- Clang now properly handles out of line template specializations when there is
+  a non-template inner-class between the function and the class template
+  (`#65810 `_).
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 3b1731edec95237..832837556601663 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -231,14 +231,18 @@ Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD,
                                     MultiLevelTemplateArgumentList &Result) {
   if (!isa(FTD->getDeclContext())) {
     NestedNameSpecifier *NNS = FTD->getTemplatedDecl()->getQualifier();
-    const Type *Ty;
-    const TemplateSpecializationType *TSTy;
-    if (NNS && (Ty = NNS->getAsType()) &&
-        (TSTy = Ty->getAs()))
-      Result.addOuterTemplateArguments(const_cast(FTD),
-                                       TSTy->template_arguments(),
-                                       /*Final=*/false);
+
+    while (const Type *Ty = NNS ? NNS->getAsType() : nullptr) {
+
+      if (const auto *TSTy = Ty->getAs())
+        Result.addOuterTemplateArguments(
+            const_cast(FTD), TSTy->template_arguments(),
+            /*Final=*/false);
+
+      NNS = NNS->getPrefix();
+    }
   }
+
   return Response::ChangeDecl(FTD->getLexicalDeclContext());
 }
 
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 4688c28b489307f..f067c02ca48f584 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -418,3 +418,51 @@ template concept A = true;
 template struct X { A auto f(); };
 template A auto X::f() {}
 }
+
+namespace GH65810 {
+template
+concept TrivialConcept =
+requires(Param param) {
+  (void)param;
+};
+
+template 
+struct Base {
+  class InnerClass;
+};
+
+template 
+class Base::InnerClass {
+  template 
+    requires TrivialConcept
+    int func(Param param) const;
+};
+
+template 
+template 
+requires TrivialConcept
+int Base::InnerClass::func(Param param) const {
+  return 0;
+}
+
+template
+struct Outermost {
+  struct Middle {
+    template
+    struct Innermost {
+      template 
+        requires TrivialConcept
+        int func(Param param) const;
+    };
+  };
+};
+
+template 
+template 
+template 
+requires TrivialConcept
+int Outermost::Middle::Innermost::func(Param param) const {
+  return 0;
+}
+
+} // namespace GH65810

shafik · 2023-09-11T19:34:49Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

-                                       TSTy->template_arguments(),
-                                       /*Final=*/false);
+
+    while (const Type *Ty = NNS ? NNS->getAsType() : nullptr) {


This make sense, I was just looking at the AST: https://godbolt.org/z/P3brjcsMG

and I see that InnerClass has:

| |-NestedNameSpecifier TypeSpec 'Base<T>'

and I wondering what the prefix is there? Maybe the AST is not telling the whole story?

So that one you're seeing there is for the InnerClass on line 13, the NestedNameSpecifier is for 'everything else' outside of that declaration. So in THAT case, the Prefix of that one is nothing, since the Base<T> is the entirety of the thing.

However, that AST doesn't show the NNS of func on line 22, for some reason we're not dumping that. In THAT case, the NNS is Base<T>::InnerClass. At that point, the InnerClass doesn't have template arguments (since it isn't a template!), but its Prefix is just the Base<T>, which has them (which is why/how this loop works).

It does not look like there is a simple way of getting like w/ CXXRecordDecl so some work would be involved.

I don't understand what you mean? TagDecl (IIRC?) has a getQualifier() to get the appropriate NestedNameSpecifier. I believe our NNS tree is correct the way I've implemented it above, with the exception of the issue I posted about a moment ago.

Yeah, I just realized I was looking at the code a moment ago FTD->getTemplatedDecl()->getQualifier()

erichkeane · 2023-09-11T19:44:05Z

Huh, blarg, build-bot found an issue with my patch, curiously in the same file I was editing! I'll look into it. I at least reproduce it, which is nice...

erichkeane · 2023-09-11T21:04:16Z

So the latest patch doesn't FIX it, it has a problem with:

template<typename T> concept Concept = true;

template<int>
struct S { 
struct Inner0 {
struct Inner1 {
template<Concept C>
static constexpr int method();
};
};
};

template<>
template<Concept C>
constexpr int S<1>::Inner0::Inner1::method() { return 0;}

The problem/special case here is the template<>, which causes there to be no level of 'template parameters' when filling in the Depths of the method. HOWEVER, our NestedNameSpecifier list ends up finding the 1 there (for obvious reasons), so the replacement is wrong.

I don't have a feeling of how to get those, I thought the NNS was a good analog, but it is clearly not. I suspect this might be the source of other similar/awkward issues elsewhere.

At the moment, I'm done with this for at least the night, so if someone else wants to take a look, they are encouraged to share their' findings.

erichkeane · 2023-09-11T22:31:14Z

Alright, I think I got it this time, we'll see how the buildbots treat it, but it passes locally for me.

cor3ntin

You obviously have a better grasp of that part of the code than I do, but the patch looks sensible to me. Thanks!

clang/docs/ReleaseNotes.rst

rupprecht · 2023-09-13T17:25:41Z

Downstream, this is causing a funny build error in c++20 mode:

__chrono/duration.h:404:12: error: missing '#include <__compare/three_way_comparable.h>'; default argument of 'three_way_comparable' must be defined before it is used
  404 |   requires three_way_comparable<common_type_t<_Rep1, _Rep2>>
      |            ^
__chrono/duration.h:406:16: note: while calculating associated constraint of template '' here
  406 | constexpr auto operator<=>(const duration<_Rep1, _Period1>& __lhs,
      |                ^
__compare/three_way_comparable.h:34:34: note: default argument declared here is not reachable
   34 | template<class _Tp, class _Cat = partial_ordering>
      |                                  ^

It doesn't seem to fail on the public libcxx buildbots, so I'm not sure what's different about our setup that triggers this failure. (cc @ilya-biryukov)

ilya-biryukov · 2023-09-13T17:43:20Z

I couldn't reproduce the error with module-enabled builds of libc++ either, but internally we are building the libc++ module from a generated module map file (and different) rather than the one distributed with libc++.

It seems that Clang has an actual bug with C++20 modules exposing the same problem: #66255. It seems that it's been there for a long time. I believe the patch somehow exposes it. @erichkeane maybe this looks familiar?

erichkeane · 2023-09-13T17:44:37Z

This doesn't, I was really hoping it did for you :) This is a really strange one, it almost seems like it doesnt' realize it is IN the header or something?

As reported in GH65810, we don't properly collect ALL of the template parameters in a nested name specifier, and were only doing the 'inner level'. This patch makes sure we collect from all levels. Fixes: llvm#65810

Fix out of line Concept-comparisons of NestedNameSpecifiers

52bcb55

As reported in GH65810, we don't properly collect ALL of the template parameters in a nested name specifier, and were only doing the 'inner level'. This patch makes sure we collect from all levels. Fixes: llvm#65810

erichkeane requested a review from a team as a code owner September 11, 2023 18:30

erichkeane requested review from cor3ntin and alexander-shaposhnikov September 11, 2023 18:30

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 11, 2023

erichkeane mentioned this pull request Sep 11, 2023

clang: concept checking bug in out-of-line definitions of inner class member functions #65810

Closed

shafik reviewed Sep 11, 2023

View reviewed changes

Make sure we add injected template args from the FTD

7258e88

Only add arguments when they participate in depth

a5bf5e5

cor3ntin approved these changes Sep 12, 2023

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Move release note to C++ section, move period.

2b86194

erichkeane merged commit bfddbda into llvm:main Sep 12, 2023
1 of 2 checks passed

vzakhari mentioned this pull request Sep 12, 2023

internap proc trampolines #66156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix out of line Concept-comparisons of NestedNameSpecifiers #65993

Fix out of line Concept-comparisons of NestedNameSpecifiers #65993

erichkeane commented Sep 11, 2023

llvmbot commented Sep 11, 2023

Fixes: #65810

shafik Sep 11, 2023

erichkeane Sep 11, 2023

shafik Sep 11, 2023

erichkeane Sep 11, 2023

shafik Sep 11, 2023

erichkeane commented Sep 11, 2023

erichkeane commented Sep 11, 2023

erichkeane commented Sep 11, 2023

cor3ntin left a comment

rupprecht commented Sep 13, 2023

ilya-biryukov commented Sep 13, 2023

erichkeane commented Sep 13, 2023

Fix out of line Concept-comparisons of NestedNameSpecifiers #65993

Fix out of line Concept-comparisons of NestedNameSpecifiers #65993

Conversation

erichkeane commented Sep 11, 2023

llvmbot commented Sep 11, 2023

Fixes: #65810

shafik Sep 11, 2023

Choose a reason for hiding this comment

erichkeane Sep 11, 2023

Choose a reason for hiding this comment

shafik Sep 11, 2023

Choose a reason for hiding this comment

erichkeane Sep 11, 2023

Choose a reason for hiding this comment

shafik Sep 11, 2023

Choose a reason for hiding this comment

erichkeane commented Sep 11, 2023

erichkeane commented Sep 11, 2023

erichkeane commented Sep 11, 2023

cor3ntin left a comment

Choose a reason for hiding this comment

rupprecht commented Sep 13, 2023

ilya-biryukov commented Sep 13, 2023

erichkeane commented Sep 13, 2023