[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood marked 2 inline comments as done.
hamzasood added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:16185-16194
+  if (getLangOpts().ModulesTS) {
+Module *CurrentModule = getCurrentModule();
+assert(CurrentModule && "Expected to be in a module scope");
+
+// If the current module has been loaded from disk, then this is an
+// implementation unit and hence we shouldn't modify the module.
+// FIXME: Is that a hacky assumption? We can't just check

rsmith wrote:
> This is not appropriate; generally whether we serialize to an AST file should 
> be treated as orthogonal to whether we're in / importing a module.
> 
> The right check here is probably `getLangOpts().getCompilingModule() == 
> CMK_ModuleInterface`.
Thanks! I completely missed that lang opt.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 150649.
hamzasood added a comment.

Addressed Richard's comments.


https://reviews.llvm.org/D40443

Files:
  include/clang/Basic/Module.h
  lib/Basic/Module.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16570,6 +16570,17 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  //   Every name of an entity with linkage other than internal linkage made
+  //   visible in the 

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:16135
+for (Module *ImportedModule : Mod->Imports)
+  VisibleModules.setVisible(ImportedModule, ModuleLoc);
+  }

For completeness, you should also call 
`getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, Loc);` here. (It 
turns out to not actually matter right now because `ModulesTS` implies 
`LocalSubmoduleVisibility`, under which `makeModuleVisible` happens to be a 
no-op.)



Comment at: lib/Sema/SemaDecl.cpp:16185-16194
+  if (getLangOpts().ModulesTS) {
+Module *CurrentModule = getCurrentModule();
+assert(CurrentModule && "Expected to be in a module scope");
+
+// If the current module has been loaded from disk, then this is an
+// implementation unit and hence we shouldn't modify the module.
+// FIXME: Is that a hacky assumption? We can't just check

This is not appropriate; generally whether we serialize to an AST file should 
be treated as orthogonal to whether we're in / importing a module.

The right check here is probably `getLangOpts().getCompilingModule() == 
CMK_ModuleInterface`.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: lib/Basic/Module.cpp:349
 
+  // Everything visible to the interface unit's global module fragment is
+  // visible to the interface unit.

boris wrote:
> This comment (not sure about the code)  sounds wrong to me: names from the 
> interface unit before the module purview (I assume this is what "global 
> module fragment" refers to) should not be visible in the implementation units.
Yes, the documentation comment for this function (in the header) notes that 
things visible to the interface unit aren’t necessarily visible to the 
implementation units.

There’re a few reasons why I chose for the function to count modules that’re 
only visible from the interface unit:


  # The `Module::Kind` for a C++ module is `ModuleInterfaceUnit`. Since the 
module identifies itself as an interface unit, I figured it’d make sense for 
queries such as this to return what’s relevant for the interface unit.
  # I think it’s just more useful that way. I can’t think of any situation 
where a module would need to know what’s visible to implementation units of 
other modules (and I couldn’t find any such examples in Clang). However 
there’re plenty of reasons why one would want to know about visibility to an 
interface unit (synthesising default members, instantiating templates etc.)




https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

I don't think it will be wise for me to accept this revision since I can't 
claim to have good understanding of Clang's internal modules model. I think it 
will be wise to have Richard take a look.




Comment at: lib/Basic/Module.cpp:349
 
+  // Everything visible to the interface unit's global module fragment is
+  // visible to the interface unit.

This comment (not sure about the code)  sounds wrong to me: names from the 
interface unit before the module purview (I assume this is what "global module 
fragment" refers to) should not be visible in the implementation units.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 128284.
hamzasood added a comment.

I've been investigating an issue regarding visibility during code synthesis, 
and I noticed that this patch half fixes it. I've added the rest of the fix 
since it's related to what's going on here, and I guess this is now a 
"correctly handle imports from an interface unit" patch.

I believe this fixes this bug  
posted on the LLVM bug tracker.

@boris if this is still okay with you after seeing the changes I've made, would 
you mind accepting the revision so I can commit this without Phabricator 
shouting at me?


https://reviews.llvm.org/D40443

Files:
  include/clang/Basic/Module.h
  lib/Basic/Module.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s 

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Ping


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 124231.
hamzasood added a comment.

Good idea, I've added that to the test.
I'll give Richard some time to look over this before committing.


https://reviews.llvm.org/D40443

Files:
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16308,6 +16308,15 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

LGTM. Maybe it makes sense to also test that an unrelated translation unit that 
imports module 'test' sees neither 'a' nor 'b'.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

This provides an implementation for the changes outlined in section 4.1 of 
P0731r0, which clarifies the intended behaviour regarding implementation units 
being able to see imports made within their corresponding interface unit.


https://reviews.llvm.org/D40443

Files:
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DINTERFACE
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify
+
+#ifdef INTERFACE
+
+import a;
+export module test;
+import b;
+
+#else // IMPLEMENTATION
+
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16308,6 +16308,15 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  //   Every name of an entity with linkage other than internal linkage made
+  //   visible in the purview of the module interface unit of a module M is
+  //   visible in the purview of all module implementation units of M.
+  if (MDK == ModuleDeclKind::Implementation) {
+for (Module