Change interpretation of function definition in friend declaration of template class.

[sepavloff]

Summary

Previously if a file-level function was defined inside befriending
template class, it was treated as defined in the code like:

void func(int);
template<typename T> class C1 {
  friend void func(int) {}
};
void m() {
  func(1);
}

The Standard requests ([temp.friend], p5) that `When a function is defined in
a friend function declaration in a class template, the function is instantiated
when the function is odr-used`. This statement however does not specify which
template is used for the instantiation, if the function is defined in several,
and what parameters are used for the instantiation. The latter is important if
the function implementation uses template parameter, such usage is not prohibited
by the Standard.

Other compilers (gcc, icc) follow viewpoint that body of the function defined
in friend declaration becomes available when corresponding class is instantiated.
This patch implements this viewpoint in clang.

Definitions introduced by friend declarations in template classes are added to
the redeclaration chain of corresponding function, but they do not make the
function defined. Only if such definition corresponds to the template
instantiation, the function becomes defined. Presence of function body is not
enough for function definition anymore.

This change fixes PR17923, PR22307 and PR25848.

[sepavloff]

Topic of friend function instantiation in C++ Standard discussion group

[rsmith]

Can we instead not add the function to the redeclaration chain until it's instantiated (like we do if it's dependent)?

[sepavloff]

Changed implementation

This implementation uses another approach. Previous version of the patch put friend functions
into redeclaration chains but modified search algorithm so that it skipped such functions. In
this version such functions are not included into redeclaration chains at all.

[rsmith]

lib/Sema/SemaDecl.cpp

8611–8612 ↗

(On Diff #51817)

Please factor this check out into a suitably-named function, shouldLinkDependentDeclWithPrevious or similar, and pass in OldDecl as well. I imagine we'll want to call this from multiple places (for instance, when handling VarDecls), and I can see a few ways of making it return true in more cases, which would allow us to provide more precise diagnostics in a few more cases.

(For instance, if the old and new declarations are in the same lexical context, we can mark them as redeclarations, and more generally I think we can do so if the new declaration has no more template parameters in scope than the old one did and the old declaration is declared within the current instantiation of the new declaration).

8613 ↗

(On Diff #51817)

I don't think it makes sense to check whether the template declaration is a definition. It should not be added to the redeclaration chain regardless of whether it's a definition.

10951–10956 ↗

(On Diff #51817)

Is this change necessary? If we're not putting dependent templates into redeclaration chains any more, we shouldn't find an existing definition ...

10962 ↗

(On Diff #51817)

... down here.

lib/Sema/SemaDeclCXX.cpp

12663–12673 ↗

(On Diff #51817)

What do these changes have to do with avoiding putting the declaration into the redeclaration chain? This looks equivalent to what the following if block will do, except that (a) it uses LookupName instead of LookupQualifiedName (which might be a bug fix but doesn't seem related to whether the context was dependent), and (b) it forgets to set DCScope (which looks like a bug).

[sepavloff]

Updated patch.

[sepavloff]

Any feedback?

Thanks,
--Serge

[rsmith]

Please also add some testcases for the corresponding case for a friend function template:

template<typename T> void f();
template<typename> struct A {
  template<typename T> void f() {}
};
template<typename> struct B {
  template<typename T> void f() {}
};
A<int> a;
B<int> b; // ill-formed

test/SemaCXX/PR25848.cpp

4 ↗

(On Diff #56015)

Maybe use int instead of a pointer to member type here? May as well keep the parts that aren't relevant to the test as simple as possible.

test/SemaCXX/friend2.cpp

77 ↗

(On Diff #56015)

"must be" -> "can be" (or maybe "should be")

We're not required to diagnose this; it's a QoI issue.

[sepavloff]

Updated patch

[sepavloff]

In D16989#432282, @rsmith wrote:

Please also add some testcases for the corresponding case for a friend function template:

template<typename T> void f();
template<typename> struct A {
  template<typename T> void f() {}
};
template<typename> struct B {
  template<typename T> void f() {}
};
A<int> a;
B<int> b; // ill-formed

Templates follow very different path. In the code you provided definitions for functions f are not generated, because they are not used, so diagnostics of such cases require special processing. Patch for template friends will be prepared soon.

[sepavloff]

Fixed test for friend functions.

[sepavloff]

Updated patch

Added handling of FunctionTemplateDecl to shouldLinkDependentDeclWithPrevious. It is not
used now but is required for subsequent patch. Also position comment correctly.

[sepavloff]

Added one more case in shouldLinkDependentDeclWithPrevious

[sepavloff]

Rebased the patch

[rsmith]

lib/Sema/SemaDecl.cpp

8657–8658 ↗

(On Diff #68376)

I don't think this is right. Given:

template<typename T> void f() {
  extern void g();
  extern void g();
}

... we do want to link the two declarations of g together. We even want to link them to a prior declaration of ::g if there is one, since they have a non-dependent type, otherwise we'll reject cases like:

void g();
constexpr void (*p)() = g;
template<bool> struct X { typedef int type; };
template<typename T> void f() {
  extern void g();
  X<&g == p>::type n; // note: no 'typename' required, not value-dependent,
}                     // must evaluate to 'true' while parsing the template

Perhaps we're trying to make this check too general, and we should instead only make this new logic apply to the case of friend declarations. Are there any cases where a friend declaration in a dependent context that names a non-member function needs to be added to the redeclaration chain?

8671–8673 ↗

(On Diff #68376)

This is not an example that reaches this case, because the lexical context of the function is not dependent.

[sepavloff]

lib/Sema/SemaDecl.cpp

8657–8658 ↗

(On Diff #68376)

It is convenient to have single function for both friend and forward declarations, as it is called from the point where both cases may be observed.

It looks like there is no mandatory requirement to add a friend declaration in a dependent context to the redeclaration chain, because ill-formed program will be detected during instantiation of enclosing template class. But making so allows to detect some problems earlier.

[sepavloff]

Updated patch

Added missing checks for forward declarations in shouldLinkDependentDeclWithPrevious.
Added test case from an ancient bug report.

[sepavloff]

Added missed test case.

[sepavloff]

Fixed DR136 for template classes

Previously clang created declarations of friend functions declared in template classes even
if that class was not instantiated. It allowed to make check for default arguments required
by DR136. This fix enables the check for the new implementation.

Also extended comment about putting friend function declarations into redeclaration chains.

Putting a friend declaration to the appropriate redeclaration chain is not necessary, any
errors in the declaration will be detected at instantiation time. However proper organization
of the declarations enables early detection of potential problems.

If a friend function is defined inline, it is almost certain that it should not be added to
the redeclaration chain. The following code demonstrates the problem:

void func();
template<typename T> class C1 {
    friend void func() {}
};
template<typename T> class C2 {
    friend void func() {}
}

Putting definitions of the friend functions into the same chain would require additional flag
to distinguish between 'actual' and 'potential' definitions. Operation with the chain becomes
more complicated. Putting only 'actual' definitions saves from the need to modify the mechanism
of redeclaration chain.

Putting non-defining declaration into the chain seems unnecessary. If there is corresponding
declaration at namespace level, as in the code:

void func();
template<typename T> class C1 {
    friend bool func() {}    // <- cannot be redeclared with different return type
};

compiler can detect invalid redeclaration because it finds prior declaration in the translation
unit in this example. Declaration of the friend function does not need to be saved for any check
in this case.

If there is no the function declaration at namespace level, putting non-defining declaration into
the chain can cause problems, for instance:

template<typename T> class C1 {
    friend void func() {}
};
template<typename T> class C2 {
    friend bool func() {}    // <- different return type
}

This code does not violates any requirement of the C++ standard. Only if both templates are
instantiated, code becomes ill-formed. But these friend function declarations cannot be put into
the same redeclaration chain.

So, friend function declarations defined in dependent context that names file-level functions
should not be put into redeclaration chain.

[rsmith]

lib/Sema/SemaDecl.cpp

8652 ↗

(On Diff #70025)

Most of the logic in this function can be replaced by something much simpler, such as

`return !(D->getFriendObjectKind() && D->getLexicalDeclContext()->isDependentContext());`

Can you try to make such a simplification? Can we use the above as the criterion here?

[sepavloff]

lib/Sema/SemaDecl.cpp

8652 ↗

(On Diff #70025)

Indeed, in negative form the condition is much simpler.
Thank you!

[sepavloff]

Updated patch

Rewrite the condition in shouldLinkDependentDeclWithPrevious in more compact form.

[rsmith]

A couple of comment nits, then this LGTM. Thanks!

lib/Sema/SemaDecl.cpp

8659–8664 ↗

(On Diff #70134)

We accept the second example without your change, and never get here (because we never find a PrevDecl). Just remove that one? The first example alone is enough.

lib/Sema/SemaDeclCXX.cpp

13770–13774 ↗

(On Diff #70134)

This is a bit of a weird thing to do, so please add a comment here explaining why we're not just looking at getPreviousDecl(). Something like:

"We can't look at FD->getPreviousDecl() because it may not have been set if we're in a dependent context. If we get this far with a non-empty Previous set, we must have a valid previous declaration of this function."

cfe/trunk/include/clang/Sema/Sema.h

public:
   void FindHiddenVirtualMethods(CXXMethodDecl *MD,
                           SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods);
   void NoteHiddenVirtualMethods(CXXMethodDecl *MD,
                           SmallVectorImpl<CXXMethodDecl*> &OverloadedMethods);
   // Returns true if the function declaration is a redeclaration
   bool CheckFunctionDeclaration(Scope *S,
                                 FunctionDecl *NewFD, LookupResult &Previous,
                                 bool IsExplicitSpecialization);
+  bool shouldLinkDependentDeclWithPrevious(Decl *D, Decl *OldDecl);
   void CheckMain(FunctionDecl *FD, const DeclSpec &D);
   void CheckMSVCRTEntryPoint(FunctionDecl *FD);
   Decl *ActOnParamDeclarator(Scope *S, Declarator &D);
   ParmVarDecl *BuildParmVarDeclForTypedef(DeclContext *DC,
                                           SourceLocation Loc,
                                           QualType T);
   ParmVarDecl *CheckParameter(DeclContext *DC, SourceLocation StartLoc,
                               SourceLocation NameLoc, IdentifierInfo *Name,

cfe/trunk/lib/Sema/SemaDecl.cpp

ClassScopeFunctionSpecializationDecl *NewSpec =
                                 HasExplicitTemplateArgs, TemplateArgs);
     CurContext->addDecl(NewSpec);
     AddToScope = false;
   }
 
   return NewFD;
 }
 
+/// \brief Checks if the new declaration declared in dependent context must be
+/// put in the same redeclaration chain as the specified declaration.
+///
+/// \param D Declaration that is checked.
+/// \param PrevDecl Previous declaration found with proper lookup method for the
+///                 same declaration name.
+/// \returns True if D must be added to the redeclaration chain which PrevDecl
+///          belongs to.
+///
+bool Sema::shouldLinkDependentDeclWithPrevious(Decl *D, Decl *PrevDecl) {
+  // Any declarations should be put into redeclaration chains except for
+  // friend declaration in a dependent context that names a function in
+  // namespace scope.
+  //
+  // This allows to compile code like:
+  //
+  //       void func();
+  //       template<typename T> class C1 { friend void func() { } };
+  //       template<typename T> class C2 { friend void func() { } };
+  //
+  // This code snippet is a valid code unless both templates are instantiated.
+  return !(D->getLexicalDeclContext()->isDependentContext() &&
+           D->getDeclContext()->isFileContext() &&
+           D->getFriendObjectKind() != Decl::FOK_None);
+}
+
 /// \brief Perform semantic checking of a new function declaration.
 ///
 /// Performs semantic analysis of the new function declaration
 /// NewFD. This routine performs all semantic checking that does not
 /// require the actual declarator involved in the declaration, and is
 /// used both for the declaration of functions as they are parsed
 /// (called via ActOnDeclarator) and for the declaration of functions
 /// that have been instantiated via C++ template instantiation (called
if (FunctionTemplateDecl *OldTemplateDecl
           FunctionDecl *const OldTemplatedDecl =
               OldTemplateDecl->getTemplatedDecl();
           assert(OldTemplatedDecl->getCanonicalDecl() == OldTemplatedDecl);
           OldTemplatedDecl->setDeletedAsWritten(false);
         }
       }
 
     } else {
+      if (shouldLinkDependentDeclWithPrevious(NewFD, OldDecl)) {
         // This needs to happen first so that 'inline' propagates.
         NewFD->setPreviousDeclaration(cast<FunctionDecl>(OldDecl));
-
         if (isa<CXXMethodDecl>(NewFD))
           NewFD->setAccess(OldDecl->getAccess());
+      } else {
+        Redeclaration = false;
+      }
     }
   }
 
   // Semantic checking for this function declaration (in isolation).
 
   if (getLangOpts().CPlusPlus) {
     // C++-specific checks.
     if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(NewFD)) {

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

if (ND->isInvalidDecl()) {
     else
       FD = cast<FunctionDecl>(ND);
 
     // C++11 [dcl.fct.default]p4: If a friend declaration specifies a
     // default argument expression, that declaration shall be a definition
     // and shall be the only declaration of the function or function
     // template in the translation unit.
     if (functionDeclHasDefaultArgument(FD)) {
-      if (FunctionDecl *OldFD = FD->getPreviousDecl()) {
+      // We can't look at FD->getPreviousDecl() because it may not have been set
+      // if we're in a dependent context. If we get this far with a non-empty
+      // Previous set, we must have a valid previous declaration of this
+      // function.
+      if (!Previous.empty()) {
         Diag(FD->getLocation(), diag::err_friend_decl_with_def_arg_redeclared);
-        Diag(OldFD->getLocation(), diag::note_previous_declaration);
+        Diag(Previous.getRepresentativeDecl()->getLocation(),
+             diag::note_previous_declaration);
       } else if (!D.isFunctionDefinition())
         Diag(FD->getLocation(), diag::err_friend_decl_with_def_arg_must_be_def);
     }
 
     // Mark templated-scope function declarations as unsupported.
     if (FD->getNumTemplateParameterLists() && SS.isValid()) {
       Diag(FD->getLocation(), diag::warn_template_qualified_friend_unsupported)
         << SS.getScopeRep() << SS.getRange()

cfe/trunk/test/SemaCXX/PR25848.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct A;
+
+inline int g();  // expected-warning{{inline function 'g' is not defined}}
+
+template<int M>
+struct R {
+  friend int g() {
+    return M;
+  }
+};
+
+void m() {
+  g();  // expected-note{{used here}}
+}

cfe/trunk/test/SemaCXX/friend2.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+
+// If a friend function is defined in several non-template classes,
+// it is an error.
+
+void func1(int);
+struct C1a {
+  friend void func1(int) {}  // expected-note{{previous definition is here}}
+};
+struct C1b {
+  friend void func1(int) {}  // expected-error{{redefinition of 'func1'}}
+};
+
+
+// If a friend function is defined in both non-template and template
+// classes it is an error only if the template is instantiated.
+
+void func2(int);
+struct C2a {
+  friend void func2(int) {}
+};
+template<typename T> struct C2b {
+  friend void func2(int) {}
+};
+
+void func3(int);
+struct C3a {
+  friend void func3(int) {}  // expected-note{{previous definition is here}}
+};
+template<typename T> struct C3b {
+  friend void func3(int) {}  // expected-error{{redefinition of 'func3'}}
+};
+C3b<long> c3;  // expected-note{{in instantiation of template class 'C3b<long>' requested here}}
+
+
+// If a friend function is defined in several template classes it is an error
+// only if several templates are instantiated.
+
+void func4(int);
+template<typename T> struct C4a {
+  friend void func4(int) {}
+};
+template<typename T> struct C4b {
+  friend void func4(int) {}
+};
+
+
+void func5(int);
+template<typename T> struct C5a {
+  friend void func5(int) {}
+};
+template<typename T> struct C5b {
+  friend void func5(int) {}
+};
+C5a<long> c5a;
+
+void func6(int);
+template<typename T> struct C6a {
+  friend void func6(int) {}  // expected-note{{previous definition is here}}
+};
+template<typename T> struct C6b {
+  friend void func6(int) {}  // expected-error{{redefinition of 'func6'}}
+};
+C6a<long> c6a;
+C6b<int*> c6b;  // expected-note{{in instantiation of template class 'C6b<int *>' requested here}}
+
+void func7(int);
+template<typename T> struct C7 {
+  friend void func7(int) {}  // expected-error{{redefinition of 'func7'}}
+                             // expected-note@-1{{previous definition is here}}
+};
+C7<long> c7a;
+C7<int*> c7b;  // expected-note{{in instantiation of template class 'C7<int *>' requested here}}
+
+
+// Even if clases are not instantiated and hence friend functions defined in them are not
+// available, their declarations can be checked.
+
+void func8(int);  // expected-note{{previous declaration is here}}
+template<typename T> struct C8a {
+  friend long func8(int);  // expected-error{{functions that differ only in their return type cannot be overloaded}}
+};
+
+void func9(int);  // expected-note{{previous declaration is here}}
+template<typename T> struct C9a {
+  friend int func9(int);  // expected-error{{functions that differ only in their return type cannot be overloaded}}
+};
+
+void func10(int);  // expected-note{{previous declaration is here}}
+template<typename T> struct C10a {
+  friend int func10(int);  // expected-error{{functions that differ only in their return type cannot be overloaded}}
+};
+
+void func_11();  // expected-note{{previous declaration is here}}
+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 {
+  friend int leak(t) { return sizeof(v); }  // expected-error{{redefinition of 'leak'}} expected-note{{previous definition is here}}
+};
+
+template struct m<char>;
+template struct m<short>;  // expected-note{{in instantiation of template class 'pr22307::m<short>' requested here}}
+
+int main() {
+  leak(t());
+}
+
+}
+
+namespace pr17923 {
+
+void f(unsigned long long);
+
+template<typename T> struct X {
+  friend void f(unsigned long long) {
+     T t;
+  }
+};
+
+int main() { f(1234); }
+
+}
+
+namespace pr17923a {
+
+int get();
+
+template< int value >
+class set {
+  friend int get()
+    { return value; } // return 0; is OK
+};
+
+template class set< 5 >;
+
+int main() {
+  get();
+}
+
+}
+
+namespace pr8035 {
+
+void Function();
+
+int main(int argc, char* argv[]) {
+  Function();
+}
+
+template <typename T>
+struct Test {
+  friend void Function() { }
+};
+
+template class Test<int>;
+
+}

cfe/trunk/test/SemaCXX/function-redecl-2.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+
+namespace redecl_in_templ {
+template<typename T> void redecl_in_templ() {
+  extern void func_1();  // expected-note  {{previous declaration is here}}
+  extern int  func_1();  // expected-error {{functions that differ only in their return type cannot be overloaded}}
+}
+
+void g();
+constexpr void (*p)() = g;
+
+template<bool> struct X {};
+template<> struct X<true> { typedef int type; };
+
+template<typename T> void f() {
+  extern void g();
+  X<&g == p>::type n;
+}
+}