[C++20][Modules] Handle template declarations in header units.

[iains]

Summary

This addresses part of https://github.com/llvm/llvm-project/issues/60079

The test for external functions was not considering function templates.

[iains]

@dblaikie - I suspect that this would be useful on the llvm-16 release branch, and so I've added you as a reviewer, if you have some chance to look ..
(I do not think @ChuanqiXu is available until February).

The issue here is that the function decl is extracted from function templates (and looks just like any other function definition at this point), so that we need to remember that it came from a template.

[iains]

this is necessary, but not sufficient (I need to make additions) .. no need to review yet.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

15265–15266

Would it make sense to use !isa<FunctionTemplateDecl>(D) here instead of adding IsFnTemplate?

[iains]

rebased, and revised to handle variable templates and instantiations.

[iains]

in my local testing, I was able to consume all libc++ headers individually.

clang/lib/Sema/SemaDecl.cpp

15265–15266

Would it make sense to use !isa<FunctionTemplateDecl>(D) here instead of adding IsFnTemplate?

I have changed this to use FD->isTemplated() to match the changes for VarDecls - where the template decl is not available. Would it be better to use the isa<>() ?

[Arthapz]

tried the patch and it seems to work (but libc++ seem's to have a little problem :D)

> rm -rf .xmake build; xmake f --toolchain=clang; xmake b
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release algorithm
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release list
[ 88%]: compiling.release src/main.cpp
[ 92%]: linking.release stl_headerunit_cpp_only
[100%]: build ok!

> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release list
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release algorithm
[ 88%]: compiling.release src/main.cpp
error: /usr/bin/../include/c++/v1/ostream:254:20: error: 'std::basic_ostream<char>::operator<<' from module '/usr/bin/../include/c++/v1/complex' is not present in definition of 'std::ostream' in module '/usr/bin/../include/c++/v1/iostream'
    basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
                   ^
/usr/bin/../include/c++/v1/ostream:221:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&))
                   ^
/usr/bin/../include/c++/v1/ostream:225:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ios<char_type, traits_type>&
                   ^
/usr/bin/../include/c++/v1/ostream:230:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(ios_base& (*__pf)(ios_base&))
                   ^
/usr/bin/../include/c++/v1/ostream:233:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(bool __n);
                   ^
/usr/bin/../include/c++/v1/ostream:234:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(short __n);
  > in src/main.cpp
  
> cat src/main.cpp
import <iostream>;
import <algorithm>;
import <deque>;
import <iostream>;
import <map>;
import <memory>;
import <set>;
import <utility>;
import <vector>;
import <string>;
import <queue>;
import <cstdlib>;
import <utility>;
import <exception>;
import <list>;
import <stack>;
import <complex>;
import <fstream>;
import <cstdio>;
import <iomanip>;

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

[iains]

In D142704#4088217, @Arthapz wrote:

tried the patch and it seems to work with libstdc++ but not with libc++

<SNIP>
> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
<SNIP>
[ 88%]: compiling.release src/main.cpp
error: /usr/bin/../include/c++/v1/ostream:254:20: error: 'std::basic_ostream<char>::operator<<' from module '/usr/bin/../include/c++/v1/complex' is not present in definition of 'std::ostream' in module '/usr/bin/../include/c++/v1/iostream'
    basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
                   ^
/usr/bin/../include/c++/v1/ostream:221:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&))
                   ^
<SNIP>

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

These seem to be a different diagnostic, do you find that is in some way related to the changes in D140261 + follow on patches?

[iains]

AFAICT the failing test is unrelated to this patch.

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

15265–15266

It looks not bad to me to use isTemplated (). And it looks like !FD->isTemplateInstantiation() is not tested? And it looks a little bit odd since the instantiated template should be implicitly inline.

[iains]

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

clang/lib/Sema/SemaDecl.cpp

15265–15266

It looks not bad to me to use isTemplated ().

Hmm. now I am not sure what you prefer; in a previous review there was an objection to not using the provided function in the decl class.

What would be your suggestion here?

(we cannot use isa<VarTemplateDecl> when testing the variables case because the caller pulls out the templated decl before we get to test it) - so it is more consistent (IMO) to use the same interface in both tests.

And it looks like !FD->isTemplateInstantiation() is not tested?

Please could you expand a bit on what you mean here?
(the test is clearly required, in practice)

And it looks a little bit odd since the instantiated template should be implicitly inline.

Did you see @dlaikie's comment on this topic above?

[iains]

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

clang/lib/Sema/SemaDecl.cpp

15265–15266

Did you see @dlaikie's comment on this topic above?

sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)

[ChuanqiXu]

In D142704#4092724, @iains wrote:

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

Yeah, this is a pretty severe problem. Luckily clang16 is going to be released in early March. So we have roughly one month to solve this problem. Even if we failed to solve this in the last minute, I think it is not too bad to revert https://github.com/llvm/llvm-project/commit/335668b116439d13c7555616e126acdc608ce59e.

clang/lib/Sema/SemaDecl.cpp

15265–15266

What would be your suggestion here?

I prefer isTemplated () here. If I get things correct, isTemplated () will cover the case that a non-template function decl inside a function template. And isa<FunctionTemplateDecl>(D) wouldn't handle the case. According to my understanding problem, we should avoid all the dependent cases.

Please could you expand a bit on what you mean here?

I mean I didn't find function template instantiation in tests of this revision page.

sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)

oh, this is because the inconsistent use of inline in different places.. this may be the price we need to pay.

[iains]

rebased, added tests for instantiated variable/function templates.

clang/lib/Sema/SemaDecl.cpp

15265–15266

Please could you expand a bit on what you mean here?

I mean I didn't find function template instantiation in tests of this revision page.

added.

[ChuanqiXu]

LGTM.

clang/lib/Sema/SemaDecl.cpp

if (RequireNonAbstractType(VDecl->getLocation(), VDecl->getType(),
                                diag::err_abstract_type_in_decl,
                                AbstractVariableType))
       VDecl->setInvalidDecl();
   }
 
   // C++ [module.import/6] external definitions are not permitted in header
   // units.
   if (getLangOpts().CPlusPlusModules && currentModuleIsHeaderUnit() &&
-      VDecl->isThisDeclarationADefinition() &&
+      !VDecl->isInvalidDecl() && VDecl->isThisDeclarationADefinition() &&
       VDecl->getFormalLinkage() == Linkage::ExternalLinkage &&
-      !VDecl->isInline()) {
+      !VDecl->isInline() && !VDecl->isTemplated() &&
+      !isa<VarTemplateSpecializationDecl>(VDecl)) {
     Diag(VDecl->getLocation(), diag::err_extern_def_in_header_unit);
     VDecl->setInvalidDecl();
   }
 
   // If adding the initializer will turn this declaration into a definition,
   // and we already have a definition for this variable, diagnose or otherwise
   // handle the situation.
   if (VarDecl *Def = VDecl->getDefinition())
Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   }
 
   // C++ [module.import/6] external definitions are not permitted in header
   // units.  Deleted and Defaulted functions are implicitly inline (but the
   // inline state is not set at this point, so check the BodyKind explicitly).
   // FIXME: Consider an alternate location for the test where the inlined()
   // state is complete.
   if (getLangOpts().CPlusPlusModules && currentModuleIsHeaderUnit() &&
+      !FD->isInvalidDecl() && !FD->isInlined() &&
+      BodyKind != FnBodyKind::Delete && BodyKind != FnBodyKind::Default &&
       FD->getFormalLinkage() == Linkage::ExternalLinkage &&
-      !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete &&
-      BodyKind != FnBodyKind::Default && !FD->isInlined()) {
+      !FD->isTemplated() && !FD->isTemplateInstantiation()) {
     assert(FD->isThisDeclarationADefinition());
     Diag(FD->getLocation(), diag::err_extern_def_in_header_unit);
     FD->setInvalidDecl();
   }
 
   // Ensure that the function's exception specification is instantiated.
   if (const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>())
     ResolveExceptionSpec(D->getLocation(), FPT);

clang/test/CXX/module/module.import/p6.cpp

 inline int ok_inline_var = 1;
 
 static int ok_static_var;
 
 int ok_var_decl;
 
 int bad_var_definition = 3;  // expected-error {{non-inline external definitions are not permitted in C++ header units}}
 
+/* The cases below should compile without diagnostics.  */
+
 class A {
 public:
     // This is a declaration instead of definition.
     static const int value = 43; 
 };
 
 void deleted_fn_ok (void) = delete;
 
 struct S {
    ~S() noexcept(false) = default;
 private:
   S(S&);
 };
 S::S(S&) = default;
+
+template <class _X>
+_X tmpl_var_ok_0 = static_cast<_X>(-1);
+
+template <typename _T>
+constexpr _T tmpl_var_ok_1 = static_cast<_T>(42);
+
+template <typename _Tp,
+          template <typename> class _T>
+constexpr int tmpl_var_ok_2 = _T<_Tp>::value ? 42 : 6174 ;
+
+template<class _Ep>
+int tmpl_OK (_Ep) { return 0; }
+
+template <class _T1>
+bool
+operator==(_T1& , _T1& ) { return false; }
+
+constexpr long one_k = 1000L;
+
+template <class ..._Args>
+void* tmpl_fn_ok
+(_Args ...__args) { return nullptr; }