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

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

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

You should use CheckForFunctionRedefinition to catch this.

[sepavloff]

lib/AST/DeclTemplate.cpp

292–311

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

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).

include/clang/AST/DeclTemplate.h

public:
   }
 
   /// Returns whether this template declaration defines the primary
   /// pattern.
   bool isThisDeclarationADefinition() const {
     return getTemplatedDecl()->isThisDeclarationADefinition();
   }
 
+  /// \brief Returns definition for this function template of null if no
+  /// definition was found.
+  ///
+  /// To properly handle function templates defined in friend declarations, the
+  /// function scans redeclaration chain and for each declaration from there
+  /// tries to find definition in the chains formed by InstantiatedFromMember.
+  ///
+  FunctionTemplateDecl *getDefinition() const;
+
   /// \brief Return the specialization with the provided arguments if it exists,
   /// otherwise return the insertion point.
   FunctionDecl *findSpecialization(ArrayRef<TemplateArgument> Args,
                                    void *&InsertPos);
 
   FunctionTemplateDecl *getCanonicalDecl() override {
     return cast<FunctionTemplateDecl>(
              RedeclarableTemplateDecl::getCanonicalDecl());

lib/AST/DeclTemplate.cpp

if (CommonPtr->LazySpecializations) {
     ASTContext &Context = getASTContext();
     uint32_t *Specs = CommonPtr->LazySpecializations;
     CommonPtr->LazySpecializations = nullptr;
     for (uint32_t I = 0, N = *Specs++; I != N; ++I)
       (void)Context.getExternalSource()->GetExternalDecl(Specs[I]);
   }
 }
 
+FunctionTemplateDecl *FunctionTemplateDecl::getDefinition() const {
+  for (auto *R : redecls()) {
+    FunctionTemplateDecl *F = cast<FunctionTemplateDecl>(R);
+    if (F->isThisDeclarationADefinition())
+      return F;
+
+    // If template does not have a body, probably it is instantiated from
+    // another template and is not used yet.
+    if (FunctionTemplateDecl *P = F->getInstantiatedFromMemberTemplate()) {
+      // If we have hit a point where the user provided a specialization of
+      // this template, we're done looking.
+      if (F->isMemberSpecialization())
+        return F;
+      if (FunctionTemplateDecl *Def = P->getDefinition())
+        return Def;
+    }
+  }
+
+  return nullptr;
+}
+
 llvm::FoldingSetVector<FunctionTemplateSpecializationInfo> &
 FunctionTemplateDecl::getSpecializations() const {
   LoadLazySpecializations();
   return getCommonPtr()->Specializations;
 }
 
 FunctionDecl *
 FunctionTemplateDecl::findSpecialization(ArrayRef<TemplateArgument> Args,

lib/Sema/SemaDecl.cpp

if (MergeFunctionDecl(NewFD, OldDecl, S, MergeTypeWithPrevious)) {
       return Redeclaration;
     }
 
     Previous.clear();
     Previous.addDecl(OldDecl);
 
     if (FunctionTemplateDecl *OldTemplateDecl
                                   = dyn_cast<FunctionTemplateDecl>(OldDecl)) {
-      NewFD->setPreviousDeclaration(OldTemplateDecl->getTemplatedDecl());
       FunctionTemplateDecl *NewTemplateDecl
         = NewFD->getDescribedFunctionTemplate();
       assert(NewTemplateDecl && "Template/non-template mismatch");
+      Redeclaration = shouldLinkDependentDeclWithPrevious(NewTemplateDecl,
+                                                          OldTemplateDecl);
+      if (Redeclaration &&
+          (NewTemplateDecl->getFriendObjectKind() != Decl::FOK_None ||
+           OldTemplateDecl->getFriendObjectKind() != Decl::FOK_None))
+        if (FunctionTemplateDecl *NewDef = NewTemplateDecl->getDefinition())
+          if (FunctionTemplateDecl *OldDef = OldTemplateDecl->getDefinition()) {
+            Diag(NewDef->getLocation(), diag::err_redefinition)
+                << NewDef->getDeclName();
+            Diag(OldDef->getLocation(), diag::note_previous_definition);
+            Redeclaration = false;
+          }
+      if (Redeclaration)
+        NewFD->setPreviousDeclaration(OldTemplateDecl->getTemplatedDecl());
       if (CXXMethodDecl *Method
             = dyn_cast<CXXMethodDecl>(NewTemplateDecl->getTemplatedDecl())) {
         Method->setAccess(OldTemplateDecl->getAccess());
         NewTemplateDecl->setAccess(OldTemplateDecl->getAccess());
       }
 
       // If this is an explicit specialization of a member that is a function
       // template, mark it as a member specialization.

test/SemaCXX/friend2.cpp

template<typename T> class C11 {
   friend int func_11();  // expected-error{{functions that differ only in their return type cannot be overloaded}}
 };
 
 void func_12(int x);  // expected-note{{previous declaration is here}}
 template<typename T> class C12 {
   friend void func_12(int x = 0);  // expected-error{{friend declaration specifying a default argument must be the only declaration}}
 };
 
-
 namespace pr22307 {
 
 struct t {
   friend int leak(t);
 };
 
 template<typename v>
 struct m {

 template <typename T>
 struct Test {
   friend void Function() { }
 };
 
 template class Test<int>;
 
 }
+
+// Case of template friend functions.
+
+template<typename T> void func_21(T *x);
+template<typename T1>
+struct C21a {
+  template<typename T> friend void func_21(T *x) {}
+};
+template<typename T1>
+struct C21b {
+  template<typename T> friend void func_21(T *x) {}
+};
+
+
+template<typename T> inline void func_22(T *x) {}
+template<typename T1>
+struct C22a {
+  template<typename T> friend void func_22(T *x) {}
+};
+template<typename T1>
+struct C22b {
+  template<typename T> friend void func_22(T *x) {}
+};
+
+
+template<typename T1>
+struct C23a {
+  template<typename T> friend void func_23(T *x) {}
+};
+template<typename T1>
+struct C23b {
+  template<typename T> friend void func_23(T *x) {}
+};
+
+
+template<typename T> inline void func_24(T *x) {}  // expected-note{{previous definition is here}}
+template<typename T1>
+struct C24 {
+  template<typename T> friend void func_24(T *x) {} // expected-error{{redefinition of 'func_24'}}
+};
+
+C24<int> v24;  // expected-note{{in instantiation of template class 'C24<int>' requested here}}
+
+
+template<typename T> inline void func_25(T *x);
+template<typename T1>
+struct C25a {
+  template<typename T> friend void func_25(T *x) {} // expected-note{{previous definition is here}}
+};
+template<typename T1>
+struct C25b {
+  template<typename T> friend void func_25(T *x) {} // expected-error{{redefinition of 'func_25'}}
+};
+
+C25a<int> v25a;
+C25b<int> v25b;  // expected-note{{in instantiation of template class 'C25b<int>' requested here}}
+
+
+template<typename T> void func_26(T *x);
+template<typename T1>
+struct C26 {
+  template<typename T> friend void func_26(T *x) {}  // expected-error{{redefinition of 'func_26'}}
+                                                     // expected-note@-1{{previous definition is here}}
+};
+
+C26<int> v26a;
+C26<long> v26b;  //expected-note{{in instantiation of template class 'C26<long>' requested here}}