Diagnose friend function template redefinitions

[sepavloff]

Summary

Friend function template becomes available if enclosing class template is
instantiated. Now the check for redefinition do not handle correctly such
function template and some redefinitions are not diagnosed. The reason is
the friend function templates in the instantiated class do not have
bodies, as they are not used yet and the search for redefinition does not
take into account.

The change modifies redefinition check so that it can find the friend
function template definitions in instantiated classes. The check is based
on the new function, 'FunctionTemplateDecl::isDefined'. In addition to the
checks made by 'isThisDeclarationADefinition' it checks if the given
declaration has uninstantiated body.

[sepavloff]

Make more correct template function definition lookup

Lookup is made recursive to cover more cases.

[sepavloff]

Updated comments, NFC otherwise.

[rsmith]

The model that the C++ standard seems to have settled on is that a friend function declaration that's instantiated from a definition is considered to be a definition, even if we've not instantiated its body yet. I think we should try to directly implement that rule.

lib/AST/DeclTemplate.cpp

292–311 ↗

(On Diff #74218)

It seems to me that we're getting something fundamentally wrong in the way we model friend function definitions where we've instantiated the declaration but not the definition. Here's a case we get wrong on the non-function-template side:

template<typename T> struct X { friend void f() {} };
X<int> xi;
void f() {}

Line 3 should be ill-formed here due to redefinition, but we don't detect that because we don't believe that the declaration we instantiated on line 2 is a definition. That seems to be the heart of the problem.

Instead of adding this function, can you try changing FunctionDecl::isThisDeclarationADefinition to contain the above logic (that is: if this function is a friend that was instantiated from a definition, then it's a definition even if we don't have a body yet)?

lib/Sema/SemaDecl.cpp

8856 ↗

(On Diff #74218)

This doesn't look right; the OldTemplateDecl might be FOK_None but could still have a definition as a friend.

I'm not convinced this is the right place for this check. There are two different cases here:

1. The redefinition error is introduced by parsing a definition in the source code. That should already be handled by ActOnStartOfFunctionDef.
2. The redefinition error is introduced by instantiating a friend function template declaration (that has a definition).

I think check (2) belongs in the template instantiation code rather than here. In fact, at the end of TemplateDeclInstantiator::VisitFunctionDecl, there is an attempt to enforce the relevant rule; it just doesn't get all the cases right.

8857–8863 ↗

(On Diff #74218)

You should use CheckForFunctionRedefinition to catch this.

[sepavloff]

lib/AST/DeclTemplate.cpp

292–311 ↗

(On Diff #74218)

The code snippet miscompiles due to another problem, it is related to friend functions, not function templates, and is addressed by D30170.

Instead of adding this function, can you try changing FunctionDecl::isThisDeclarationADefinition to contain the above logic

FunctionDecl::isThisDeclarationADefinition is used in many places, in some it should not consider uninstantiated bodies. Making special function for such checks makes the fix more compact and reduces the risk to break existing code. For function declarations it is isThisDeclarationAnOdrDefinition introduced in D30170, for function templates isDefined is created in the updated version of this patch.

lib/Sema/SemaDecl.cpp

8856 ↗

(On Diff #74218)

Indeed, marking both FunctionTemplateDecl and corresponding FunctionDecl as friends was enough to make TemplateDeclInstantiator::VisitFunctionDecl work properly.

As for the case (1), the check was moved to CheckForFunctionRedefinition, which is eventually called from ActOnStartOfFunctionDef.

[sepavloff]

Updated patch

[sepavloff]

Aligned implementation with D30170.

[sepavloff]

Simplified patch

[sepavloff]

Rebased

[sepavloff]

Updated patch

• Rebased relative to recent ToT
• Removed the change in FunctionDecl::getTemplateInstantiationPattern(), as it is not necessary for error detection,
• Added test for use in module.

[sepavloff]

Other compilers successfully recognize errors (checked using https://godbolt.org/). For instance, the code:

template<typename T> inline void func_35(T *x);
template<typename T1>
struct C35a {
  template<typename T> friend void func_35(T *x) {}
};
template<typename T1>
struct C35b {
  template<typename T> friend void func_35(T *x) {}
};
C35a<int> v35a;
C35b<int> v35b;

is rejected by MSVC 19:

example.cpp
<source>(7): error C2995: 'void func_37(T *)': function template has already been defined
<source>(1): note: see declaration of 'func_37'
Microsoft (R) C/C++ Optimizing Compiler Version 19.10.25017 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.
Compiler returned: 2

by gcc 7.3:

<source>:7:27: error: redefinition of 'template<class T> void func_37(T*)'
 template<typename T> void func_37(T *x) {}
                           ^~~~~~~
<source>:1:27: note: 'template<class T> void func_37(T*)' previously declared here
 template<typename T> void func_37(T *x);
                           ^~~~~~~
Compiler returned: 1

and icc 18:

<source>(7): error: function template "func_37" has already been defined
  template<typename T> void func_37(T *x) {}
                            ^
compilation aborted for <source> (code 2)
Compiler returned: 2

[sepavloff]

Rebased the patch

[sepavloff]

Updated patch

The fix for https://bugs.llvm.org/show_bug.cgi?id=39742 put a test
case, which is not a valid code. The error is detected with this
patch, tests are updated accordingly.

[rsmith]

Thanks (and sorry for dropping the ball on this review).

cfe/trunk/include/clang/AST/DeclBase.h

public:
   /// These declarations appear in the lexical context of the friending
   /// class, but in the semantic context of the actual entity.  This property
   /// applies only to a specific decl object;  other redeclarations of the
   /// same entity may not (and probably don't) share this property.
   void setObjectOfFriendDecl(bool PerformFriendInjection = false) {
     unsigned OldNS = IdentifierNamespace;
     assert((OldNS & (IDNS_Tag | IDNS_Ordinary |
                      IDNS_TagFriend | IDNS_OrdinaryFriend |
-                     IDNS_LocalExtern)) &&
+                     IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
            "namespace includes neither ordinary nor tag");
     assert(!(OldNS & ~(IDNS_Tag | IDNS_Ordinary | IDNS_Type |
                        IDNS_TagFriend | IDNS_OrdinaryFriend |
-                       IDNS_LocalExtern)) &&
+                       IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
            "namespace includes other than ordinary or tag");
 
     Decl *Prev = getPreviousDecl();
     IdentifierNamespace &= ~(IDNS_Ordinary | IDNS_Tag | IDNS_Type);
 
     if (OldNS & (IDNS_Tag | IDNS_TagFriend)) {
       IdentifierNamespace |= IDNS_TagFriend;
       if (PerformFriendInjection ||
           (Prev && Prev->getIdentifierNamespace() & IDNS_Tag))
         IdentifierNamespace |= IDNS_Tag | IDNS_Type;
     }
 
-    if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend | IDNS_LocalExtern)) {
+    if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend |
+                 IDNS_LocalExtern | IDNS_NonMemberOperator)) {
       IdentifierNamespace |= IDNS_OrdinaryFriend;
       if (PerformFriendInjection ||
           (Prev && Prev->getIdentifierNamespace() & IDNS_Ordinary))
         IdentifierNamespace |= IDNS_Ordinary;
     }
   }
 
   enum FriendObjectKind {

cfe/trunk/lib/Sema/SemaDecl.cpp

for (auto I : FD->redecls()) {
           if (Original->isThisDeclarationADefinition()) {
             Definition = I;
             break;
           }
         }
       }
     }
   }
+
+  if (!Definition)
+    // Similar to friend functions a friend function template may be a
+    // definition and do not have a body if it is instantiated in a class
+    // template.
+    if (FunctionTemplateDecl *FTD = FD->getDescribedFunctionTemplate()) {
+      for (auto I : FTD->redecls()) {
+        auto D = cast<FunctionTemplateDecl>(I);
+        if (D != FTD) {
+          assert(!D->isThisDeclarationADefinition() &&
+                 "More than one definition in redeclaration chain");
+          if (D->getFriendObjectKind() != Decl::FOK_None)
+            if (FunctionTemplateDecl *FT =
+                                       D->getInstantiatedFromMemberTemplate()) {
+              if (FT->isThisDeclarationADefinition()) {
+                Definition = D->getTemplatedDecl();
+                break;
+              }
+            }
+        }
+      }
+    }
+
   if (!Definition)
     return;
 
   if (canRedefineFunction(Definition, getLangOpts()))
     return;
 
   // Don't emit an error when this is redefinition of a typo-corrected
   // definition.

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

Decl *TemplateDeclInstantiator::VisitFunctionDecl(FunctionDecl *D,
 
   NamedDecl *PrincipalDecl = (TemplateParams
                               ? cast<NamedDecl>(FunctionTemplate)
                               : Function);
 
   // If the original function was part of a friend declaration,
   // inherit its namespace state and add it to the owner.
   if (isFriend) {
-    PrincipalDecl->setObjectOfFriendDecl();
+    Function->setObjectOfFriendDecl();
+    if (FunctionTemplateDecl *FT = Function->getDescribedFunctionTemplate())
+      FT->setObjectOfFriendDecl();
     DC->makeDeclVisibleInContext(PrincipalDecl);
 
     bool QueuedInstantiation = false;
 
     // C++11 [temp.friend]p4 (DR329):
     //   When a function is defined in a friend function declaration in a class
     //   template, the function is instantiated when the function is odr-used.
     //   The same restrictions on multiple declarations and definitions that

cfe/trunk/test/Modules/friend-definition.cpp

 // RUN: %clang_cc1 -fmodules -std=c++14 %s -verify
 // expected-no-diagnostics
 
 #pragma clang module build A
 module A {}
 #pragma clang module contents
 #pragma clang module begin A
 template<typename T> struct A {
   friend A operator+(const A&, const A&) { return {}; }
+  template<typename T2> friend void func_1(const A&, const T2 &) {}
 };
 #pragma clang module end
 #pragma clang module endbuild
 
 #pragma clang module build B
 module B {}
 #pragma clang module contents
 #pragma clang module begin B

 
 #pragma clang module import A
 #pragma clang module import B
 #pragma clang module import C
 
 void h() {
   A<int> a;
   a + a;
+  func_1(a, 0);
 }

cfe/trunk/test/SemaCXX/friend-template-redecl.cpp

 template <class T>
 void foo(const HasFriend<T> &m) noexcept(false) {}
 
 void f() {
   HasFriend<int> x;
   foo(x);
   bar(x);
 }
-
-namespace PR39742 {
-template<typename>
-struct wrapper {
-  template<typename>
-  friend void friend_function_template() {}
-};
-
-wrapper<bool> x;
-// FIXME: We should really error here because of the redefinition of
-// friend_function_template.
-wrapper<int> y;
-}

cfe/trunk/test/SemaCXX/friend2.cpp

 template<typename T> struct C22b {
   friend void func_22();
 };
 C22a<int> c22ai;
 C22b<int> c22bi;
 void func_22() {} // expected-error{{redefinition of 'func_22'}}
 
 
+// Case of template friend functions.
+
+template<typename T> void func_31(T *x);
+template<typename T1>
+struct C31a {
+  template<typename T> friend void func_31(T *x) {}
+};
+template<typename T1>
+struct C31b {
+  template<typename T> friend void func_31(T *x) {}
+};
+
+
+template<typename T> inline void func_32(T *x) {}
+template<typename T1>
+struct C32a {
+  template<typename T> friend void func_32(T *x) {}
+};
+template<typename T1>
+struct C32b {
+  template<typename T> friend void func_32(T *x) {}
+};
+
+
+template<typename T1>
+struct C33a {
+  template<typename T> friend void func_33(T *x) {}
+};
+template<typename T1>
+struct C33b {
+  template<typename T> friend void func_33(T *x) {}
+};
+
+
+template<typename T> inline void func_34(T *x) {}  // expected-note{{previous definition is here}}
+template<typename T1>
+struct C34 {
+  template<typename T> friend void func_34(T *x) {} // expected-error{{redefinition of 'func_34'}}
+};
+
+C34<int> v34;  // expected-note{{in instantiation of template class 'C34<int>' requested here}}
+
+
+template<typename T> inline void func_35(T *x);
+template<typename T1>
+struct C35a {
+  template<typename T> friend void func_35(T *x) {} // expected-note{{previous definition is here}}
+};
+template<typename T1>
+struct C35b {
+  template<typename T> friend void func_35(T *x) {} // expected-error{{redefinition of 'func_35'}}
+};
+
+C35a<int> v35a;
+C35b<int> v35b;  // expected-note{{in instantiation of template class 'C35b<int>' requested here}}
+
+
+template<typename T> void func_36(T *x);
+template<typename T1>
+struct C36 {
+  template<typename T> friend void func_36(T *x) {}  // expected-error{{redefinition of 'func_36'}}
+                                                     // expected-note@-1{{previous definition is here}}
+};
+
+C36<int> v36a;
+C36<long> v36b;  //expected-note{{in instantiation of template class 'C36<long>' requested here}}
+
+
+template<typename T> void func_37(T *x);
+template<typename T1>
+struct C37 {
+  template<typename T> friend void func_37(T *x) {} // expected-note{{previous definition is here}}
+};
+
+C37<int> v37;
+template<typename T> void func_37(T *x) {} // expected-error{{redefinition of 'func_37'}}
+
 
 namespace pr22307 {
 
 struct t {
   friend int leak(t);
 };
 
 template<typename v>

 
 template <typename K> bool operator<(const D<K> &, const D<K> &);
 
 void func() {
   D<D<int>> cache;
   cache.insert();
 }
 }
+
+namespace PR39742 {
+template<typename>
+struct wrapper {
+  template<typename>
+  friend void friend_function_template() {}  // expected-error{{redefinition of 'friend_function_template'}}
+                                             // expected-note@-1{{previous definition is here}}
+};
+
+wrapper<bool> x;
+wrapper<int> y;  // expected-note{{in instantiation of template class 'PR39742::wrapper<int>' requested here}}
+}