Fix merging of member-like constrained friends across modules.

[rsmith]

Summary

When a friend declaration has a requires-clause, and either it's a
non-template function or it's a function template whose requires-clause
depends on an enclosing template parameter, it is member-like for the
purpose of redeclaration checking. Specifically, the lexically enclosing
class becomes part of its signature, so it can only be redeclared by
another declaration within the same class.

Previously we supported this rule when parsing declarations, but not
when merging them across modules.

[rsmith]

• Merge branch 'main' into merge-constrained-friends

[rsmith]

Hm, looks like rGdb1b827ecfed6 landed the actual functioal change here, making this just a no-functional-change refactoring plus an added test, so I'm going to go ahead and land it.

[erichkeane]

In D147068#4235155, @rsmith wrote:

Hm, looks like rGdb1b827ecfed6 landed the actual functioal change here, making this just a no-functional-change refactoring plus an added test, so I'm going to go ahead and land it.

Makes sense to me, thanks!

[rsmith]

Refactoring and test landed in rGa07abe27b4d1d39ebb940a7f4e6235302444cbf0.

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fix crash on invalid code when looking up a destructor in a templated class
   inside a namespace.
   (`#59446 <https://github.com/llvm/llvm-project/issues/59446>`_)
 - Fix crash when evaluating consteval constructor of derived class whose base
   has more than one field.
   (`#60166 <https://github.com/llvm/llvm-project/issues/60166>`_)
+- Fix incorrect merging of member-like constrained friends across modules, as
+  described in C++ [temp.friend]/9.
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed

clang/include/clang/AST/ASTContext.h

public:
   /// nested-name-specifier, which itself contains all canonical
   /// types, values, and templates.
   TemplateName getCanonicalTemplateName(const TemplateName &Name) const;
 
   /// Determine whether the given template names refer to the same
   /// template.
   bool hasSameTemplateName(const TemplateName &X, const TemplateName &Y) const;
 
-  /// Determine whether two Friend functions are different because constraints
-  /// that refer to an enclosing template, according to [temp.friend] p9.
-  bool FriendsDifferByConstraints(const FunctionDecl *X,
-                                  const FunctionDecl *Y) const;
-
   /// Determine whether the two declarations refer to the same entity.
   bool isSameEntity(const NamedDecl *X, const NamedDecl *Y) const;
 
   /// Determine whether two template parameter lists are similar enough
   /// that they may be used in declarations of the same template.
   bool isSameTemplateParameterList(const TemplateParameterList *X,
                                    const TemplateParameterList *Y) const;
 

clang/include/clang/AST/Decl.h

public:
   }
   // Indicates this function is a constrained friend, where the constraint
   // refers to an enclosing template for hte purposes of [temp.friend]p9.
   bool FriendConstraintRefersToEnclosingTemplate() const {
     return getCanonicalDecl()
         ->FunctionDeclBits.FriendConstraintRefersToEnclosingTemplate;
   }
 
+  /// Determine whether a function is a friend function that cannot be
+  /// redeclared outside of its class, per C++ [temp.friend]p9.
+  bool isMemberLikeConstrainedFriend() const;
+
   /// Gets the kind of multiversioning attribute this declaration has. Note that
   /// this can return a value even if the function is not multiversion, such as
   /// the case of 'target'.
   MultiVersionKind getMultiVersionKind() const;
 
 
   /// True if this function is a multiversioned dispatch function as a part of
   /// the cpu_specific/cpu_dispatch functionality.

clang/lib/AST/ASTContext.cpp

for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
     // Return false if any of the enable_if expressions of A and B are
     // different.
     if (Cand1ID != Cand2ID)
       return false;
   }
   return true;
 }
 
-bool ASTContext::FriendsDifferByConstraints(const FunctionDecl *X,
-                                            const FunctionDecl *Y) const {
-  // If these aren't friends, then they aren't friends that differ by
-  // constraints.
-  if (!X->getFriendObjectKind() || !Y->getFriendObjectKind())
-    return false;
-
-  // If the two functions share lexical declaration context, they are not in
-  // separate instantations, and thus in the same scope.
-  if (X->getLexicalDeclContext() == Y->getLexicalDeclContext())
-    return false;
-
-  if (!X->getDescribedFunctionTemplate()) {
-    assert(!Y->getDescribedFunctionTemplate() &&
-           "How would these be the same if they aren't both templates?");
-
-    // If these friends don't have constraints, they aren't constrained, and
-    // thus don't fall under temp.friend p9. Else the simple presence of a
-    // constraint makes them unique.
-    return X->getTrailingRequiresClause();
-  }
-
-  return X->FriendConstraintRefersToEnclosingTemplate();
-}
-
 bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
   if (X == Y)
     return true;
 
   if (X->getDeclName() != Y->getDeclName())
     return false;
 
   // Must be in the same context.
if (FuncX->isMultiVersion()) {
       const auto *TAX = FuncX->getAttr<TargetAttr>();
       const auto *TAY = FuncY->getAttr<TargetAttr>();
       assert(TAX && TAY && "Multiversion Function without target attribute");
 
       if (TAX->getFeaturesStr() != TAY->getFeaturesStr())
         return false;
     }
 
+    // Per C++20 [temp.over.link]/4, friends in different classes are sometimes
+    // not the same entity if they are constrained.
+    if ((FuncX->isMemberLikeConstrainedFriend() ||
+         FuncY->isMemberLikeConstrainedFriend()) &&
+        !FuncX->getLexicalDeclContext()->Equals(
+            FuncY->getLexicalDeclContext())) {
+      return false;
+    }
+
     if (!isSameConstraintExpr(FuncX->getTrailingRequiresClause(),
                               FuncY->getTrailingRequiresClause()))
       return false;
 
     auto GetTypeAsWritten = [](const FunctionDecl *FD) {
       // Map to the first declaration that we've already merged into this one.
       // The TSI of redeclarations might not match (due to calling conventions
       // being inherited onto the type but not the TSI), but the TSI type of

clang/lib/AST/Decl.cpp

if (hasAttr<NoReturnAttr>() || hasAttr<CXX11NoReturnAttr>() ||
     return true;
 
   if (auto *FnTy = getType()->getAs<FunctionType>())
     return FnTy->getNoReturnAttr();
 
   return false;
 }
 
+bool FunctionDecl::isMemberLikeConstrainedFriend() const {
+  // C++20 [temp.friend]p9:
+  //   A non-template friend declaration with a requires-clause [or]
+  //   a friend function template with a constraint that depends on a template
+  //   parameter from an enclosing template [...] does not declare the same
+  //   function or function template as a declaration in any other scope.
+
+  // If this isn't a friend then it's not a member-like constrained friend.
+  if (!getFriendObjectKind()) {
+    return false;
+  }
+
+  if (!getDescribedFunctionTemplate()) {
+    // If these friends don't have constraints, they aren't constrained, and
+    // thus don't fall under temp.friend p9. Else the simple presence of a
+    // constraint makes them unique.
+    return getTrailingRequiresClause();
+  }
+
+  return FriendConstraintRefersToEnclosingTemplate();
+}
 
 MultiVersionKind FunctionDecl::getMultiVersionKind() const {
   if (hasAttr<TargetAttr>())
     return MultiVersionKind::Target;
   if (hasAttr<TargetVersionAttr>())
     return MultiVersionKind::TargetVersion;
   if (hasAttr<CPUDispatchAttr>())
     return MultiVersionKind::CPUDispatch;

clang/lib/Sema/SemaOverload.cpp

if (FunctionDecl *OldF = OldD->getAsFunction()) {
           HideUsingShadowDecl(S, cast<UsingShadowDecl>(*I));
           continue;
         }
 
         if (!isa<FunctionTemplateDecl>(OldD) &&
             !shouldLinkPossiblyHiddenDecl(*I, New))
           continue;
 
-        // C++20 [temp.friend] p9: A non-template friend declaration with a
-        // requires-clause shall be a definition.  A friend function template
-        // with a constraint that depends on a template parameter from an
-        // enclosing template shall be a definition.  Such a constrained friend
-        // function or function template declaration does not declare the same
-        // function or function template as a declaration in any other scope.
-        if (Context.FriendsDifferByConstraints(OldF, New))
-          continue;
-
         Match = *I;
         return Ovl_Match;
       }
 
       // Builtins that have custom typechecking or have a reference should
       // not be overloadable or redeclarable.
       if (!getASTContext().canBuiltinBeRedeclared(OldF)) {
         Match = *I;
bool Sema::IsOverload(FunctionDecl *New, FunctionDecl *Old,
   // parameters (C++ 1.3.10), which includes the presence or absence
   // of the ellipsis; see C++ DR 357).
   if (OldQType != NewQType &&
       (OldType->getNumParams() != NewType->getNumParams() ||
        OldType->isVariadic() != NewType->isVariadic() ||
        !FunctionParamTypesAreEqual(OldType, NewType)))
     return true;
 
+  // For member-like friends, the enclosing class is part of the signature.
+  if ((New->isMemberLikeConstrainedFriend() ||
+       Old->isMemberLikeConstrainedFriend()) &&
+      !New->getLexicalDeclContext()->Equals(Old->getLexicalDeclContext()))
+    return true;
+
   if (NewTemplate) {
     // C++ [temp.over.link]p4:
     //   The signature of a function template consists of its function
     //   signature, its return type and its template parameter list. The names
     //   of the template parameters are significant only for establishing the
     //   relationship between the template parameters and the rest of the
     //   signature.
     //

clang/test/Modules/merge-constrained-friends.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++2b %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++2b %t/Use.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- A.cppm
+module;
+export module A;
+
+struct B {};
+
+export template<int N> struct A : B {
+  friend constexpr const int *f(B) requires true {
+    static constexpr int result = N;
+    return &result;
+  }
+
+  template<int M>
+  friend constexpr const int *g(B) requires (M >= 0) && (N >= 0) {
+    static constexpr int result = M * 10 + N;
+    return &result;
+  }
+};
+
+export inline A<1> a1;
+export inline A<2> a2;
+export inline A<3> a3;
+
+static_assert(f(a1) != f(a2) && f(a2) != f(a3));
+static_assert(g<1>(a1) != g<1>(a2) && g<1>(a2) != g<1>(a3));
+
+static_assert(*f(a1) == 1);
+static_assert(*f(a2) == 2);
+static_assert(*f(a3) == 3);
+
+static_assert(*g<4>(a1) == 41);
+static_assert(*g<5>(a2) == 52);
+static_assert(*g<6>(a3) == 63);
+
+//--- Use.cpp
+// expected-no-diagnostics
+import A;
+
+// Try some instantiations we tried before and some we didn't.
+static_assert(f(a1) != f(a2) && f(a2) != f(a3));
+static_assert(g<1>(a1) != g<1>(a2) && g<1>(a2) != g<1>(a3));
+static_assert(g<2>(a1) != g<2>(a2) && g<2>(a2) != g<2>(a3));
+
+A<4> a4;
+static_assert(f(a1) != f(a4) && f(a2) != f(a4) && f(a3) != f(a4));
+static_assert(g<3>(a1) != g<3>(a4));
+
+static_assert(*f(a1) == 1);
+static_assert(*f(a2) == 2);
+static_assert(*f(a3) == 3);
+static_assert(*f(a4) == 4);
+
+static_assert(*g<4>(a1) == 41);
+static_assert(*g<5>(a2) == 52);
+static_assert(*g<6>(a3) == 63);
+
+static_assert(*g<7>(a1) == 71);
+static_assert(*g<8>(a4) == 84);