Skip to content

Reland [Clang] skip default argument instantiation for non-defining friend declarations to meet [dcl.fct.default] p4 #115487

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
merged 20 commits into from
Dec 18, 2024

Conversation

a-tarasyuk
Copy link
Member

@a-tarasyuk a-tarasyuk commented Nov 8, 2024

This fixes a crash when instantiating default arguments for templated friend function declarations which lack a definition.
There are implementation limits which prevents us from finding the pattern for such functions, and this causes difficulties
setting up the instantiation scope for the function parameters.

This patch skips instantiating the default argument in these cases, which causes a minor regression in error recovery, but otherwise avoids the crash.

The previous attempt #113777 accidentally skipped all default argument constructions, causing some regressions. This patch resolves that by moving the guard to InstantiateDefaultArgument() where the handling of templates takes place.

Fixes #113324

@a-tarasyuk a-tarasyuk marked this pull request as ready for review November 8, 2024 18:31
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #113324

#113777 (comment)
#113777 (comment)


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+11)
  • (modified) clang/test/CXX/temp/temp.res/p4.cpp (+43)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d4bf05651a63eb..a3bce0e77581fb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -641,6 +641,8 @@ Bug Fixes to C++ Support
   an implicitly instantiated class template specialization. (#GH51051)
 - Fixed an assertion failure caused by invalid enum forward declarations. (#GH112208)
 - Name independent data members were not correctly initialized from default member initializers. (#GH114069)
+- Fixed an assertion failure caused by invalid default argument substitutions in non-defining
+  friend declarations. (#GH113324).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 5a001843e2ba46..0bbab95001ad8e 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4694,6 +4694,17 @@ bool Sema::InstantiateDefaultArgument(SourceLocation CallLoc, FunctionDecl *FD,
                                       ParmVarDecl *Param) {
   assert(Param->hasUninstantiatedDefaultArg());
 
+  // FIXME: We don't track member specialization info for non-defining
+  // friend declarations, so we will not be able to later find the function
+  // pattern. As a workaround, don't instantiate the default argument in this
+  // case. This is correct per wording and only an error recovery issue, as per
+  // [dcl.fct.default]p4:
+  //   if a friend declaration D specifies a default argument expression,
+  //   that declaration shall be a definition.
+  if (FD->getFriendObjectKind() != Decl::FOK_None &&
+      !FD->getTemplateInstantiationPattern())
+    return true;
+
   // Instantiate the expression.
   //
   // FIXME: Pass in a correct Pattern argument, otherwise
diff --git a/clang/test/CXX/temp/temp.res/p4.cpp b/clang/test/CXX/temp/temp.res/p4.cpp
index f54d8649f5da88..cf6c45b4c351c5 100644
--- a/clang/test/CXX/temp/temp.res/p4.cpp
+++ b/clang/test/CXX/temp/temp.res/p4.cpp
@@ -185,3 +185,46 @@ template<typename T> struct S {
   friend void X::f(T::type);
 };
 }
+
+namespace GH113324 {
+template <typename = int> struct S1 {
+  friend void f1(S1, int = 0); // expected-error {{friend declaration specifying a default argument must be a definition}}
+  friend void f2(S1 a, S1 = decltype(a){}); // expected-error {{friend declaration specifying a default argument must be a definition}}
+};
+
+template <class T> using alias = int;
+template <typename T> struct S2 {
+  // FIXME: We miss diagnosing the default argument instantiation failure
+  // (forming reference to void)
+  friend void f3(S2, int a = alias<T &>(1)); // expected-error {{friend declaration specifying a default argument must be a definition}}
+};
+
+struct S3 {
+  friend void f4(S3, int = 42) { }
+};
+
+template <bool, class> using __enable_if_t = int;
+template <int v> struct S4 {
+  static const int value = v;
+};
+struct S5 {
+  template <__enable_if_t<S4<true>::value, int> = 0>
+  S5(const char *);
+};
+struct S6 {
+  template <typename a, typename b>
+  friend void f5(int, S6, a, b, S5 = "") { }
+};
+
+void test() {
+  f1(S1<>{});
+  f2(S1<>{});
+  f3(S2<void>());
+
+  S3 s3;
+  f4(s3);
+
+  S6 s6;
+  auto result = f5(0, s6, [] {}, [] {}); // expected-error {{variable has incomplete type 'void}}
+}
+} // namespace GH113324

@a-tarasyuk a-tarasyuk changed the title [Clang] skip default argument instantiation for non-defining friend declarations to meet [dcl.fct.default] p4 Reland [Clang] skip default argument instantiation for non-defining friend declarations to meet [dcl.fct.default] p4 Nov 12, 2024
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.

LGTM modulo 1 nit.
@mizvekov are you happy with it?

@zyn0217 zyn0217 requested a review from mizvekov November 21, 2024 02:00
Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

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

Yeah, this LGTM, thanks again.

@a-tarasyuk a-tarasyuk merged commit 9daf10f into llvm:main Dec 18, 2024
9 checks passed
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] clang trunk crashes at clang::FunctionDecl::getNumParams()
6 participants