[C++20][Modules] Handle defaulted and deleted functions in header units.

[iains]

Summary

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

Deleted and Defaulted functions are implicitly inline, but that state
is not set at the point that we perform the diagnostic checks for externally-
visible non-inline functions; check the function body type explicitly in the
diagnostic.

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

15258

I prefer to add a FIXME here to say that we need to find a better place for the check to eliminate the unnecessary check for BodyKind . The current check for BodyKind looks a little bit hacky to me.

15263

It looks like we need to check FD->isThisDeclarationADefinition() too.

And personally, I prefer to check BodyKind explicitly. Otherwise the readers need to checkout the definition of FnBodyKind to understand the code.

[iains]

rebased, address review comments

clang/lib/Sema/SemaDecl.cpp

15258

When the patch was originally done, this was found to be a good place to do the check (i.e. less duplication of testing and to avoid duplication of diagnostics) so I do not think I agree that there is a FIXME to move it.

BodyKind is already used elsewhere in this function for similar purposes - it does not look hacky to me.

15263

It looks like we need to check FD->isThisDeclarationADefinition() too.

This is an unnecessary test, it will always return true at this point.

And personally, I prefer to check BodyKind explicitly. Otherwise the readers need to checkout the definition of FnBodyKind to understand the code.

You prefer two tests instead of one?
OK, I guess

[ChuanqiXu]

LGTM basically. I still feel we need a FIXME there. But I don't want to block this for this reason especially we need to land this before the branch.

clang/lib/Sema/SemaDecl.cpp

15258

It looks hacky to me since we shouldn't care if it is deleted or defaulted here and it should be enough to check FD->isInlied(). And I don't see similar usage of BodyKind in this function.

15263

This is an unnecessary test, it will always return true at this point.

Oh, I found it now. It may be better to have an assertion assert(FD->isThisDeclarationADefinition()).

[iains]

In D141908#4061409, @ChuanqiXu wrote:

LGTM basically. I still feel we need a FIXME there. But I don't want to block this for this reason especially we need to land this before the branch.

Well.. we have time for another iteration, I will add the assert...

clang/lib/Sema/SemaDecl.cpp

15258

It looks hacky to me since we shouldn't care if it is deleted or defaulted here and it should be enough to check FD->isInlied().

that means checking much later and in muliple places, I think - but if you want to make a follow-on patch, I will be happy to review.

And I don't see similar usage of BodyKind in this function.

line 15208?

15263

yeah, I was thinking maybe to do that (it is kind of documenting that it is always true - perhaps a comment would be better?)

[ChuanqiXu]

Well.. we have time for another iteration,

I am going to take a vacation for the Chinese New Year since tomorrow to February. So I am a little bit hurried : )

clang/lib/Sema/SemaDecl.cpp

15258

line 15208?

line 15208 checks the wording unless the function is deleted (C++ specifc, C++ [dcl.fct.def.general]p2). So it is used to check if this is a delete function, which looks fine. I mean it would be best if we can check FD->isInlined() only.

that means checking much later and in muliple places, I think - but if you want to make a follow-on patch, I will be happy to review.

I think I won't work on this. But it would be meaningful for future developers. And it looks not impossible to refactor it. (I'm not asking you to do the change)

15263

I always prefer assertion than comments since some people may not like to read the comments. And the assertion may be useful. For example, if someday we want to move the codes to another place but the FD is not always a definition. Then the assertion can save us a lot of time.

[iains]

address review commments, add an assert and a FIXME.

[iains]

In D141908#4061451, @ChuanqiXu wrote:

Well.. we have time for another iteration,

I am going to take a vacation for the Chinese New Year since tomorrow to February. So I am a little bit hurried : )

(I added the FIXME) Have a good holiday!

clang/lib/Sema/SemaDecl.cpp

15258

that means checking much later and in muliple places, I think - but if you want to make a follow-on patch, I will be happy to review.

I think I won't work on this. But it would be meaningful for future developers.

I re-checked (to remind myself) .. essentially that state is set by SetFunctionBodyKind() which is called much later. I still think we would most likely end up with having to place the diagnostic in multiple places. However, I added the FIXME,

And it looks not impossible to refactor it. (I'm not asking you to do the change)

such a refactoring looks non-trivial and certainly not suitable for a few days before a branch ...

[iains]

rebased.

clang/lib/Sema/SemaDecl.cpp

for (auto *Param : FD->parameters()) {
     if (Param->getIdentifier() && FnBodyScope) {
       CheckShadow(FnBodyScope, Param);
 
       PushOnScopeChains(Param, FnBodyScope);
     }
   }
 
   // C++ [module.import/6] external definitions are not permitted in header
-  // units.
+  // 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->getFormalLinkage() == Linkage::ExternalLinkage && !FD->isInlined()) {
+      FD->getFormalLinkage() == Linkage::ExternalLinkage &&
+      !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete &&
+      BodyKind != FnBodyKind::Default && !FD->isInlined()) {
+    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

 int bad_var_definition = 3;  // expected-error {{non-inline external definitions are not permitted in C++ header units}}
 
 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;