[C++20][Modules] Fix incorrect visibilities in implementation units.

[iains]

Summary

Some of the testcases committed during the merging of modules contain
FIXMEs concerning the visibility of declarations with internal-linkage in
module interface units as seen from module implementation units.

This is, in part, caused by an oversight in the utility routine that determines
whether a module is part of the current TU or not.

The lookup definition for visible was also too wide, failing to account
cases where a module was visible, but some declarations from it were not.

This patch has two components:
1/ Checking for visibility of imported decls and excluding thsose with
internal linkage (when from a different TU) or from a private module
fragment. The logic for this has to accommodate differences for ADL when
the lookup is for a templated entity that is defined in a different TU
from the importer.

2/ Updating the diagnostics in reponse to this, in particular dealing
better with the case that the user attempts to use a module-local linkage
item in an importing TU (we now provide a fixit to indicate that the item
needs to be exported to be used). When we search for possible "typos",
which we do in response to failing to find an entity, we do so allowing
finds of 'hidden' names. Before we offer names found this as possible
typo fixes, we need to check that they would be viable if not hidden.

There are a number of basically mechanical changes to tests to accommodate
the fixed rejection of relevant declarations and the improved diagnostic
messages.

[iains]

notes:
a) I need to update the commit log message so that it formats properly in the browser
b) since we are now seeing performance complaints against modules, and some of these routines are used many times (sometimes several time per name), I have tried to structure tests so that the cheapest ones execute first - and likewise the utility routines.

[ChuanqiXu]

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring). I feel this is more natural and consistent. I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports. So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.

BTW, I suggest we file an issue first if we want to do something. So that we can avoid solving the same problem.

clang/lib/Sema/SemaModule.cpp

307–308

This can be a separate patch.

[iains]

In D145965#4191973, @ChuanqiXu wrote:

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring).

I think it is not small refactoring - we make extensive use of "getCurrentModule()" to determine if we are processing a module and the interface to know which module the implementation belongs to.

If we add the implicitly imported interface to "imports" instead of the module scope we will need to make sure that we deal with all these cases where we rely on the interface module for this information.

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

I feel this is more natural and consistent.

Yes, if we were starting from 0, that is so - but we have an implementation with long history - we would need to see the impact. [I am not against making this cleaner, but concerned about the amount of change].

I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports.

I was wondering whether some of the reports filed naming duplicate and clashing symbols could be related, but I did not analyse.

So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.

I was working on this because it was affecting my P1815 stuff - but I can use this patch as a temporary solution.

BTW, I suggest we file an issue first if we want to do something. So that we can avoid solving the same problem.

There are (I think your) FIXMEs in the testcases (but I do not know if there is a corresponding PR).

The checks for internal-linkage symbols and the improvements to diagnostics do not depend on how Sema::isModuleUnitOfCurrentTU is implemented, so that I think we could fix these problems and then deal with the refactoring later.

In either case, I do not have much time to work more on this right now.

[ChuanqiXu]

In D145965#4192051, @iains wrote:

In D145965#4191973, @ChuanqiXu wrote:

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring).

I think it is not small refactoring - we make extensive use of "getCurrentModule()" to determine if we are processing a module and the interface to know which module the implementation belongs to.

If we add the implicitly imported interface to "imports" instead of the module scope we will need to make sure that we deal with all these cases where we rely on the interface module for this information.

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

Yeah, understood. I had a mental model but I am not sure if it can work very well as I expected. I'll try to give it a try recently.

I feel this is more natural and consistent.

Yes, if we were starting from 0, that is so - but we have an implementation with long history - we would need to see the impact. [I am not against making this cleaner, but concerned about the amount of change].

I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports.

I was wondering whether some of the reports filed naming duplicate and clashing symbols could be related, but I did not analyse.

So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.

I was working on this because it was affecting my P1815 stuff - but I can use this patch as a temporary solution.

Thanks!

BTW, I suggest we file an issue first if we want to do something. So that we can avoid solving the same problem.

There are (I think your) FIXMEs in the testcases (but I do not know if there is a corresponding PR).

Oh sorry. My bad clearly. Once upon time, someone told me it is a bad idea to add FIXME in the test case... Now I understand.

The checks for internal-linkage symbols and the improvements to diagnostics do not depend on how Sema::isModuleUnitOfCurrentTU is implemented, so that I think we could fix these problems and then deal with the refactoring later.

In either case, I do not have much time to work more on this right now.

I'll try to make it.

[iains]

In D145965#4192072, @ChuanqiXu wrote:

In D145965#4192051, @iains wrote:

The checks for internal-linkage symbols and the improvements to diagnostics do not depend on how Sema::isModuleUnitOfCurrentTU is implemented, so that I think we could fix these problems and then deal with the refactoring later.

In either case, I do not have much time to work more on this right now.

I'll try to make it.

Perhaps then, we can split out the changes to Sema::isModuleUnitOfCurrentTU and see how much of the functionality is still working (with the intent that the full fix will come along later).

So if this is split into two

1. the fix to Sema::isModuleUnitOfCurrentTU (which you will be updating) and
2. the uses of that to fix the lookups and diagnostics?

(I can use both as a temporary fix .. but 2 could be applied with the existing isModuleUnitOfCurrentTU implementation).

[ChuanqiXu]

In D145965#4192147, @iains wrote:

In D145965#4192072, @ChuanqiXu wrote:

In D145965#4192051, @iains wrote:

The checks for internal-linkage symbols and the improvements to diagnostics do not depend on how Sema::isModuleUnitOfCurrentTU is implemented, so that I think we could fix these problems and then deal with the refactoring later.

In either case, I do not have much time to work more on this right now.

I'll try to make it.

Perhaps then, we can split out the changes to Sema::isModuleUnitOfCurrentTU and see how much of the functionality is still working (with the intent that the full fix will come along later).

So if this is split into two

1. the fix to Sema::isModuleUnitOfCurrentTU (which you will be updating) and
2. the uses of that to fix the lookups and diagnostics?

(I can use both as a temporary fix .. but 2 could be applied with the existing isModuleUnitOfCurrentTU implementation).

I feel it may be better to wait for me to have an initial experiment. On the one hand, if we can make it relatively quickly, we can save some time. On the other hand, now I understand that removing codes is much harder than adding codes... So let's wait for some time. The next release is in the end of July, so we have plenty time.

[iains]

In D145965#4192051, @iains wrote:

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

I have changed my mind here.
It seems that we are getting confusing decl context entries without having a proper module for the implementation (even though we will never write such a module to a PCM, since it is not importable).

I was wondering if it would be possible to avoid having separate values for PartitionInterface/Implementation (since they both have a BMI - it is only a question of whether that can be re-exported). We would have to add one bit to the PCM to deal with that, so that we might as well extend the ModuleKind enum to 4 bits.

So .. please wait for one or two days, I will try to refresh the implementation patch (D126959) and see if that resolves some of the issues I am seeing with P1815

(this does not alter the situation that you are going to revisit the isModuleUnitOfCurrentTU if that is needed - but it might not be; since having a separate module for the impl. will avoid some of the ambiguities anyway).

Additional Note: we also have a situation in the lookups for dependent names during template instantiation that the visibility of decls can need to be assessed in a different module context from the current module. So that we might also need to have a areTheseModulesFromTheSameTU(Module *A. Module *B)

[ChuanqiXu]

In D145965#4194514, @iains wrote:

In D145965#4192051, @iains wrote:

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

I have changed my mind here.
It seems that we are getting confusing decl context entries without having a proper module for the implementation (even though we will never write such a module to a PCM, since it is not importable).

I was wondering if it would be possible to avoid having separate values for PartitionInterface/Implementation (since they both have a BMI - it is only a question of whether that can be re-exported). We would have to add one bit to the PCM to deal with that, so that we might as well extend the ModuleKind enum to 4 bits.

So .. please wait for one or two days, I will try to refresh the implementation patch (D126959) and see if that resolves some of the issues I am seeing with P1815

(this does not alter the situation that you are going to revisit the isModuleUnitOfCurrentTU if that is needed - but it might not be; since having a separate module for the impl. will avoid some of the ambiguities anyway).

Additional Note: we also have a situation in the lookups for dependent names during template instantiation that the visibility of decls can need to be assessed in a different module context from the current module. So that we might also need to have a areTheseModulesFromTheSameTU(Module *A. Module *B)

Yeah. What I had in mind is to remove ModulePartitionInterface and ModulePartitionImplementation in ModuleKinds and rename ModuleInterfaceUnit to NamedModules. And we can introduce 2 bits for IsPartition and IsImplementation. Then we can introduce ImplementationUnit without any loss.

[ChuanqiXu]

I filed another issue (https://github.com/llvm/llvm-project/issues/61427) to reflect [basic.lookup.general]p2.3. So there are multiple issues. Let's handle them in multiple patches too.

[iains]

rebased, split the changes to module and private linkage out,

this needs to be refactored to collect the test functions into either
module.h and / or sema.h

[iains]

although this patch does handle the cases needed, it really needs refactoring.
posting here since we are both working in this area and this might be useful input.

[ChuanqiXu]

Just took a quick look.

clang/lib/Sema/SemaLookup.cpp

2081

We should check header units here.

2100

Let's not use isFromASTFile(). It is a low level API without higher level semantics. I think it is good enough to check the module of ND.

[iains]

rebased

[iains]

I updated this because I am going to push the latest version of the P1815 patch and that depends on the lookup changes.

clang/lib/Sema/SemaLookup.cpp

2081

The point of checking module map modules was to avoid affecting clang modules with the change in semantics.

At the moment, I am not sure why we could exclude lookup from finding decls in an imported header unit?

2100

lookup is very heavily used; the purpose of the isFromAST() check is to short-circuit the more expensive checks when we know that a decl must be in the same TU (i.e. it is not from an AST file).

If we can find a suitable inexpensive check that has better semantics, I am fine to change this,

[ChuanqiXu]

clang/lib/Sema/SemaLookup.cpp

2081

At the moment, I am not sure why we could exclude lookup from finding decls in an imported header unit?

We're not excluding decls from an imported header units. We're trying to include the decls from the headers units. Since we've allowed decls with internal linkage to appear to header units. We must need to allow decls with internal linkage in header units to be found here. Otherwise, the following example may fail:

// foo.h
static int a = 43;

// m.cppm
export module m;
import "foo.h";
use of a...

BTW, given the semantics of header units and clang modules are pretty similar, we should use isHeaderModule in most cases.

2100

It looks good enough to me to check that ND lives in a module. And from what I profiled, the lookup process is not hot really.

[iains]

clang/lib/Sema/SemaLookup.cpp

2081

ah yes, that special case, I should have remembered.
OK I will adjust this at the next iteration (I think you mean isHeaderLikeModule())

[ChuanqiXu]

Did another quick look. And I feel the title and the summary of the page is not consistent with the patch itself. I think it is better to split this to make the change to focus on the entities with internal linkage. I don't know what's wrong with the module linkage things. Maybe we can file an issue ahead in next time.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11228

The 'private' here makes me to think about module private fragment while it is not true. I prefer to refactor it to something like "it is not exported".

clang/include/clang/Sema/Sema.h

2370

While I am not a native speaker, I feel isSameModuleWithCurrentTU may be a better name.

clang/lib/Sema/SemaLookup.cpp

1784

It looks better to me if we can insert codes here

3912–3936

What's the intention for the change? And why is the current behavior bad without this?

4515

What if we don't touch the typo part? I am still confusing.

5819–5830

I feel like this can be another change. I'm a little bit confused since I feel the patch did multiple things at the same time again..

[ChuanqiXu]

clang/lib/Sema/SemaLookup.cpp

3912–3936

What's the intention for the change? And why is the current behavior bad without this?

3912–3936

Oh, I understand why I feel the code is not good since the decl with internal linkage or module linkage shouldn't be visible. So even if there are problems, we should handle them elsewhere.

[iains]

rebased, addressed review comments (patch still needs refactoring)

[iains]

in the end, we have to deal with the following cases:

1. decls in the same TU (including GMF and PMF)
2. decls in the current named module (excluding GMF)
3. decls on an instantiation path (which can include internal-linkage entities)
4. decls that are needed by an exported entity from a *differennt" named module [e.g. a module-linkage entity that is used by some exported content].

[iains]

clang/include/clang/Sema/Sema.h

2370

actually, as noted, both cases are needed. So I have made that clear by splitting the function into two.

clang/lib/Sema/SemaLookup.cpp

2100

OK, we can always revisit performance later,

5819–5830

sure, we can refactor; the current patch is a placeholder to allow work to continue on p1815.

[iains]

rebased and adjusted for upstream changes

[iains]

about the comments that the patch seems to do multiple things; I do not think we can fix the lookup without touching the typo-fixes since it uses the same underlying machinery - if we do not adjust the typo fixes, we will regress diagnostics.

[iains]

rebased, added testcase for Issue 61601.

[ChuanqiXu]

if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11228

let's try rewording private to invisible?

clang/include/clang/Sema/Sema.h

2370

Let's use !DeclBase::isInAnotherModuleUnit() instead now.

2372–2380

nit: I prefer this to be a freestanding function in Module.h. This looks slightly not good within Sema.

clang/lib/Sema/SemaLookup.cpp

3912–3936

Could we tried to address this? The change smells not so good.

[iains]

In D145965#4431846, @ChuanqiXu wrote:

if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.

Because when we do typo checking, we relax the visibility conditions so that we can see some decl that might be hidden or misspelled - then we can say

"you need to import module XX before using YY",

or

"did you mean ZZ"

(I would be happy if we did not need to do this in this patch, but not sure how we can work around it).

clang/include/clang/Basic/DiagnosticSemaKinds.td

11228

I will reword.

clang/include/clang/Sema/Sema.h

2370

'isInAnotherModuleUnit' is not precise - for this work we need to have :

"is in another module unit of the same named module"
"is in another module unit of a different named module"

so I have made two functions to do these two jobs (although they are in Sema now, we can move them either to decl or module - as suggested later)

2372–2380

what about moving all the module tu-related tests to decl.cpp/h (or maybe to module.cpp/h)?

clang/lib/Sema/SemaLookup.cpp

1784

I am not sure exactly what you mean by "insert codes"?

3912–3936

I am not sure what you mean here - I would like us to get this lookup stuff fixed for 17, so will be working on it when back in the office (traveling today)

There is a different behaviour between cases where the entry is from an named module (but not the current one) and a different TU of the same named module.

[ChuanqiXu]

In D145965#4431888, @iains wrote:

In D145965#4431846, @ChuanqiXu wrote:

if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.

Because when we do typo checking, we relax the visibility conditions so that we can see some decl that might be hidden or misspelled - then we can say

"you need to import module XX before using YY",

or

"did you mean ZZ"

(I would be happy if we did not need to do this in this patch, but not sure how we can work around it).

It is OK to see the misspelled decl. But it looks weird to me to see the hidden decls. I think such regression should be called correction.

clang/include/clang/Sema/Sema.h

2370

Sounds not bad to me.

2372–2380

I prefer moving these to Module.h since its operand is Module instead of Decl.

clang/lib/Sema/SemaLookup.cpp

1784

Oh, this is a little bit historical. I mean it looks better to do the main job of the patch (correcting the visibility of internal linkage entities) if possible.

3912–3936

I mean the we should put the similar things together as much as possible. It looks that the codes are trying to correcting the visibilities returned from isVisible(). But this sounds not good. Since it implies that the result from isVisible() is not valid.

There is a different behaviour between cases where the entry is from an named module (but not the current one) and a different TU of the same named module.

Maybe we can tried to solve this by adding other isVisible interfaces.

I would like us to get this lookup stuff fixed for 17, so will be working on it when back in the office (traveling today)

Yeah, but we still have one month. And even if we didn't get it in time. (Given the size of the patch) It is still OK to back port this before the first week of September. So we can be more relaxed.

[iains]

I will look at the rest of the comments once back in the office.

In D145965#4431929, @ChuanqiXu wrote:

In D145965#4431888, @iains wrote:

In D145965#4431846, @ChuanqiXu wrote:

if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.

Because when we do typo checking, we relax the visibility conditions so that we can see some decl that might be hidden or misspelled - then we can say

"you need to import module XX before using YY",

or

"did you mean ZZ"

(I would be happy if we did not need to do this in this patch, but not sure how we can work around it).

It is OK to see the misspelled decl. But it looks weird to me to see the hidden decls. I think such regression should be called correction.

It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way)

clang/lib/Sema/SemaLookup.cpp

1784

OK. I agree that is ideal; it remains to be seen if it is feasible.

3912–3936

I mean the we should put the similar things together as much as possible. It looks that the codes are trying to correcting the visibilities returned from isVisible(). But this sounds not good. Since it implies that the result from isVisible() is not valid.

Yes, indeed, I agree - if we were starting from scratch, we would do all this in the lower-level 'isVisible' - but since that is used in different ways by the higher levels, it seemed quite tricky to change.

There is a different behaviour between cases where the entry is from an named module (but not the current one) and a different TU of the same named module.

Maybe we can tried to solve this by adding other isVisible interfaces.

maybe that might work, I believe that when I looked before it was going to be quite complex, I will look again.

[ChuanqiXu]

It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way)

I got your point. But I prefer to implement an all-visible mode (not available for users) for such situations. And I still think it is not problem for the special case. Since this is patch is working for internal decls.

[iains]

In D145965#4431997, @ChuanqiXu wrote:

It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way)

I got your point. But I prefer to implement an all-visible mode (not available for users) for such situations. And I still think it is not problem for the special case. Since this is patch is working for internal decls.

Yes, but AFAIR we also support the user naming an internal entity and then say "you must export PP before using it".
(I am sure that these changes were needed to avoid regressing existing diagnostics).

I have some ideas now - will thin about them during travel.

[shafik]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11227

I only see declaration and definition cases covered for this diagnostic in the tests.

11231

I only see the declaration case covered for this diagnostic. Please add tests to cover the other cases.

11235

I only see the declaration and definition case covered in the tests we should cover all possible diagnostics. Especially in new code, bugs often come about on code we don't cover.

clang/lib/Sema/SemaLookup.cpp

4522

Fix to conform w/ bugprone-argument-comment

4535

4621

[shafik]

Adding clang-language-wg for more visibility.

[iains]

just rebased to support p1815 work

[iains]

rebased, fixed some format issues; again to support p1815 work only.

[iains]

changes are needed to address review comments - but this revision is needed as a parent to other work (so might need to be rebased from time to time)

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_module_private_local : Error<
   "__module_private__">;
 def err_module_private_local_class : Error<
   "local %select{struct|interface|union|class|enum}0 cannot be declared "
   "__module_private__">;
 def err_module_unimported_use : Error<
   "%select{declaration|definition|default argument|"
   "explicit specialization|partial specialization}0 of %1 must be imported "
   "from module '%2' before it is required">;
+def err_module_private_use : Error<
+  "%select{declaration|definition|default argument|"
+  "explicit specialization|partial specialization}0 of %1 is private to module "
+  "'%2'">;
+def err_module_internal_use : Error<
+  "%select{declaration|definition|default argument|"
+  "explicit specialization|partial specialization}0 of %1 is internal to "
+  "'%2'">;
+def note_suggest_export : Note<
+  "export the %select{declaration|definition|default argument|"
+  "explicit specialization|partial specialization}0 to make it available">;
 def err_module_unimported_use_header : Error<
   "%select{missing '#include'|missing '#include %3'}2; "
   "%select{||default argument of |explicit specialization of |"
   "partial specialization of }0%1 must be "
   "%select{declared|defined|defined|declared|declared}0 "
   "before it is used">;
 def err_module_unimported_use_multiple : Error<
   "%select{declaration|definition|default argument|"

clang/include/clang/Sema/Lookup.h

public:
   /// Return the base paths structure that's associated with
   /// these results, or null if none is.
   CXXBasePaths *getBasePaths() const {
     return Paths;
   }
 
   /// Determine whether the given declaration is visible to the
   /// program.
-  static bool isVisible(Sema &SemaRef, NamedDecl *D);
+  static bool isVisible(Sema &SemaRef, NamedDecl *D, bool ForTypo = false);
 
   static bool isReachable(Sema &SemaRef, NamedDecl *D);
 
   static bool isAcceptable(Sema &SemaRef, NamedDecl *D,
                            Sema::AcceptableKind Kind) {
     return Kind == Sema::AcceptableKind::Visible ? isVisible(SemaRef, D)
                                                  : isReachable(SemaRef, D);
   }

clang/include/clang/Sema/Sema.h

return ModuleScopes.empty() ? false
                                 : ModuleScopes.back().Module->isHeaderUnit();
   }
 
   /// Get the module owning an entity.
   Module *getOwningModule(const Decl *Entity) {
     return Entity->getOwningModule();
   }
 
+  // Determine whether the module M belongs to the  current TU.
+  bool isModuleUnitOfCurrentTU(const Module *M) const;
+
+  /// Determine whether the module MA is part of the same named module as MB.
+  bool arePartOfSameNamedModule(const Module *MA, const Module *MB) const {
+    if (!MA || MA->isGlobalModule())
+      return false;
+    if (!MB || MB->isGlobalModule())
+      return false;
+    return MA->getPrimaryModuleInterfaceName() ==
+           MB->getPrimaryModuleInterfaceName();
+  }
+
+  /// Determine whether the module M is part of the current named module.
+  bool isPartOfCurrentNamedModule(const Module *M) const {
+    return arePartOfSameNamedModule(getCurrentModule(), M);
+  }
+
   /// Make a merged definition of an existing hidden definition \p ND
   /// visible at the specified location.
   void makeMergedDefinitionVisible(NamedDecl *ND);
 
   bool isModuleVisible(const Module *M, bool ModulePrivate = false);
 
   // When loading a non-modular PCH files, this is used to restore module
   // visibility.

clang/lib/Sema/SemaLookup.cpp

 /// your module).
 bool LookupResult::isAcceptableSlow(Sema &SemaRef, NamedDecl *D,
                                     Sema::AcceptableKind Kind) {
   assert(!D->isUnconditionallyVisible() &&
          "should not call this: not in slow case");
 
   Module *DeclModule = SemaRef.getOwningModule(D);
   assert(DeclModule && "hidden decl has no owning module");
 
-  // If the owning module is visible, the decl is acceptable.
+  // If the owning module is visible, the decl is potentially acceptable.
   if (SemaRef.isModuleVisible(DeclModule,
                               D->isInvisibleOutsideTheOwningModule()))
     return true;
 
   // Determine whether a decl context is a file context for the purpose of
   // visibility/reachability. This looks through some (export and linkage spec)
   // transparent contexts, but not others (enums).
   auto IsEffectivelyFileContext = [](const DeclContext *DC) {
if (auto *ND = dyn_cast<NamespaceDecl>(D)) {
     if (Acceptable)
       getSema().VisibleNamespaceCache.insert(std::make_pair(Key, Acceptable));
     return Acceptable;
   }
 
   return findAcceptableDecl(getSema(), D, IDNS);
 }
 
-bool LookupResult::isVisible(Sema &SemaRef, NamedDecl *D) {
+bool LookupResult::isVisible(Sema &SemaRef, NamedDecl *D, bool ForTypo) {
   // If this declaration is already visible, return it directly.
   if (D->isUnconditionallyVisible())
     return true;
 
   // During template instantiation, we can refer to hidden declarations, if
   // they were visible in any module along the path of instantiation.
-  return isAcceptableSlow(SemaRef, D, Sema::AcceptableKind::Visible);
+  if (!isAcceptableSlow(SemaRef, D, Sema::AcceptableKind::Visible))
+    return false;
+
+  if (ForTypo && SemaRef.getLangOpts().CPlusPlusModules) {
+    // There is no point in offering a typo correction to an item that we will
+    // reject because it has internal linkage and is in a different module TU;
+    Module *DeclModule = SemaRef.getOwningModule(D);
+    if (DeclModule && !DeclModule->isHeaderLikeModule() &&
+        !SemaRef.isModuleUnitOfCurrentTU(DeclModule) &&
+        (D->getFormalLinkage() == Linkage::InternalLinkage ||
+         DeclModule->isPrivateModule()))
+      return false; // This is not usefully visible.
+  }
+  return true;
 }
 
 bool LookupResult::isReachable(Sema &SemaRef, NamedDecl *D) {
   if (D->isUnconditionallyVisible())
     return true;
 
   return isAcceptableSlow(SemaRef, D, Sema::AcceptableKind::Reachable);
 }
 
 bool LookupResult::isAvailableForLookup(Sema &SemaRef, NamedDecl *ND) {
   // We should check the visibility at the callsite already.
-  if (isVisible(SemaRef, ND))
+  if (isVisible(SemaRef, ND)) {
+    if (SemaRef.getLangOpts().CPlusPlusModules) {
+      // The module that owns the decl is visible; However
+      // [basic.lookup.general]/2, specifically 2.3
+      // If the owning module is part of a different TU than the current one,
+      // and declaration is internal (or from the PMF), then we should not
+      // include it.
+      Module *DeclModule = SemaRef.getOwningModule(ND);
+      if (DeclModule && !DeclModule->isHeaderLikeModule() &&
+          !SemaRef.isModuleUnitOfCurrentTU(DeclModule) &&
+          (ND->getFormalLinkage() == Linkage::InternalLinkage ||
+           DeclModule->isPrivateModule()))
+        return false;
+    }
     return true;
+  }
 
   // Deduction guide lives in namespace scope generally, but it is just a
   // hint to the compilers. What we actually lookup for is the generated member
   // of the corresponding template. So it is sufficient to check the
   // reachability of the template decl.
   if (auto *DeductionGuide = ND->getDeclName().getCXXDeductionGuideTemplate())
     return SemaRef.hasReachableDefinition(DeductionGuide);
 
for (auto *D : R) {
 
       // The declaration is visible to argument-dependent lookup if either
       // it's ordinarily visible or declared as a friend in an associated
       // class.
       bool Visible = false;
       for (D = D->getMostRecentDecl(); D;
            D = cast_or_null<NamedDecl>(D->getPreviousDecl())) {
         if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
-          if (isVisible(D)) {
-            Visible = true;
+          Visible = isVisible(D);
+          if (!getLangOpts().CPlusPlusModules) {
+            if (Visible)
               break;
+            continue;
           }
 
-          if (!getLangOpts().CPlusPlusModules)
+          Module *FM = D->getOwningModule();
+          if (Visible) {
+            if (!FM)
+              break;
+            assert(Underlying->hasLinkage() && "an imported func with no linkage?");
+            // Unless the module is a defining one for the
+            bool Ovr = true;
+            for (unsigned I = 0; I < CodeSynthesisContexts.size(); ++I) {
+              Module *M = CodeSynthesisContexts[I].Entity
+                              ? getDefiningModule(
+                                    *this, CodeSynthesisContexts[I].Entity)
+                              : nullptr;
+              if (FM == M) {
+                Ovr = false;
+                break;
+              }
+            }
+            if (Ovr && Underlying->getFormalLinkage() == Linkage::InternalLinkage &&
+                !isModuleUnitOfCurrentTU(FM)) {
+              Visible = false;
+              continue;
+            }
+            if (Ovr && Underlying->getFormalLinkage() == Linkage::ModuleLinkage &&
+                !isPartOfCurrentNamedModule(FM)) {
+              Visible = false;
               continue;
+            }
+            break;
+          }
 
           if (D->isInExportDeclContext()) {
-            Module *FM = D->getOwningModule();
             // C++20 [basic.lookup.argdep] p4.3 .. are exported ...
             // exports are only valid in module purview and outside of any
             // PMF (although a PMF should not even be present in a module
             // with an import).
             assert(FM && FM->isModulePurview() && !FM->isPrivateModule() &&
                    "bad export context");
             // .. are attached to a named module M, do not appear in the
             // translation unit containing the point of the lookup..
static void LookupPotentialTypoResult(Sema &SemaRef,
                                       DeclContext *MemberContext,
                                       bool EnteringContext,
                                       bool isObjCIvarLookup,
                                       bool FindHidden);
 
 /// Check whether the declarations found for a typo correction are
 /// visible. Set the correction's RequiresImport flag to true if none of the
 /// declarations are visible, false otherwise.
 static void checkCorrectionVisibility(Sema &SemaRef, TypoCorrection &TC) {
   TypoCorrection::decl_iterator DI = TC.begin(), DE = TC.end();
 
-  for (/**/; DI != DE; ++DI)
-    if (!LookupResult::isVisible(SemaRef, *DI))
+  // During the lookups for Typo corrections, we allow 'hidden' decls to be
+  // found, this can also include (potential typo correction) decls that are
+  // not in their own right viable.  Filter these from the visible set.
+  for (/**/; DI != DE; ++DI) {
+    if (!LookupResult::isVisible(SemaRef, *DI, /*ForTypo*/ true))
       break;
+  }
   // No filtering needed if all decls are visible.
   if (DI == DE) {
     TC.setRequiresImport(false);
     return;
   }
 
   llvm::SmallVector<NamedDecl*, 4> NewDecls(TC.begin(), DI);
   bool AnyVisibleDecls = !NewDecls.empty();
 
   for (/**/; DI != DE; ++DI) {
-    if (LookupResult::isVisible(SemaRef, *DI)) {
+    if (LookupResult::isVisible(SemaRef, *DI, /*ForTypo*/ true)) {
       if (!AnyVisibleDecls) {
         // Found a visible decl, discard all hidden ones.
         AnyVisibleDecls = true;
         NewDecls.clear();
       }
       NewDecls.push_back(*DI);
-    } else if (!AnyVisibleDecls && !(*DI)->isModulePrivate())
-      NewDecls.push_back(*DI);
+    } else if (!AnyVisibleDecls) {
+      // We do not want to push a hidden decl if it is not usefully reported
+      // as a typo (which means the proposed replacement has to be viable).
+      NamedDecl *ND = *DI;
+      if (SemaRef.getLangOpts().CPlusPlusModules && ND->isFromASTFile() &&
+          ND->getOwningModule() &&
+          !SemaRef.isModuleUnitOfCurrentTU(ND->getOwningModule()) &&
+          ND->getFormalLinkage() == Linkage::InternalLinkage)
+        continue; // Internal symbol from a different TU, not visible.
+      if (!ND->isModulePrivate())
+        NewDecls.push_back(ND);
+    }
   }
 
   if (NewDecls.empty())
     TC = TypoCorrection();
   else {
     TC.setCorrectionDecls(NewDecls);
     TC.setRequiresImport(!AnyVisibleDecls);
   }
void TypoCorrectionConsumer::FoundDecl(NamedDecl *ND, NamedDecl *Hiding,
   // special names (constructors, overloaded operators, selectors,
   // etc.).
   IdentifierInfo *Name = ND->getIdentifier();
   if (!Name)
     return;
 
   // Only consider visible declarations and declarations from modules with
   // names that exactly match.
-  if (!LookupResult::isVisible(SemaRef, ND) && Name != Typo)
+  if (!LookupResult::isVisible(SemaRef, ND, /*ForTypo*/ true) && Name != Typo)
     return;
 
   FoundName(Name->getName());
 }
 
 void TypoCorrectionConsumer::FoundName(StringRef Name) {
   // Compute the edit distance between the typo and the name of this
   // entity, and add the identifier to the list of results.
if (!HeaderName.empty() || UniqueModules.empty()) {
     NotePrevious();
     if (Recover)
       createImplicitModuleImportForErrorRecovery(UseLoc, Modules[0]);
     return;
   }
 
   Modules = UniqueModules;
 
+  bool DidFixit = false;
   if (Modules.size() > 1) {
     std::string ModuleList;
     unsigned N = 0;
     for (const auto *M : Modules) {
       ModuleList += "\n        ";
       if (++N == 5 && N != Modules.size()) {
         ModuleList += "[...]";
         break;
       }
       ModuleList += M->getFullModuleName();
     }
 
     Diag(UseLoc, diag::err_module_unimported_use_multiple)
       << (int)MIK << Decl << ModuleList;
+  } else if (Decl->hasLinkage() &&
+             Decl->getFormalLinkage() == Linkage::ModuleLinkage) {
+    Diag(UseLoc, diag::err_module_private_use)
+        << (int)MIK << Decl << Modules[0]->getFullModuleName();
+    Diag(Decl->getBeginLoc(), diag::note_suggest_export)
+        << (int)MIK
+        << FixItHint::CreateInsertion(Decl->getBeginLoc(), "export");
+    DidFixit = true;
+  } else if (Decl->hasLinkage() &&
+             Decl->getFormalLinkage() == Linkage::InternalLinkage) {
+    Diag(UseLoc, diag::err_module_internal_use)
+        << (int)MIK << Decl << Modules[0]->getFullModuleName();
   } else {
     // FIXME: Add a FixItHint that imports the corresponding module.
     Diag(UseLoc, diag::err_module_unimported_use)
       << (int)MIK << Decl << Modules[0]->getFullModuleName();
   }
 
+  if (!DidFixit)
     NotePrevious();
 
   // Try to recover by implicitly importing this module.
   if (Recover)
     createImplicitModuleImportForErrorRecovery(UseLoc, Modules[0]);
 }
 
 /// Diagnose a successfully-corrected typo. Separated from the correction
 /// itself to allow external validation of the result, etc.

clang/lib/Sema/SemaModule.cpp

if (!getLangOpts().CurrentModule.empty() &&
     return nullptr;
   }
   const_cast<LangOptions&>(getLangOpts()).CurrentModule = ModuleName;
 
   auto &Map = PP.getHeaderSearchInfo().getModuleMap();
   Module *Mod;                 // The module we are creating.
   Module *Interface = nullptr; // The interface for an implementation.
   switch (MDK) {
   case ModuleDeclKind::Interface:
   case ModuleDeclKind::PartitionInterface: {
     // We can't have parsed or imported a definition of this module or parsed a
     // module map defining it already.
     if (auto *M = Map.findModule(ModuleName)) {
       Diag(Path[0].second, diag::err_module_redefinition) << ModuleName;
       if (M->DefinitionLoc.isValid())
         Diag(M->DefinitionLoc, diag::note_prev_module_definition);
       else if (OptionalFileEntryRef FE = M->getASTFile())
         Diag(M->DefinitionLoc, diag::note_prev_module_definition_from_ast_file)

 }
 
 void Sema::PopImplicitGlobalModuleFragment() {
   assert(!ModuleScopes.empty() &&
          getCurrentModule()->isImplicitGlobalModule() &&
          "left the wrong module scope, which is not global module fragment");
   ModuleScopes.pop_back();
 }
+
+bool Sema::isModuleUnitOfCurrentTU(const Module *M) const {
+  assert(M);
+
+  Module *CurrentModuleUnit = getCurrentModule();
+
+  // If we are not in a module currently, M must not be the module unit of
+  // current TU.
+  if (!CurrentModuleUnit)
+    return false;
+
+  return M->isSubModuleOf(CurrentModuleUnit->getTopLevelModule());
+}

clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp

void test_late() {
   // expected-note@* {{not visible}}
 
   global_module_fragment = 1; // expected-error {{missing '#include'; 'global_module_fragment' must be declared before it is used}}
 
   exported = 1;
 
   not_exported = 1;
 #ifndef IMPLEMENTATION
-  // expected-error@-2 {{declaration of 'not_exported' must be imported from module 'A' before it is required}}
-  // [e-mail]:19 {{declaration here is not visible}}
+  // expected-error@-2 {{declaration of 'not_exported' is private to module 'A'}}
+  // [e-mail]:19 {{export the declaration to make it available}}
 #endif
 
-  internal = 1;
-#ifndef IMPLEMENTATION
-  // expected-error@-2 {{declaration of 'internal' must be imported from module 'A' before it is required}}
-  // [e-mail]:20 {{declaration here is not visible}}
-#endif
+  internal = 1; // expected-error {{use of undeclared identifier 'internal'}}
 
-  not_exported_private = 1;
-#ifndef IMPLEMENTATION
-  // FIXME: should not be visible here
-  // expected-error@-3 {{undeclared identifier}}
-#endif
+  not_exported_private = 1; // expected-error {{use of undeclared identifier 'not_exported_private'}}
 
-  internal_private = 1;
-#ifndef IMPLEMENTATION
-  // FIXME: should not be visible here
-  // expected-error@-3 {{undeclared identifier}}
-#endif
+  internal_private = 1; // expected-error {{undeclared identifier 'internal_private'}}
 }
 
 #endif

clang/test/CXX/module/basic/basic.def.odr/p4.cppm

 //--- module.cpp
 
 // CHECK-DAG: @_ZW6Module19extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module19inline_var_exported = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module18const_var_exported = available_externally {{(dso_local )?}}constant i32 3,
 //
 // CHECK-DAG: @_ZW6Module25extern_var_module_linkage = external {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module25inline_var_module_linkage = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZL25static_var_module_linkage = internal {{(dso_local )?}}global i32 0,
 // CHECK-DAG: @_ZW6Module24const_var_module_linkage = available_externally {{(dso_local )?}}constant i32 3,
 
 module Module;
 
 void use() {
   // CHECK: define linkonce_odr {{.*}}@_ZW6Module20used_inline_exportedv
   used_inline_exported();
   // CHECK: declare {{.*}}@_ZW6Module18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
   (void)&const_var_exported;
 
   // CHECK: define linkonce_odr {{.*}}@_ZW6Module26used_inline_module_linkagev
   used_inline_module_linkage();
 
   // CHECK: declare {{.*}}@_ZW6Module24noninline_module_linkagev
   noninline_module_linkage();
 
   (void)&extern_var_module_linkage;
   (void)&inline_var_module_linkage;
+  (void)&const_var_module_linkage;
 
-  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
-  // should not be not visible here.
-  (void)&static_var_module_linkage; // FIXME: Should not be visible here.
-
-  (void)&const_var_module_linkage; // FIXME: will be visible after P2788R0
+  // Internal-linkage declarations are not visible here.
 }
 
 //--- user.cpp
 
 // CHECK-DAG: @_ZW6Module19extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module19inline_var_exported = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module18const_var_exported = available_externally {{(dso_local )?}}constant i32 3
 
 import Module;
 
 void use() {
   // CHECK: define linkonce_odr {{.*}}@_ZW6Module20used_inline_exportedv
   used_inline_exported();
   // CHECK: declare {{.*}}@_ZW6Module18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
   (void)&const_var_exported;
 
-  // Internal-linkage declarations are not visible here.
   // Module-linkage declarations are not visible here.
+  // Internal-linkage declarations are not visible here.
 }

clang/test/CXX/module/basic/basic.link/p2.cppm

void use() {
   (void)module_linkage_class{};
   (void)internal_linkage_class{};
   (void)external_linkage_var;
   (void)module_linkage_var;
   (void)internal_linkage_var;
 }
 
 //--- M.cpp
-
 module M;
 
 void use_from_module_impl() {
   external_linkage_fn();
   module_linkage_fn();
-  internal_linkage_fn(); // expected-error {{no matching function for call to 'internal_linkage_fn'}}
+  internal_linkage_fn(); // expected-error {{use of undeclared identifier 'internal_linkage_fn'; did you mean 'external_linkage_fn'}}
+                         // [e-mail]:8 {{'external_linkage_fn' declared here}}
   (void)external_linkage_class{};
   (void)module_linkage_class{};
+  (void)internal_linkage_class;  // expected-error {{use of undeclared identifier 'internal_linkage_class'}}
   (void)external_linkage_var;
   (void)module_linkage_var;
-
-  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
-  // should not be not visible here.
-  (void)internal_linkage_class{};
-  (void)internal_linkage_var;
+  (void)internal_linkage_var; // expected-error {{use of undeclared identifier 'internal_linkage_var'}}
 }
 
 //--- user.cpp
 import M;
 
 void use_from_module_impl() {
   external_linkage_fn();
-  module_linkage_fn();   // expected-error {{declaration of 'module_linkage_fn' must be imported}}
-  internal_linkage_fn(); // expected-error {{declaration of 'internal_linkage_fn' must be imported}}
+  module_linkage_fn();   // expected-error {{declaration of 'module_linkage_fn' is private to module 'M'}}
+                         // [e-mail]:9 {{export the declaration to make it available}}
+  internal_linkage_fn(); // expected-error {{use of undeclared identifier 'internal_linkage_fn'; did you mean 'external_linkage_fn'}}
   (void)external_linkage_class{};
   (void)module_linkage_class{}; // expected-error {{undeclared identifier}} expected-error 0+{{}}
   (void)internal_linkage_class{}; // expected-error {{undeclared identifier}} expected-error 0+{{}}
-  // [e-mail]:9 {{declaration here is not visible}}
-  // [e-mail]:10 {{declaration here is not visible}}
+  // [e-mail]:8 {{'external_linkage_fn' declared here}}
   (void)external_linkage_var;
   (void)module_linkage_var; // expected-error {{undeclared identifier}}
   (void)internal_linkage_var; // expected-error {{undeclared identifier}}
 }

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

 //--- M.cppm
 export module M;
 import :impl;
 export A f();
 
 //--- Use.cpp
 import M;
 void test() {
-  A a; // expected-error {{declaration of 'A' must be imported from module 'M:impl'}}
-       // expected-error@-1 {{definition of 'A' must be imported from module 'M:impl'}} expected-error@-1 {{}}
-       // [e-mail]:2 {{declaration here is not visible}}
-       // [e-mail]:2 {{definition here is not reachable}} [e-mail]:2 {{}}
+  A a; // expected-error {{declaration of 'A' is private to module 'M:impl'}}
+       // [e-mail]:2 {{export the declaration to make it available}}
+       // expected-error@-2 1+{{definition of 'A' is private to module 'M:impl'}}
+       // [e-mail]:2 1+{{export the definition to make it available}}
 }
 
 //--- UseInPartA.cppm
 // expected-no-diagnostics
 export module M:partA;
 import :impl;
 void test() {
   A a;
 }
 
 //--- UseInAnotherModule.cppm
 export module B;
 import M;
 void test() {
-  A a; // expected-error {{declaration of 'A' must be imported from module 'M:impl'}}
-       // expected-error@-1 {{definition of 'A' must be imported from module 'M:impl'}} expected-error@-1 {{}}
-       // [e-mail]:2 {{declaration here is not visible}}
-       // [e-mail]:2 {{definition here is not reachable}} [e-mail]:2 {{}}
+  A a; // expected-error {{declaration of 'A' is private to module 'M:impl'}}
+       // [e-mail]:2 {{export the declaration to make it available}}
+       // expected-error@-2 1+{{definition of 'A' is private to module 'M:impl'}}
+       // [e-mail]:2 1+{{export the definition to make it available}}
 }
 
 //--- Private.cppm
 export module A;
 module :private;
 
 class A {};
 void test() {

clang/test/CXX/module/module.interface/p2.cpp

 int use_header() { return foo + bar::baz(); }
 
 #elif defined(USER)
 import p2;
 import "header.h";
 
 void use() {
   // namespace A is implicitly exported by the export of A::g.
-  A::f(); // expected-error {{declaration of 'f' must be imported from module 'p2' before it is required}}
-          // expected-note@* {{declaration here is not visible}}
+  A::f(); // expected-error {{declaration of 'f' is private to module 'p2'}}
+          // expected-note@* {{export the declaration to make it available}}
   A::g();
-  A::h();                   // expected-error {{declaration of 'h' must be imported from module 'p2' before it is required}}
-                            // expected-note@* {{declaration here is not visible}}
+  A::h();                   // expected-error {{declaration of 'h' is private to module 'p2'}}
+                            // expected-note@* {{export the declaration to make it available}}
   using namespace A::inner; // expected-error {{declaration of 'inner' must be imported from module 'p2' before it is required}}
                             // expected-note@* {{declaration here is not visible}}
 
   // namespace B and B::inner are explicitly exported
   using namespace B;
   using namespace B::inner;
-  B::f(); // expected-error {{declaration of 'f' must be imported from module 'p2' before it is required}}
-          // expected-note@* {{declaration here is not visible}}
-  f();    // expected-error {{declaration of 'f' must be imported from module 'p2' before it is required}}
-          // expected-note@* {{declaration here is not visible}}
+  B::f(); // expected-error {{declaration of 'f' is private to module 'p2'}}
+          // expected-note@* {{export the declaration to make it available}}
+  f();    // expected-error {{declaration of 'f' is private to module 'p2'}}
+          // expected-note@* {{export the declaration to make it available}}
 
   // namespace C is not exported
   using namespace C; // expected-error {{declaration of 'C' must be imported from module 'p2' before it is required}}
                      // expected-note@* {{declaration here is not visible}}
 
   // namespace D is exported, but D::f is not
-  D::f(); // expected-error {{declaration of 'f' must be imported from module 'p2' before it is required}}
-          // expected-note@* {{declaration here is not visible}}
+  D::f(); // expected-error {{declaration of 'f' is private to module 'p2'}}
+          // expected-note@* {{export the declaration to make it available}}
 }
 
 int use_header() { return foo + bar::baz(); }
 
 #else
 #error unknown mode
 #endif

clang/test/CXX/module/module.interface/p7.cpp

void test() {
   typename decltype(reachable)::using_type h;
   typename decltype(reachable)::template templ_using_type<int> j;
   auto value = reachable();
 }
 
 void test2() {
   auto a = E1::e1;               // OK, namespace-scope name E1 is visible and e1 is reachable
   auto b = e1;                   // OK, namespace-scope name e1 is visible
-  auto c = E2::e2;               // expected-error {{declaration of 'E2' must be imported from module}}
-                                 // expected-note@* {{declaration here is not visible}}
+  auto c = E2::e2;               // expected-error {{declaration of 'E2' is private to module 'p7'}}
+                                 // expected-note@* {{export the declaration to make it available}}
   auto d = e2;                   // should be error, namespace-scope name e2 is not visible
   auto e = E2U::e2;              // OK, namespace-scope name E2U is visible and E2::e2 is reachable
-  auto f = E3::e3;               // expected-error {{declaration of 'E3' must be imported from module 'p7' before it is required}}
-                                 // expected-note@* {{declaration here is not visible}}
+  auto f = E3::e3;               // expected-error {{declaration of 'E3' is private to module 'p7'}}
+                                 // expected-note@* {{export the declaration to make it available}}
   auto g = e3;                   // should be error, namespace-scope name e3 is not visible
   auto h = decltype(func())::e3; // OK, namespace-scope name f is visible and E3::e3 is reachable
 }

clang/test/CXX/module/module.reach/ex1.cpp

 export import :A;
 import :B;
 B b2;
 export void f(B b = B());
 
 //--- X.cppm
 export module X;
 import M;
-B b3; // expected-error {{definition of 'B' must be imported from module 'M:B' before it is required}} expected-error {{}}
-      // expected-note@* {{definition here is not reachable}} expected-note@* {{}}
-// FIXME: We should emit an error for unreachable definition of B.
+B b3; // expected-error {{definition of 'B' is private to module 'M:B'}} expected-error {{}}
+      // expected-note@* {{export the definition to make it available}} expected-note@* {{}}
+
 void g() { f(); }
-void g1() { f(B()); } // expected-error 1+{{definition of 'B' must be imported from module 'M:B' before it is required}}
-                      // expected-note@* 1+{{definition here is not reachable}}
+void g1() { f(B()); } // expected-error 1+{{definition of 'B' is private to module 'M:B'}}
+                      // expected-note@* 1+{{export the definition to make it available}}
                       // [e-mail]:5 {{passing argument to parameter 'b' here}}

clang/test/CXX/module/module.reach/p2.cpp

 //--- M.cppm
 export module M;
 import :impl;
 export A f();
 
 //--- UseStrict.cpp
 import M;
 void test() {
-  auto a = f(); // expected-error {{definition of 'A' must be imported from module 'M:impl' before it is required}} expected-error{{}}
-                // expected-note@* {{definition here is not reachable}} expected-note@* {{}}
+  auto a = f(); // expected-error 1+{{definition of 'A' is private to module 'M:impl'}}
+                // expected-note@* 1+{{export the definition to make it available}}
 }

clang/test/CXX/module/module.reach/p5.cpp

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: split-file %s %t
 //
 // RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
 // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/B.cppm -fsyntax-only -verify
 //
 //--- A.cppm
 export module A;
 struct X {};
 export using Y = X;
 
 //--- B.cppm
 export module B;
 import A;
 Y y; // OK, definition of X is reachable
-X x; // expected-error {{declaration of 'X' must be imported from module 'A' before it is required}}
-     // expected-note@* {{declaration here is not visible}}
+X x; // expected-error {{declaration of 'X' is private to module 'A'}}
+     // expected-note@* {{export the declaration to make it available}}

clang/test/Modules/Reachability-template-default-arg.cpp

 struct A {
   T a;
 };
 
 //--- Use.cpp
 import template_default_arg;
 void bar() {
   A<> a0;
-  A<t> a1; // expected-error {{declaration of 't' must be imported from module 'template_default_arg' before it is required}}
-           // expected-note@* {{declaration here is not visible}}
+  A<t> a1; // expected-error {{declaration of 't' is private to module 'template_default_arg'}}
+           // expected-note@* {{export the declaration to make it available}}
 }

clang/test/Modules/cxx20-10-1-ex2.cpp

 //--- std10-1-ex2-tu5.cpp
 module B;   // implicitly imports B
 int &c = n; // OK
 // expected-no-diagnostics
 
 //--- std10-1-ex2-tu6.cpp
 import B;
 // error, n is module-local and this is not a module.
-int &c = n; // expected-error {{declaration of 'n' must be imported}}
-            // expected-note@* {{declaration here is not visible}}
+int &c = n; // expected-error {{declaration of 'n' is private to module 'B'}}
+            // expected-note@* {{export the declaration to make it available}}
 
 //--- std10-1-ex2-tu7.cpp
 // expected-no-diagnostics
 module B:X3; // does not implicitly import B
 import :X2; // X2 is an implementation unit import B.
 // According to [module.import]p7:
 //   Additionally, when a module-import-declaration in a module unit of some
 //   module M imports another module unit U of M, it also imports all
 //   translation units imported by non-exported module-import-declarations in
 //   the module unit purview of U.
 //
 // So B is imported in B:X3 due to B:X2 imported B. So n is visible here.
 int &c = n;

clang/test/Modules/deduction-guide3.cppm

 };
 
 template<typename T>
 Templ(T t) -> Templ<T>;
 
 //--- Use.cpp
 import Templ;
 void func() {
-    Templ t(5); // expected-error {{declaration of 'Templ' must be imported from module 'Templ' before it is required}}
+    Templ t(5); // expected-error {{declaration of 'Templ' is private to module 'Templ'}}
                 // expected-error@-1 {{unknown type name 'Templ'}}
-                // [e-mail]:3 {{declaration here is not visible}}
+                // [e-mail]:3 {{export the declaration to make it available}}
 }
 

clang/test/Modules/diagnose-missing-import.m

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/diagnose-missing-import \
 // RUN:   -Werror=implicit-function-declaration -fsyntax-only \
 // RUN:   -fimplicit-module-maps -verify %s
 @import NCI;
 
 void foo(void) {
   XYZLogEvent(xyzRiskyCloseOpenParam, xyzRiskyCloseOpenParam); // expected-error {{call to undeclared function 'XYZLogEvent'; ISO C99 and later do not support implicit function declarations}} \
-                                                                  expected-error {{declaration of 'XYZLogEvent' must be imported}} \
-                                                                  expected-error {{declaration of 'xyzRiskyCloseOpenParam' must be imported from module 'NCI.A'}} \
-                                                                  expected-error {{declaration of 'xyzRiskyCloseOpenParam' must be imported from module 'NCI.A'}}
+                                                                  expected-error {{declaration of 'XYZLogEvent' is internal to 'NCI.A'}} \
+                                                                  expected-error {{declaration of 'xyzRiskyCloseOpenParam' is internal to 'NCI.A'}} \
+                                                                  expected-error {{declaration of 'xyzRiskyCloseOpenParam' is internal to 'NCI.A'}}
 }
 
 // expected-note@Inputs/diagnose-missing-import/a.h:5 {{declaration here is not visible}}
 // expected-note@Inputs/diagnose-missing-import/a.h:5 {{declaration here is not visible}}
 // expected-note@Inputs/diagnose-missing-import/a.h:6 {{declaration here is not visible}}
 

clang/test/Modules/pr61601.cpp

+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// All of the following should build without diagnostics.
+//
+// RUN: %clang_cc1 -std=c++20 %t/Part1.cpp -emit-module-interface -o %t/TheMod-Part1.pcm
+//
+// RUN: %clang_cc1 -std=c++20 %t/Part2.cpp -emit-module-interface -o %t/TheMod-Part2.pcm
+//
+// RUN: %clang_cc1 -std=c++20 %t/TheMod.cpp -emit-module-interface -o %t/TheMod.pcm \
+// RUN: -fprebuilt-module-path=%t
+
+//--- Part1.cpp
+
+export module TheMod:Part1;
+static int loog = 1;
+
+//--- Part2.cpp
+
+export module TheMod:Part2;
+static int loog = 2;
+
+//--- TheMod.cpp
+
+export module TheMod;
+export import :Part1;
+export import :Part2;
+
+static int loog = 3;
+export int V = loog;