[C++20][Modules] Improve handing of Private Module Fragment diagnostics.

[iains]

Summary

This adds a check for exported inline functions, that there is a definition in
the definition domain (which, in practice, can only be the module purview but
before any PMF starts) since the PMF definition domain cannot contain exports.

This is:
[dcl.inline]/7
If an inline function or variable that is attached to a named module is declared in
a definition domain, it shall be defined in that domain.

The patch also amends diagnostic output by excluding the PMF sub-module from the
set considered as sources of missing decls. There is no point in telling the user
that the import of a PMF object is missing - since such objects are never reachable
to an importer. We still show the definition (as unreachable), to help point out
this.

[ChuanqiXu]

It looks like we need to handle inline variable as well to match the intention.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

From my reading, 'exported' is not emphasized.

clang/lib/Sema/SemaModule.cpp

913–923

So we might need to move this to somewhere else.

[iains]

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

it is here:
https://eel.is/c++draft/module#private.frag-2.1
( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).

It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?

clang/lib/Sema/SemaModule.cpp

913–923

depending on reading of the cases that can have this effect.

[iains]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

it is here:
https://eel.is/c++draft/module#private.frag-2.1
( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).

hmm... my linkage comment is wrong - however the distinction between exported and odr-used seems to be made here (fn_m() and fn_e()).

It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

What I read is:

[dcl.inline]p7: https://eel.is/c++draft/dcl.inline#7

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

and the definition of definition domain is:

[basic.def.odr]p12: https://eel.is/c++draft/basic#def.odr-12

A definition domain is a private-module-fragment or the portion of a translation unit excluding its private-module-fragment (if any).

The definition of "attached to a named module" is:

[module.unit]p7: https://eel.is/c++draft/module.unit#7

A module is either a named module or the global module. A declaration is attached to a module as follows: ...

So it is clearly not consistency with [module.private.frag]p2.1. I would send this to WG21.

[vsapsai]

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

clang/test/Modules/cxx20-10-5-ex1.cpp

26–29

Can export inline function call other non-export inline functions? Sorry if it is tested somewhere else. Curious what are the transitive restrictions, so we test edge cases.

[ChuanqiXu]

In D128328#3603781, @vsapsai wrote:

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

The suggested diagnostic message might not be right since the following one should be legal:

export module m;
...
module :private;
// foo() is not declared earlier.
inline void foo() {...}

The definition of 'foo' here should be legal although it is inline and in private fragment.

clang/test/Modules/cxx20-10-5-ex1.cpp

26–29

I guess it is not tested. But a non-export function wouldn't be exported if it is called by export function from the perspective of std. Although more tests should be fine all the time, the current test case should come from the example in the standard. We could send another test if we want.

[iains]

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

In D128328#3603781, @vsapsai wrote:

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

I agree my error message is kinda "implementor-speak" really, there's a tension between using something that will allow the user to find the source of the problem with a google search and and avoiding that.

We could implement what you suggest (pending a resolution of what we're actually supposed to be implementing) - I guess - but we'd need to defer the check until the end of the TU - i.e. after any potential PMF. I think we can differentiate the case that @ChuanqiXu noted (definition local to the PMF) because that would not have an entry in the PendingInlineExports table.

As for transitive cases, I agree we need to defer consideration of that until we can discuss with core - otherwise this patch will become a rabbit hole ;)

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

Yes, that was what I found - maybe we are missing something about the export that changes those rules.
.

[ChuanqiXu]

In D128328#3603940, @iains wrote:

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

But I feel like we couldn't go on before we get response from WG21.

[iains]

In D128328#3603942, @ChuanqiXu wrote:

In D128328#3603940, @iains wrote:

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

but we reject this at the moment with "redefinition of 'a'" - so that implies we do not have fully correct C++17 handling here?

But I feel like we couldn't go on before we get response from WG21.

Agreed, and anyway I think we would want to add a new test case, not to amend the example from the std (otherwise that becomes confusing as well)

[ChuanqiXu]

In D128328#3603945, @iains wrote:

In D128328#3603942, @ChuanqiXu wrote:

In D128328#3603940, @iains wrote:

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

but we reject this at the moment with "redefinition of 'a'" - so that implies we do not have fully correct C++17 handling here?

Sorry, my bad. The configuration of my godbolt was wrong (The input is LLVM IR). I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

The meaning of [export] depends on the result of the feedback from WG21.

But I feel like we couldn't go on before we get response from WG21.

Agreed, and anyway I think we would want to add a new test case, not to amend the example from the std (otherwise that becomes confusing as well)

[iains]

In D128328#3603953, @ChuanqiXu wrote:

In D128328#3603945, @iains wrote:

In D128328#3603942, @ChuanqiXu wrote:

In D128328#3603940, @iains wrote:

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

hmm ... isn't that implicitly initialised with 0?

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

The meaning of [export] depends on the result of the feedback from WG21.

yes, I saw your post to ext.

[ChuanqiXu]

In D128328#3603967, @iains wrote:

In D128328#3603953, @ChuanqiXu wrote:

In D128328#3603945, @iains wrote:

In D128328#3603942, @ChuanqiXu wrote:

In D128328#3603940, @iains wrote:

In D128328#3602646, @iains wrote:

In D128328#3601080, @ChuanqiXu wrote:

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

hmm ... isn't that implicitly initialised with 0?

Oh, I just realized every declaration is a definition too except some cases: https://eel.is/c++draft/basic.def#2

And I failed to give an example that a undefined inline variable declaration attached to the named modules. I wondered:

export module A;
extern "C++" inline int a;
extern "C++" int a = 0;

But now 'a' is attached to global module instead of named modules. So I just failed to find a good example... very sorry for wasting the time

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

[iains]

In D128328#3603980, @ChuanqiXu wrote:

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

Right, that should be an error - we do not have a test for it.
ActOnEndOfTranslationUnitFragment should be called with "Kind =TUFragmentKind::Normal" whether there us a PMF or not.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L1131
so - modulo the underlying question about 'export' we probably just need a second test-case
(we could still defer the processing to the end of the TU and implement the error messages as suggested by @vsapsai )

[ChuanqiXu]

In D128328#3604110, @iains wrote:

In D128328#3603980, @ChuanqiXu wrote:

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

Right, that should be an error - we do not have a test for it.
ActOnEndOfTranslationUnitFragment should be called with "Kind =TUFragmentKind::Normal" whether there us a PMF or not.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L1131

I don't understand the intention of the suggestion. I feel like the current one looks right.

so - modulo the underlying question about 'export' we probably just need a second test-case

Agreed.

(we could still defer the processing to the end of the TU and implement the error messages as suggested by @vsapsai )

Oh, the intention of your suggestion is to implement the suggestion by @vsapsai, right? If it is, I would like to suggest to move the processing of PendingInlineExports to ActOnEndOfTranslationUnit instead of change the argument of ActOnEndOfTranslationUnitFragment.

[vsapsai]

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

[ChuanqiXu]

From the discussion, it looks like the 'export' part is not necessary here and we don't need to care about linkage in this revision.

In D128328#3609827, @vsapsai wrote:

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

It makes sense to me.

[iains]

In D128328#3611194, @ChuanqiXu wrote:

From the discussion, it looks like the 'export' part is not necessary here and we don't need to care about linkage in this revision.

Indeed.

In D128328#3609827, @vsapsai wrote:

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

It makes sense to me.

So I will re-work this patch to deal with the two changes (I think that the proposed merge of changes to the example should be enough to go on).

[iains]

rebased, reworked

• to follow the changes proposed by core
• to make the diagnostics follow that and a compromise for the proposed revision before the core amendment.

[iains]

the revised diagnostics look like this:

error: {un-}exported inline function not defined before the private module fragment

with
note: private module fragment begins here pointing to the start of the PMF

If there is no PMF then we just say:

error: {un-}exported inline function not defined

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

I think that we can consider this closed by the question to the ext reflector and the amendment proposed by core.

clang/test/Modules/cxx20-10-5-ex1.cpp

26–29

I've changed the example to match the proposed amendment to the standard.

If you think we should have some other test case (additional to Modules/Reachability-Private), that's fine - would you like to propose one (or maybe add to an existing)?

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

I prefer inline function attached to a named module not defined %select{| before the private module fragment}1. Since the export part is not important here and the important part is whether or not they are attached to a named module.

clang/lib/Sema/SemaDecl.cpp

9849

clang/test/Modules/cxx20-10-5-ex1.cpp

26–29

The current one looks fine to me.

[iains]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

I took the error message from here:
https://github.com/cplusplus/draft/pull/5537
which was prepared after the dicussion that you started on the ext reflector. Actually, I do not have a strong feeling either way.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

Oh, I didn't track that. I feel my suggested version is slightly better. Otherwise users might want to make it work by adding/removing export clause.

[iains]

rebased, retested.

[iains]

rebased, addressed review comments

[iains]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11145

I removed the reference to un/exported.

[ChuanqiXu]

clang/include/clang/Sema/Sema.h

2264

I'm curious why the element is not FunctionDecl?

clang/lib/Sema/Sema.cpp

1262

nit:

[iains]

rebased, addressed remaing comments.

[iains]

clang/lib/Sema/Sema.cpp

1262

(I added this one)
I wonder if we could try to reduce the number of asserts included by considering making a test-case to cover the condition that is of concern - since most devs build with assertions enabled, we should ensure that they are only covering some case we really cannot test directly.

[ChuanqiXu]

clang/lib/Sema/Sema.cpp

1262

I think the test cases and assertion are not conflicted with each other. For example, when I made a change and a test case fails, I may spent a lot of time to locate the problem. But if the assertion fails, I will get the problem more quickly. And for the specific case, it looks like not easy to construct a test case.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_export_internal : Error<
   "declaration of %0 with internal linkage cannot be exported">;
 def err_export_using_internal : Error<
   "using declaration referring to %1 with %select{internal|module|unknown}0 "
   "linkage cannot be exported">;
 def err_export_not_in_module_interface : Error<
   "export declaration can only be used within a module interface unit"
   "%select{ after the module declaration|}0">;
+def err_export_inline_not_defined : Error<
+  "inline function not defined%select{| before the private module fragment}0">;
 def err_export_partition_impl : Error<
   "module partition implementations cannot be exported">;
 def err_export_in_private_module_fragment : Error<
   "export declaration cannot be used in a private module fragment">;
 def note_private_module_fragment : Note<
   "private module fragment begins here">;
 def err_private_module_fragment_not_module : Error<
   "private module fragment declaration with no preceding module declaration">;

clang/include/clang/Sema/Sema.h

private:
   clang::Module *GlobalModuleFragment = nullptr;
 
   /// The modules we imported directly.
   llvm::SmallPtrSet<clang::Module *, 8> DirectModuleImports;
 
   /// Namespace definitions that we will export when they finish.
   llvm::SmallPtrSet<const NamespaceDecl*, 8> DeferredExportedNamespaces;
 
+  /// In a C++ standard module, inline declarations require a definition to be
+  /// present at the end of a definition domain.  This set holds the decls to
+  /// be checked at the end of the TU.
+  llvm::SmallPtrSet<const FunctionDecl *, 8> PendingInlineFuncDecls;
+
   /// Helper function to judge if we are in module purview.
   /// Return false if we are not in a module.
   bool isCurrentModulePurview() const {
     return getCurrentModule() ? getCurrentModule()->isModulePurview() : false;
   }
 
   /// Enter the scope of the global module.
   Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);

clang/lib/Sema/Sema.cpp

if (Module *CurrentModule = PP.getCurrentModule()) {
       }
     }
 
     // Warnings emitted in ActOnEndOfTranslationUnit() should be emitted for
     // modules when they are built, not every time they are used.
     emitAndClearUnusedLocalTypedefWarnings();
   }
 
+  // C++ standard modules. Diagnose cases where a function is declared inline
+  // in the module purview but has no definition before the end of the TU or
+  // the start of a Private Module Fragment (if one is present).
+  if (!PendingInlineFuncDecls.empty()) {
+    for (auto *D : PendingInlineFuncDecls) {
+      if (auto *FD = dyn_cast<FunctionDecl>(D)) {
+        bool DefInPMF = false;
+        if (auto *FDD = FD->getDefinition()) {
+          assert(FDD->getOwningModule() &&
+                 FDD->getOwningModule()->isModulePurview());
+          DefInPMF = FDD->getOwningModule()->isPrivateModule();
+          if (!DefInPMF)
+            continue;
+        }
+        Diag(FD->getLocation(), diag::err_export_inline_not_defined)
+            << DefInPMF;
+        // If we have a PMF it should be at the end of the ModuleScopes.
+        if (DefInPMF &&
+            ModuleScopes.back().Module->Kind == Module::PrivateModuleFragment) {
+          Diag(ModuleScopes.back().BeginLoc,
+               diag::note_private_module_fragment);
+        }
+      }
+    }
+    PendingInlineFuncDecls.clear();
+  }
+
   // C99 6.9.2p2:
   //   A declaration of an identifier for an object that has file
   //   scope without an initializer, and without a storage-class
   //   specifier or with the storage-class specifier static,
   //   constitutes a tentative definition. If a translation unit
   //   contains one or more tentative definitions for an identifier,
   //   and the translation unit contains no external definition for
   //   that identifier, then the behavior is exactly as if the

clang/lib/Sema/SemaDecl.cpp

if (getLangOpts().CPlusPlus) {
     //   as if it were specified with noexcept(true).
     const FunctionProtoType *FPT = R->getAs<FunctionProtoType>();
     if ((Name.getCXXOverloadedOperator() == OO_Delete ||
          Name.getCXXOverloadedOperator() == OO_Array_Delete) &&
         getLangOpts().CPlusPlus11 && FPT && !FPT->hasExceptionSpec())
       NewFD->setType(Context.getFunctionType(
           FPT->getReturnType(), FPT->getParamTypes(),
           FPT->getExtProtoInfo().withExceptionSpec(EST_BasicNoexcept)));
+
+    // C++20 [dcl.inline]/7
+    // If an inline function or variable that is attached to a named module
+    // is declared in a definition domain, it shall be defined in that
+    // domain.
+    // So, if the current declaration does not have a definition, we must
+    // check at the end of the TU (or when the PMF starts) to see that we
+    // have a definition at that point.
+    if (isInline && !D.isFunctionDefinition() && getLangOpts().CPlusPlus20 &&
+        NewFD->hasOwningModule() &&
+        NewFD->getOwningModule()->isModulePurview()) {
+      PendingInlineFuncDecls.insert(NewFD);
+    }
   }
 
   // Filter out previous declarations that don't match the scope.
   FilterLookupForScope(Previous, OriginalDC, S, shouldConsiderLinkage(NewFD),
                        D.getCXXScopeSpec().isNotEmpty() ||
                        isMemberSpecialization ||
                        isFunctionTemplateSpecialization);
 

clang/lib/Sema/SemaLookup.cpp

auto NotePrevious = [&] {
     // declaration was previously reached.
     Diag(DeclLoc, diag::note_unreachable_entity) << (int)MIK;
   };
 
   // Weed out duplicates from module list.
   llvm::SmallVector<Module*, 8> UniqueModules;
   llvm::SmallDenseSet<Module*, 8> UniqueModuleSet;
   for (auto *M : Modules) {
-    if (M->Kind == Module::GlobalModuleFragment)
+    if (M->isGlobalModule() || M->isPrivateModule())
       continue;
     if (UniqueModuleSet.insert(M).second)
       UniqueModules.push_back(M);
   }
 
   // Try to find a suitable header-name to #include.
   std::string HeaderName;
   if (const FileEntry *Header =

clang/lib/Sema/SemaModule.cpp

SourceLocation BlockStart =
         ED->hasBraces() ? ED->getBeginLoc() : SourceLocation();
     for (auto *Child : ED->decls()) {
       if (checkExportedDecl(*this, Child, BlockStart)) {
         // If a top-level child is a linkage-spec declaration, it might contain
         // no declarations (transitively), in which case it's ill-formed.
         diagExportedUnnamedDecl(*this, UnnamedDeclKind::Context, Child,
                                 BlockStart);
       }
+      if (auto *FD = dyn_cast<FunctionDecl>(Child)) {
+        // [dcl.inline]/7
+        // If an inline function or variable that is attached to a named module
+        // is declared in a definition domain, it shall be defined in that
+        // domain.
+        // So, if the current declaration does not have a definition, we must
+        // check at the end of the TU (or when the PMF starts) to see that we
+        // have a definition at that point.
+        if (FD->isInlineSpecified() && !FD->isDefined())
+          PendingInlineFuncDecls.insert(FD);
+      }
     }
   }
 
   return D;
 }
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
                                        bool IsImplicit) {

clang/test/Modules/Reachability-Private.cpp

 // Tests that the definition in private module fragment is not reachable to its users.
 //
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: split-file %s %t
 //
-// RUN: %clang_cc1 -std=c++20 %t/Private.cppm -emit-module-interface -o %t/Private.pcm
-// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/Private.cppm -emit-module-interface \
+// RUN: -o %t/Private.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp \
+// RUN: -DTEST_BADINLINE -verify -fsyntax-only
 
 //--- Private.cppm
 export module Private;
-inline void fn_m(); // OK, module-linkage inline function
+#ifdef TEST_BADINLINE
+inline void fn_m(); // expected-error {{un-exported inline function not defined before the private module fragment}}
+                    // [e-mail]:13 {{private module fragment begins here}}
+#endif
 static void fn_s();
 export struct X;
 
 export void g(X *x) {
   fn_s(); // OK, call to static function in same translation unit
-  fn_m(); // OK, call to module-linkage inline function
+#ifdef TEST_BADINLINE
+  fn_m(); // fn_m is not OK.
+#endif
 }
 export X *factory(); // OK
 
 module :private;
 struct X {}; // definition not reachable from importers of A
 X *factory() {
   return new X();
 }
 void fn_m() {}
 void fn_s() {}
 
 //--- Use.cpp
 import Private;
 void foo() {
-  X x; // expected-error {{definition of 'X' must be imported from module 'Private.<private>' before it is required}}
-       // expected-error@-1 {{definition of 'X' must be imported from module 'Private.<private>' before it is required}}
-       // expected-note@* {{definition here is not reachable}}
-       // expected-note@* {{definition here is not reachable}}
+  X x; // expected-error 1+{{missing '#include'; 'X' must be defined before it is used}}
+       // [e-mail]:18 1+{{definition here is not reachable}}
   auto _ = factory();
   auto *__ = factory();
   X *___ = factory();
 
   g(__);
   g(___);
   g(factory());
 }

clang/test/Modules/cxx20-10-5-ex1.cpp

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-5-ex1-interface.cpp \
+// RUN: -DBAD_FWD_DECL  -fsyntax-only -verify
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-5-ex1-interface.cpp \
+// RUN: -o A.pcm
+
+// RUN: %clang_cc1 -std=c++20 std-10-5-ex1-use.cpp  -fmodule-file=A.pcm \
+// RUN:    -fsyntax-only -verify
+
+//--- std-10-5-ex1-interface.cpp
+
+export module A;
+#ifdef BAD_FWD_DECL
+export inline void fn_e(); // expected-error {{inline function not defined before the private module fragment}}
+                           // [e-mail]:21 {{private module fragment begins here}}
+#endif
+export inline void ok_fn() {}
+export inline void ok_fn2();
+#ifdef BAD_FWD_DECL
+inline void fn_m(); // expected-error {{inline function not defined before the private module fragment}}
+                    // [e-mail]:21 {{private module fragment begins here}}
+#endif
+static void fn_s();
+export struct X;
+export void g(X *x) {
+  fn_s();
+}
+export X *factory();
+void ok_fn2() {}
+
+module :private;
+struct X {};
+X *factory() {
+  return new X();
+}
+
+void fn_e() {}
+void fn_m() {}
+void fn_s() {}
+
+//--- std-10-5-ex1-use.cpp
+
+import A;
+
+void foo() {
+  X x; // expected-error 1+{{missing '#include'; 'X' must be defined before it is used}}
+       // [e-mail]:22 1+{{definition here is not reachable}}
+  X *p = factory();
+}