[clang] [Clang] Ensure default arguments in friend declarations are only allowed in defining declarations to prevent multiple reachable declarations (PR #113777)

[Matheus Izvekov via cfe-commits]

================
@@ -4694,6 +4694,15 @@ bool Sema::InstantiateDefaultArgument(SourceLocation CallLoc, FunctionDecl *FD,
                                       ParmVarDecl *Param) {
   assert(Param->hasUninstantiatedDefaultArg());
 
+  // C++ [dcl.fct.default]p4
+  //   If a friend declaration D specifies a default argument expression, that
+  //   declaration shall be a definition and there shall be no other declaration
+  //   of the function or function template which is reachable from D or from
+  //   which D is reachable.
+  if (FD->getFriendObjectKind() != Decl::FOK_None &&
+      FD->getTemplateInstantiationPattern() == nullptr)
+    return true;
----------------
mizvekov wrote:

I think the `FD->getTemplateInstantiationPattern() == nullptr` check is a bit of a roundabout way of going about doing what the wording says. It doesn't mean the function has no reachable definition per se, that's only currently true for friend functions, and the consequence of a trick which we could improve in the future.

I am not saying necessarily to change that aspect of the fix, but it's worth calling it out.
Though in this case, the `FD->getFriendObjectKind() != Decl::FOK_None` check seems unnecessary.
By the way, `getTemplateInstantiationPattern` is not exactly trivial cost, so it might be worth moving this check to where the non-null return value would be used anyway.

Also, with this approach, we stop diagnosing substitution failures in the default argument, in these cases where the friend function decl has no definition. Since that was already an error, this is just worse error recovery, but it might be worth adding a test for it.

I think it's also worth calling this out in a comment with a FIXME so that we are aware this can be improved in the future.

https://github.com/llvm/llvm-project/pull/113777