[Clang][Sema] fix noexecpt mismatch of friend declaration

[jcsxky]

Creating a new local scope without combining outer one will makes some instantiated template parameters can't be found in instantiate a friend function. This will lead to the mismatch with its definition as they have template parameter with different depths. This patch fix this issue by adding instantiated template parameters to current scope. This will make clang find the correct instantiated template parameters and make the depth of template parameter correct.
Fix issue #101330

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Creating a new local scope without combining outer one will makes some instantiated template parameters can't be found in instantiate a friend function. This will lead to the mismatch with its definition. This patch fix this issue by adding instantiated template parameters to current scope.
Fix issue #101330

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+15)
• (added) clang/test/SemaCXX/pr101330.cpp (+16)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 829a759ca4a0b..7b759cfd17119 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -192,6 +192,8 @@ Bug Fixes to C++ Support
 - Clang now preserves the unexpanded flag in a lambda transform used for pack expansion. (#GH56852), (#GH85667),
   (#GH99877).
 - Fixed a bug when diagnosing ambiguous explicit specializations of constrained member functions.
+- Fix mismatch of noexecpt in friend function declaration by copying template instantiated parameters to
+  current local scope.(#GH101330).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index f93cd113988ae..2ffea99a49378 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4698,7 +4698,22 @@ void Sema::InstantiateExceptionSpec(SourceLocation PointOfInstantiation,
   // Enter the scope of this instantiation. We don't use
   // PushDeclContext because we don't have a scope.
   Sema::ContextRAII savedContext(*this, Decl);
+
+  FunctionDecl *Source = Proto->getExtProtoInfo().ExceptionSpec.SourceTemplate;
+  FunctionTemplateDecl *SourceTemplate = Source->getDescribedFunctionTemplate();
+  llvm::SmallDenseMap<clang::Decl *, clang::Decl *> InstTemplateParams;
+  if (CurrentInstantiationScope && SourceTemplate)
+    if (TemplateParameterList *TPL = SourceTemplate->getTemplateParameters())
+      for (NamedDecl *TemplateParam : *TPL)
+        if (auto *Found =
+                CurrentInstantiationScope->findInstantiationOf(TemplateParam))
+          if (auto *InstTemplateParam = Found->dyn_cast<clang::Decl *>())
+            InstTemplateParams[TemplateParam] = InstTemplateParam;
+
   LocalInstantiationScope Scope(*this);
+  for (auto [TemplateParam, InstTemplateParam] : InstTemplateParams) {
+    Scope.InstantiatedLocal(TemplateParam, InstTemplateParam);
+  }
 
   MultiLevelTemplateArgumentList TemplateArgs =
       getTemplateInstantiationArgs(Decl, Decl->getLexicalDeclContext(),
diff --git a/clang/test/SemaCXX/pr101330.cpp b/clang/test/SemaCXX/pr101330.cpp
new file mode 100644
index 0000000000000..3d42bd5635562
--- /dev/null
+++ b/clang/test/SemaCXX/pr101330.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// expected-no-diagnostics
+
+template <typename T>
+struct C {
+    template <int N, typename U>
+    friend void func(const C<U> &m) noexcept(N == 0);
+};
+
+template <int N, typename U>
+void func(const C<U> &m) noexcept(N == 0) {}
+
+int main() {
+    C<int> t;
+    return 0;
+}

[sdkrystian]

I don't think this is the right approach. I think we should compare the operands of the noexcept-specifier the same way we compare constraints (i.e. substitute so all references to template parameters have the correct depth and then check whether the substituted expressions are equivalent). WDYT @mizvekov?

[jcsxky]

substitute so all references to template parameters have the correct depth

@sdkrystian Agree! The underlying issue is the incorrect depth makes the comparison of the operands of the noexcept-specifier failed. because clang can't find instantiated template parameter declarations in local scope. This approach adds the instantiated template parameters to current scope to make clang find it and we get the correct depth.

[jcsxky]

@sdkrystian Could you please take another look at this patch? The approach does what you said before.
When checking exception specific equivalence of the two functions, we do instantiation and substitute all the template parameters with instantiated ones. But we can't find them as we create a new LocalInstantiationScope. So these instantiated parameters should be added to current new LocalInstantiationScope(like what addInstantiatedParametersToScope does in the following code) to make sure they can be found.
Consider the test case:

template <typename T>
struct C {
    template <int N>
    friend void func() noexcept(N == 0);
};

template <int N>
void func() noexcept(N == 0) {}

int main() {
    C<int> t;
    return 0;
}

Without this patch, Sema::FindInstantiatedDecl returns original template parameter in transforming exception specific when trying to find instantiated parameter of N. So we use the original parameter(N) whose depth is 1 in the primary template and this is incorrect and make the comparison failed since it should be 0 in the template specialization. Adding instantiated parameters to current LocalInstantiationScope makes Sema::FindInstantiatedDecl returns the correct instantiated parameter and it works.

[jcsxky]

gently ping~

[ericniebler]

nudge. i'd love to see #101330 fixed.

[sdkrystian]

@ericniebler Once I merge #106585, I have a follow up patch ready that will fix #101330.

I'm at cppcon right now but I'll be back Thursday.

[cor3ntin]

I don't think this is handling the pack case..
Do we have a generic way to copy instantiated template params? @zyn0217 @mizvekov @Sirraide
We do similar things in a bunch of places (including when checking constraints)

[zyn0217]

Not that I’m aware of. I think we can wait for @sdkrystian‘s refactoring patch of MLTAL which might make such things easier.

[zyn0217]

... including when checking constraints ...

This might not be the case for template parameters, as we (almost always, in my recollection) would have transformed the associated template parameters before calling the constraint matching function, so...

[sdkrystian]

Yeah, we should wait until I reland #106585 and then we can fix this. What we can then do is normalize the exception specification the same way we normalize constraints prior to comparing them.

[zyn0217]

... the same way we normalize constraints prior to comparing them.

Which means we don't need an individual instantiation scope for the noexcept specification thereafter, so the question here could be resolved naturally I presume?