[C++20] [Modules] Judge isInCurrentModule currently

[ChuanqiXu]

Summary

Now the implementation would accept following code:

//--- impl.cppm
module M:impl;
class A {};

//--- M.cppm
export module M;
import :impl;

//--- Use.cpp
import M;
void test() {
    A a; // Expected error. A is not visible here.
}

which is clearly wrong. The root cause is the implementation of isInCurrentModule would return true if the module is a partition! So in the above example, although Use.cpp is not a module unit, isInCurrentModule would still return true when the compiler tries to see if the owning module of A is the current module. I believe this is an oversight. And we could fix it simply by comparing the primary module name.

[ChuanqiXu]

Fix tests

[iains]

clang/include/clang/Basic/Module.h

537–543

I do not understand the purpose of this change, we already iterated over making the function more efficient.

For C++20 modules there cannot be more than one parent - so that the nested call to getTopLevelModule(), via getTopLevelModuleName() is redundant.

Is there some other case that needs special handling?
(If so then I think we should guard this at a higher level)

clang/lib/Sema/SemaLookup.cpp

1572–1573

... of course, "correctness before optimisation", but this test function seems like it would benefit from some refactoring as suggested below.

it seems a bit unfortunate that we are placing a string comparison in the "acceptable decl" lookup path, which might be made many times (the previous use of the string compare was in making the module decl - which only happens once in a TU).

• Sema has visibility of the import graph of modules (via DirectModuleImports and Exports).
• We also know what the module parse state is (FirstDecl, GMF etc)
• If were are not parsing a module (i.e. LangOpts.CurrentModule.empty()) the we could return false early?
• if ModuleScopes is empty we know we are not (yet) parsing a module
• Once we are parsing a module then we can test ModuleScopes.back() to find the current module

there are only two users of isInCurrentModule() - so maybe we refactor could make more use of the higher level state information - and / or cache information on the parents of partitions to avoid the complexity of parsing strings each time.

what do you think?

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

62–69

@rsmith

so, I wonder if this is intended, it seems to defeat the mechanisms to avoid circular dependencies.

TU1:
module M : Impl; // this is said not to generate a circular dependency because M : Impl does not implicitly import M

import M; // but now we've done it manually ... and if that module is implicitly exported to importers of the Impl. module interface....
....

TU2:

export module M;
import : Impl;

// ... then boom! we have a circular dep.

[ChuanqiXu]

Address comments:

• Add Cache in isInCurrentModule to avoid too many string comparisons.

[ChuanqiXu]

clang/include/clang/Basic/Module.h

537–543

The primary purpose of the change is that we need to get the primary module name for getLangOpts().CurrentModule. So we need a static member function which takes a StringRef.

Another point might be the correctness. For example, for the private module fragment, the current implementation would return the right result while the original wouldn't. (Although we didn't use private module fragment much...)

For the most case, I admit the current implementation would call more function a little more. But I think it is negligible.

clang/lib/Sema/SemaLookup.cpp

1572–1573

Yeah, string comparison is relatively expensive and we should avoid that. I added a search cache here and I thought it should handle the most cases.

For the solution you described, I am not sure if I understood correctly. It looks like a quick return path if we found we are not parsing a module? If yes, I think the current check could do similar works.

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

62–69

@rsmith should be the right one to answer this.

Personally, I feel the case you described should be avoided. And I found it is hard to right cyclic module dependent code in current fashion. Since when I compile TU1, the compiler suggests that it failed to found M. And when I compile TU2, the compiler suggests that it failed to found M:impl... So it looks like we could avoid worrying this now. Maybe the build system could give better diagnostic message in the future. I think this might not be a big problem.

[iains]

clang/include/clang/Basic/Module.h

537–543

(Maybe I am worrying too much - but it does seem that we have quite some complexity in an operation that we expect to repeat many times per TU).

---

The primary purpose of the change is that we need to get the primary module name for getLangOpts().CurrentModule. So we need a static member function which takes a StringRef.

That is, of course, fine (and we are stuck in the case that we are building a partition, with the fact that the primary module is not available). It seems that there's not a lot we can do there.

When we have module, we could cache the result of the split, tho - so have a "PrimaryModuleName" entry in each module and populate it on the first use. We are still stuck with the string comparison - but at least we could avoid repeating the split?
As I noted, the reason we did not bother to do this in the first use was that that use was only once per TU).

When we are building a consumer of a module, on the other hand, we *do* have the complete module import graph.

Another point might be the correctness. For example, for the private module fragment, the current implementation would return the right result while the original wouldn't. (Although we didn't use private module fragment much...)

Of course, we must fix correctness issues ..
Since there is only GMF and PMF, perhaps we could reasonably treat those specially if (isGlobalModule()) .. and/or if (isPrivateModule())?

For the most case, I admit the current implementation would call more function a little more. But I think it is negligible.

It is hard to judge at the moment - perhaps we should carry on with what we have and then instrument some reasonable body of code?

clang/lib/Sema/SemaLookup.cpp

1572–1573

That looks good.

On the actual test itself:

1/
we have the check of (M->isGlobalModule() && !M->Parent)

So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
if M->isGlobalModule() && M->Parent ?
it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?

2/
When we are actually parsing the PMF we have a similar case, no?
(so that there is no Parent pointer set yet, and I think that means that we would need to examine ModuleScopes to find the Parent module?)

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

62–69

Right, one cannot actually build the faulty module (actually, in the same way as one cannot build example 3 from section 10.3 of the std).

So probably my concern is unfounded - it's just something the user will have to figure out - we are not required to diagnose the situation (and that would be hard to do for the compiler anyway).

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/include/clang/Basic/Module.h

537–543

(Maybe I am worrying too much - but it does seem that we have quite some complexity in an operation that we expect to repeat many times per TU).

Never mind! This is the meaning of reviewing.

Of course, we must fix correctness issues ..

Since there is only GMF and PMF, perhaps we could reasonably treat those specially if (isGlobalModule()) .. and/or if (isPrivateModule())?

Done.

After consideration, I think what you here makes sense. Especially now Module::getPrimaryModuleInterfaceName is only used for Sema.getLangOpts().CurrentModule. So it might be better to handle this in Sema or LangOptions.

549–550

I thought to add an assertion here. But I feel like it is not so necessary and friendly.

clang/lib/Sema/SemaLookup.cpp

1572–1573

1/
we have the check of (M->isGlobalModule() && !M->Parent)

So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
if M->isGlobalModule() && M->Parent ?
it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?

Good Catcha! Now in this revision, the function would return false for the case of M->isGlobalModule() && M->Parent. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name isUsableModule for it from the wording of [module.global.frag]p1. I tried to call it isVisibleModule but I found there is already one... I feel isUsableModule is not perfectly good... Any suggestion?

2/
When we are actually parsing the PMF we have a similar case, no?

There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.

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

62–69

Agreed. This should be the work for build systems.

[ChuanqiXu]

Rename Sema::CurrentModuleUnitsCache to Sema::UsableModuleUnitsCache

clang/lib/Sema/SemaLookup.cpp

1581

I thought to add a comment for this. But I feel like it might be too wordy...

[iains]

it would be good to add tests for GMF and PMF cases?

clang/include/clang/Basic/Module.h

537–543

Especially now Module::getPrimaryModuleInterfaceName is only used for Sema.getLangOpts().CurrentModule. So it might be better to handle this in Sema or LangOptions.

Yeah, perhaps cache that split in Sema - adding another LangOption does not feel quite right (unless we find we need this information more widely - but I didn't see that yet).

542–543

if we find this is repeated too many times, we could cache it in the module as PrimaryModuleName.

clang/lib/Sema/SemaLookup.cpp

1572–1573

1/
we have the check of (M->isGlobalModule() && !M->Parent)

So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
if M->isGlobalModule() && M->Parent ?
it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?

Good Catcha! Now in this revision, the function would return false for the case of M->isGlobalModule() && M->Parent. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name isUsableModule for it from the wording of [module.global.frag]p1. I tried to call it isVisibleModule but I found there is already one... I feel isUsableModule is not perfectly good... Any suggestion?

actually that seems a reasonable name for now - I am beginning to feel that once we have things basically working - the whole module visibility stack could be refactored ...

2/
When we are actually parsing the PMF we have a similar case, no?

There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.

Yes, but....

a) When we are actually parsing the PMF, there will be no 'Parent' pointer set, so that we would have to find the parent some other way, right?
(perhaps it is not too complex - since we should already have diagnosed a bad parse situation if we see a :private module line in the wrong place - so that all we need to be able to do is to locate the current module - that has to be the top level one by the rules of PMF)

https://eel.is/c++draft/module#private.frag-1

b) The contents of the PMF are not 'part of the module' reachable to importers of the parent module - so it seems that we only have to consider then when the TU is the current top level module interface.

https://eel.is/c++draft/module#reach-3

1572–1573

1/
we have the check of (M->isGlobalModule() && !M->Parent)

So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
if M->isGlobalModule() && M->Parent ?
it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?

Good Catcha! Now in this revision, the function would return false for the case of M->isGlobalModule() && M->Parent. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name isUsableModule for it from the wording of [module.global.frag]p1. I tried to call it isVisibleModule but I found there is already one... I feel isUsableModule is not perfectly good... Any suggestion?

2/
When we are actually parsing the PMF we have a similar case, no?

There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.

[ChuanqiXu]

Address comments:

• Add tests for GMF
• Avoid to compare string if M is private module fragment.

[ChuanqiXu]

clang/include/clang/Basic/Module.h

537–543

Yeah, it makes sense to add a new method in Sema once it is used more widely.

542–543

I am worrying the benefit is not worth for the cost. It requires to change both Serialization and Deserialization to add a new member in Module. The scale would be larger and the benefit wouldn't be significant.

clang/lib/Sema/SemaLookup.cpp

1572–1573

I am beginning to feel that once we have things basically working - the whole module visibility stack could be refactored ...

Completely agreed.

When we are actually parsing the PMF, there will be no 'Parent' pointer set, so that we would have to find the parent some other way, right?

This is not right. When we are parsing the PMF, we already created its parent. So we could assign the Parent field. This is different from GMF.

The contents of the PMF are not 'part of the module' reachable to importers of the parent module - so it seems that we only have to consider then when the TU is the current top level module interface.

Yeah, I think the newest revision addresses the comment.

1572–1573

It looks like there is anything missing?

[iains]

thanks for multiple iterations!

I think maybe you are using a too old clang-format?
it seems that clang-format >= llvm-14 removes spaces around module partition colons ... so A : Part==>A:Part and : Impl => :Impl.
IMO this is the correct formatting (since it follows the examples in the standard).

Anyway, this is most likely the reason for the clang format fails on the CI.

This now LGTM - but please fix the formatting in the test cases.

clang/lib/Sema/SemaLookup.cpp

1572–1573

When we are actually parsing the PMF, there will be no 'Parent' pointer set, so that we would have to find the parent some other way, right?

This is not right. When we are parsing the PMF, we already created its parent. So we could assign the Parent field. This is different from GMF.

Indeed; I missed that the Parent is set on the creation for the PMF.

[ChuanqiXu]

In D123837#3461474, @iains wrote:

thanks for multiple iterations!

Thanks for the reviewing!

I think maybe you are using a too old clang-format?
it seems that clang-format >= llvm-14 removes spaces around module partition colons ... so A : Part==>A:Part and : Impl => :Impl.
IMO this is the correct formatting (since it follows the examples in the standard).

Anyway, this is most likely the reason for the clang format fails on the CI.

This now LGTM - but please fix the formatting in the test cases.

Yeah... It looks like I did't set clang-format to the newest version. Thanks for reminding. I would do it when committing the patch.

clang/include/clang/Basic/Module.h

public:
 
   /// Is this module a header unit.
   bool isHeaderUnit() const { return Kind == ModuleHeaderUnit; }
   // Is this a C++20 module interface or a partition.
   bool isInterfaceOrPartition() const {
     return Kind == ModuleInterfaceUnit || isModulePartition();
   }
 
+  static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+    return Name.split(':').first;
+  }
+
   /// Get the primary module interface name from a partition.
   StringRef getPrimaryModuleInterfaceName() const {
-    if (isModulePartition()) {
-      auto pos = Name.find(':');
-      return StringRef(Name.data(), pos);
-    }
-    return Name;
+    return getPrimaryModuleInterfaceName(getTopLevelModuleName());
   }
 
   /// Retrieve the full name of this module, including the path from
   /// its top-level module.
   /// \param AllowStringLiterals If \c true, components that might not be
   ///        lexically valid as identifiers will be emitted as string literals.
   std::string getFullModuleName(bool AllowStringLiterals = false) const;
 
   /// Whether the full name of this module is equal to joining
   /// \p nameParts with "."s.
   ///
   /// This is more efficient than getFullModuleName().
   bool fullModuleNameIs(ArrayRef<StringRef> nameParts) const;
 
   /// Retrieve the top-level module for this (sub)module, which may

clang/include/clang/Sema/Sema.h

private:
 
   /// Enter the scope of the global module.
   Module *PushGlobalModuleFragment(SourceLocation BeginLoc, bool IsImplicit);
   /// Leave the scope of the global module.
   void PopGlobalModuleFragment();
 
   VisibleModuleSet VisibleModules;
 
+  /// Cache for module units which is in the same module with the current
+  /// module.
+  llvm::DenseSet<const Module *> CurrentModuleUnitsCache;
+
+  bool isInCurrentModule(const Module *M);
+
 public:
   /// Get the module owning an entity.
   Module *getOwningModule(const Decl *Entity) {
     return Entity->getOwningModule();
   }
 
   bool isModuleDirectlyImported(const Module *M) {
     return DirectModuleImports.contains(M);

clang/lib/Sema/SemaLookup.cpp

Module *M = CodeSynthesisContexts[I].Entity ?
                 nullptr;
     if (M && !LookupModulesCache.insert(M).second)
       M = nullptr;
     CodeSynthesisContextLookupModules.push_back(M);
   }
   return LookupModulesCache;
 }
 
-/// Determine whether the module M is part of the current module from the
-/// perspective of a module-private visibility check.
-static bool isInCurrentModule(const Module *M, const LangOptions &LangOpts) {
+/// Determine whether the module M is in the same module with the current
+/// module.
+bool Sema::isInCurrentModule(const Module *M) {
+  assert(M && "We shouldn't check nullness for module here");
+  // Return quickly if we cached the result.
+  if (CurrentModuleUnitsCache.count(M))
+    return true;
+
   // If M is the global module fragment of a module that we've not yet finished
   // parsing, then it must be part of the current module.
-  // If it's a partition, then it must be visible to an importer (since only
-  // another partition or the named module can import it).
-  return M->getTopLevelModuleName() == LangOpts.CurrentModule ||
-         (M->Kind == Module::GlobalModuleFragment && !M->Parent) ||
-         M->Kind == Module::ModulePartitionInterface ||
-         M->Kind == Module::ModulePartitionImplementation;
+  if ((M->isGlobalModule() && !M->Parent) ||
+      (M->getPrimaryModuleInterfaceName() ==
+       Module::getPrimaryModuleInterfaceName(getLangOpts().CurrentModule))) {
+    CurrentModuleUnitsCache.insert(M);
+    return true;
+  }
+
+  return false;
 }
 
 bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
   for (const Module *Merged : Context.getModulesWithMergedDefinition(Def))
     if (isModuleVisible(Merged))
       return true;
   return false;
 }
 
 bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) {
   for (const Module *Merged : Context.getModulesWithMergedDefinition(Def))
-    if (isInCurrentModule(Merged, getLangOpts()))
+    if (isInCurrentModule(Merged))
       return true;
   return false;
 }
 
 template<typename ParmDecl>
 static bool
 hasVisibleDefaultArgument(Sema &S, const ParmDecl *D,
                           llvm::SmallVectorImpl<Module *> *Modules) {
bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) {
   return false;
 }
 
 bool Sema::isModuleVisible(const Module *M, bool ModulePrivate) {
   // The module might be ordinarily visible. For a module-private query, that
   // means it is part of the current module. For any other query, that means it
   // is in our visible module set.
   if (ModulePrivate) {
-    if (isInCurrentModule(M, getLangOpts()))
+    if (isInCurrentModule(M))
       return true;
     else if (M->Kind == Module::ModuleKind::ModulePartitionImplementation &&
              isModuleDirectlyImported(M))
       // Unless a partition implementation is directly imported it is not
       // counted as visible for lookup, although the contained decls might
       // still be reachable.  It's a partition, so it must be part of the
       // current module to be a valid import.
       return true;

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

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c++20 %t/impl.cppm -emit-module-interface -o %t/M-impl.pcm
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -fprebuilt-module-path=%t -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/Use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/UseInPartA.cppm -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/UseInAnotherModule.cppm -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- impl.cppm
+module M:impl;
+class A {};
+
+//--- M.cppm
+export module M;
+import :impl;
+export A f();
+
+//--- Use.cpp
+import M;
+void test() {
+  A a; // expected-error {{unknown type name 'A'}}
+}
+
+//--- 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 {{unknown type name 'A'}}
+}

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

 // 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 {{use of undeclared identifier}}
 
 //--- std10-1-ex2-tu7.cpp
+// expected-no-diagnostics
 module B:X3; // does not implicitly import B
-import :X2;  // X2 is an implementation so exports nothing.
-             // error: n not visible here.
-int &c = n;  // expected-error {{use of undeclared identifier }}
+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;