Omit sumbodule semantics for TS modules

[boris]

Summary

If a TS module name has more than one component (e.g., foo.bar) then we erroneously activate the submodule semantics when encountering a module declaration in the module implementation unit (e.g., module foo.bar;).

[boris]

Ping.

[boris]

Ping.

[rsmith]

This will need a test case.

lib/Frontend/CompilerInstance.cpp

1598–1612 ↗

(On Diff #107486)

The strategy we've been using so far is this: components after the first in a module path name submodules (visibility groups within a module). Modules TS modules use only the first name component, which may contain periods if the module name contains periods.

So the burden should be on the code that produces a ModuleIdPath to do this flattening, not on consumers of the ModuleIdPath.

(FWIW, I think we may want to consider this later, so that import foo.bar; could import either the Modules TS module foo.bar, or the submodule bar of the module map module foo, but that will take some fundamental restructuring of how we model Module objects and their hierarchy.)

[boris]

Ping.

[boris]

Richard, sorry for the last ping, somehow I missed your review.

I've uploaded a new revision with a test case (note that the issue is with 'module a.b' not 'import a.b' but I have tested both for good measure).

My understanding of your comment is that the rest is ok for now (since there will probably be a redesign in this area). If, however, I misunderstood and you sill want to move the id flattening to the caller, let me know.

[rsmith]

In D35678#840092, @boris wrote:

My understanding of your comment is that the rest is ok for now (since there will probably be a redesign in this area). If, however, I misunderstood and you sill want to move the id flattening to the caller, let me know.

I'd still like the id flattening moved to the caller. I don't know when or if the redesign will ever happen, so we should aim for the state of the codebase to be consistent in the interim (which might be forever). I'm fine with that being done as a separate change after this one, though, if you'd prefer.

Do you need someone to commit this for you?

[boris]

In D35678#842891, @rsmith wrote:

I'd still like the id flattening moved to the caller. [...] I'm fine with that being done as a separate change after this one, though, if you'd prefer.

Yes, that would probably be easier.

Do you need someone to commit this for you?

Yes, please, I don't have commit rights. Though if I were given them, I could do that myself (and it would also help with the other patch, I guess).

cfe/trunk/lib/Frontend/CompilerInstance.cpp

 }
 
 ModuleLoadResult
 CompilerInstance::loadModule(SourceLocation ImportLoc,
                              ModuleIdPath Path,
                              Module::NameVisibilityKind Visibility,
                              bool IsInclusionDirective) {
   // Determine what file we're searching from.
-  StringRef ModuleName = Path[0].first->getName();
+  // FIXME: Should we be deciding whether this is a submodule (here and
+  // below) based on -fmodules-ts or should we pass a flag and make the
+  // caller decide?
+  std::string ModuleName;
+  if (getLangOpts().ModulesTS) {
+    // FIXME: Same code as Sema::ActOnModuleDecl() so there is probably a
+    // better place/way to do this.
+    for (auto &Piece : Path) {
+      if (!ModuleName.empty())
+        ModuleName += ".";
+      ModuleName += Piece.first->getName();
+    }
+  }
+  else
+    ModuleName = Path[0].first->getName();
+
   SourceLocation ModuleNameLoc = Path[0].second;
 
   // If we've already handled this import, just return the cached result.
   // This one-element cache is important to eliminate redundant diagnostics
   // when both the preprocessor and parser see the same import declaration.
   if (ImportLoc.isValid() && LastModuleImportLoc == ImportLoc) {
     // Make the named module visible.
     if (LastModuleImportResult && ModuleName != getLangOpts().CurrentModule)
CompilerInstance::loadModule(SourceLocation ImportLoc,
   }
   
   // If we never found the module, fail.
   if (!Module)
     return ModuleLoadResult();
 
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
-  if (Path.size() > 1) {
+  if (!getLangOpts().ModulesTS && Path.size() > 1) {
     for (unsigned I = 1, N = Path.size(); I != N; ++I) {
       StringRef Name = Path[I].first->getName();
       clang::Module *Sub = Module->findSubmodule(Name);
       
       if (!Sub) {
         // Attempt to perform typo correction to find a module name that works.
         SmallVector<StringRef, 2> Best;
         unsigned BestEditDistance = (std::numeric_limits<unsigned>::max)();

cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: echo 'export module x; export int a, b;' > %t/x.cppm
 // RUN: echo 'export module x.y; export int c;' > %t/x.y.cppm
+// RUN: echo 'export module a.b; export int d;' > %t/a.b.cppm
 //
 // RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
 // RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/x.y.cppm -o %t/x.y.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/a.b.cppm -o %t/a.b.pcm
 //
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -verify %s \
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -fmodule-file=%t/a.b.pcm -verify %s \
 // RUN:            -DMODULE_NAME=z
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -verify %s \
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -fmodule-file=%t/a.b.pcm -verify %s \
+// RUN:            -DMODULE_NAME=a.b
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -fmodule-file=%t/a.b.pcm -verify %s \
 // RUN:            -DMODULE_X -DMODULE_NAME=x
 
 module MODULE_NAME;
 
 int use_1 = a;
 #if !MODULE_X
 // expected-error@-2 {{declaration of 'a' must be imported from module 'x' before it is required}}
 // [e-mail]:1 {{here}}
 #endif
 
 import x;
 
 int use_2 = b; // ok
 
 // There is no relation between module x and module x.y.
 int use_3 = c; // expected-error {{declaration of 'c' must be imported from module 'x.y'}}
                // [e-mail]:1 {{here}}
 
 import x [[]];
 import x [[foo]]; // expected-warning {{unknown attribute 'foo' ignored}}
 import x [[noreturn]]; // expected-error {{'noreturn' attribute cannot be applied to a module import}}
 import x [[blarg::noreturn]]; // expected-warning {{unknown attribute 'noreturn' ignored}}
 
 import x.y;
+import a.b; // Does not imply existence of module a.
 import x.; // expected-error {{expected a module name after 'import'}}
 import .x; // expected-error {{expected a module name after 'import'}}
 
 int use_4 = c; // ok
 
 import blarg; // expected-error {{module 'blarg' not found}}