c++/modules: Cleanup import handling [PR99682]

This patch fixes some issues with import handling.

The main functional change is to allow importing a module's interface
file into its implementation file indirectly.  [module.import] p9
forbids an explicit 'import M;' declaration, but there's no provision
against having 'import X;' where module X itself imports M, so this
patch splits up the detection of circular imports to handle this better.
I also updated the errors to be closer to the standard wording.

A second issue I found while testing this is that we don't properly
handle name visibility when a partition implementation unit imports its
primary module interface (g++.dg/modules/part-10).  This is resolved by
setting 'module_p' on the primary interface when it gets imported.

Solving this I incidentally removed the assertion that PR121808 was
failing on, which was never valid: we can enter import_module for a
module previously seen as a module-declaration if parsing bails before
declare_module is called.  I experimented with guaranteeing that
declare_module always gets called for a module_state generated during
preprocessing if at all possible, but the resulting errors didn't seem a
lot better so I've left it as-is for now.

I did make a small adjustment so that a direct import of a module
doesn't overwrite the location of a module-declaration from earlier in
the file; this is important because preprocessing (and thus the
assigning of these locations) seems to happen for the whole file before
parsing begins, which can otherwise cause confusing locations referring
to later in the file than parsing would otherwise indicate.

	PR c++/99682
	PR c++/121808

gcc/cp/ChangeLog:

	* module.cc (class module_state): Add comment to 'parent'.
	(module_state::check_not_purview): Rename to...
	(module_state::check_circular_import): ...this.  Only handle
	interface dependencies, adjust diagnostic message.
	(module_state::read_imports): Use new function.  Pass location
	of import as from_loc instead of the module location.
	(module_state::do_import): Set module_p when importing the
	primary interface for the current module.
	(import_module): Split out check for imports in own unit.
	Remove incorrect assertion.
	(preprocess_module): Don't overwrite module-decl location with
	later import.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/circ-1_c.C: Adjust diagnostic.
	* g++.dg/modules/mod-decl-1.C: Likewise.
	* g++.dg/modules/mod-decl-2_b.C: Likewise.
	* g++.dg/modules/pr99174.H: Likewise.
	* g++.dg/modules/import-3_a.C: New test.
	* g++.dg/modules/import-3_b.C: New test.
	* g++.dg/modules/import-3_c.C: New test.
	* g++.dg/modules/mod-decl-9.C: New test.
	* g++.dg/modules/part-10_a.C: New test.
	* g++.dg/modules/part-10_b.C: New test.
	* g++.dg/modules/part-10_c.C: New test.
	* g++.dg/modules/part-10_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Reviewed-by: Jason Merrill <jason@redhat.com>

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 50fc1af035410198cbd3996b9073646fc57923db..d5c3a83c728b7015411db5b2984c4883b3805ef3 100644 (file)
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -3667,8 +3667,12 @@ class GTY((chain_next ("%h.parent"), for_user)) module_state {
   bitmap imports;      /* Transitive modules we're importing.  */
   bitmap exports;      /* Subset of that, that we're exporting.  */
 
+  /* For a named module interface A.B, parent is A and name is B.
+     For a partition M:P, parent is M and name is P.
+     For an implementation unit I, parent is I's interface and name is NULL.
+     Otherwise parent is NULL and name will be the flatname.  */
   module_state *parent;
-  tree name;           /* Name of the module.  */
+  tree name;
 
   slurping *slurp;     /* Data for loading.  */
 
@@ -3782,7 +3786,7 @@ class GTY((chain_next ("%h.parent"), for_user)) module_state {
   }
 
  public:
-  bool check_not_purview (location_t loc);
+  bool check_circular_import (location_t loc);
 
  public:
   void mangle (bool include_partition);
@@ -15996,20 +16000,19 @@ recursive_lazy (unsigned snum = ~0u)
   return false;
 }
 
-/* If THIS is the current purview, issue an import error and return false.  */
+/* If THIS has an interface dependency on itself, report an error and
+   return false.  */
 
 bool
-module_state::check_not_purview (location_t from)
+module_state::check_circular_import (location_t from)
 {
-  module_state *imp = this_module ();
-  if (imp && !imp->name)
-    imp = imp->parent;
-  if (imp == this)
+  if (this == this_module ())
     {
       /* Cannot import the current module.  */
       auto_diagnostic_group d;
-      error_at (from, "cannot import module in its own purview");
-      inform (loc, "module %qs declared here", get_flatname ());
+      error_at (from, "module %qs depends on itself", get_flatname ());
+      if (!header_module_p ())
+       inform (loc, "module %qs declared here", get_flatname ());
       return false;
     }
   return true;
@@ -16320,7 +16323,7 @@ module_state::read_imports (bytes_in &sec, cpp_reader *reader, line_maps *lmaps)
          if (sec.get_overrun ())
            break;
 
-         if (!imp->check_not_purview (loc))
+         if (!imp->check_circular_import (floc))
            continue;
 
          if (imp->loadedness == ML_NONE)
@@ -21535,6 +21538,12 @@ module_state::do_import (cpp_reader *reader, bool outermost)
 {
   gcc_assert (global_namespace == current_scope () && loadedness == ML_NONE);
 
+  /* If this TU is a partition of the module we're importing,
+     that module is the primary module interface.  */
+  if (this_module ()->is_partition ()
+      && this == get_primary (this_module ()))
+    module_p = true;
+
   loc = linemap_module_loc (line_table, loc, get_flatname ());
 
   if (lazy_open >= lazy_limit)
@@ -21807,7 +21816,17 @@ void
 import_module (module_state *import, location_t from_loc, bool exporting_p,
               tree, cpp_reader *reader)
 {
-  if (!import->check_not_purview (from_loc))
+  /* A non-partition implementation unit has no name.  */
+  if (!this_module ()->name && this_module ()->parent == import)
+    {
+      auto_diagnostic_group d;
+      error_at (from_loc, "import of %qs within its own implementation unit",
+               import->get_flatname());
+      inform (import->loc, "module declared here");
+      return;
+    }
+
+  if (!import->check_circular_import (from_loc))
     return;
 
   if (!import->is_header () && current_lang_depth ())
@@ -21829,7 +21848,7 @@ import_module (module_state *import, location_t from_loc, bool exporting_p,
       from_loc = ordinary_loc_of (line_table, from_loc);
       linemap_module_reparent (line_table, import->loc, from_loc);
     }
-  gcc_checking_assert (!import->module_p);
+
   gcc_checking_assert (import->is_direct () && import->has_location ());
 
   direct_import (import, reader);
@@ -22322,7 +22341,10 @@ preprocess_module (module_state *module, location_t from_loc,
        linemap_module_reparent (line_table, module->loc, from_loc);
       else
        {
-         module->loc = from_loc;
+         /* Don't overwrite the location if we're importing ourselves
+            after already having seen a module-declaration.  */
+         if (!(is_import && module->is_module ()))
+           module->loc = from_loc;
          if (!module->flatname)
            module->set_flatname ();
        }

diff --git a/gcc/testsuite/g++.dg/modules/circ-1_c.C b/gcc/testsuite/g++.dg/modules/circ-1_c.C
index cea17d7a2e0a3563c445b2e1f543aa47690aed17..8bb2ce16052ffd782207476b7a296a65285ffa02 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/circ-1_c.C
+++ b/gcc/testsuite/g++.dg/modules/circ-1_c.C
@@ -4,6 +4,6 @@ export module Bob; // { dg-message "declared here" }
 
 import Kevin;
 // { dg-error "failed to read" "" { target *-*-* } 0 }
-// { dg-error "cannot import module" "" { target *-*-* } 0 }
+// { dg-error "depends on itself" "" { target *-*-* } 0 }
 // { dg-prune-output "fatal error:" }
 // { dg-prune-output "compilation terminated" }

diff --git a/gcc/testsuite/g++.dg/modules/import-3_a.C b/gcc/testsuite/g++.dg/modules/import-3_a.C
new file mode 100644 (file)
index 0000000..8143da0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/import-3_a.C
@@ -0,0 +1,6 @@
+// PR c++/99682
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi foo }
+
+export module foo;
+export void foo();

diff --git a/gcc/testsuite/g++.dg/modules/import-3_b.C b/gcc/testsuite/g++.dg/modules/import-3_b.C
new file mode 100644 (file)
index 0000000..3c6a5d7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/import-3_b.C
@@ -0,0 +1,8 @@
+// PR c++/99682
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi bar }
+
+export module bar;
+import foo;
+
+export void bar();

diff --git a/gcc/testsuite/g++.dg/modules/import-3_c.C b/gcc/testsuite/g++.dg/modules/import-3_c.C
new file mode 100644 (file)
index 0000000..2508d61
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/import-3_c.C
@@ -0,0 +1,11 @@
+// PR c++/99682
+// { dg-additional-options "-fmodules" }
+
+module foo;
+import bar;  // not an interface dependency
+import bar;  // double import is not an error either
+
+void test() {
+  foo();
+  bar();
+}

diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-1.C b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
index ac7bb84699a1abdf9e97948f99b18c0ae8db2273..45c540914a1e90cef11be520b47d6957c155a54d 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/mod-decl-1.C
+++ b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
@@ -1,10 +1,10 @@
 // { dg-additional-options "-fmodules-ts" }
 module;
 
-export module frist;
+export module frist;  // { dg-message "declared here" }
 // { dg-module-cmi "!frist" }
 
-import frist; // { dg-error {cannot import module.* in its own purview} }
+import frist; // { dg-error {module 'frist' depends on itself} }
 
 module foo.second; // { dg-error "only permitted as" }
 

diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-2_b.C b/gcc/testsuite/g++.dg/modules/mod-decl-2_b.C
index a3ea9b5aa6ac53fceba2c61dff427da8ab479334..f01f0b06566c032285e46ffe46e22aae84f9e333 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/mod-decl-2_b.C
+++ b/gcc/testsuite/g++.dg/modules/mod-decl-2_b.C
@@ -1,7 +1,7 @@
 // { dg-additional-options "-fmodules-ts" }
 module bob;
 
-import bob; // { dg-error "cannot import module.* in its own purview" }
+import bob; // { dg-error "import of 'bob' within its own" }
 
 // module linkage
 void Baz ()

diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-9.C b/gcc/testsuite/g++.dg/modules/mod-decl-9.C
new file mode 100644 (file)
index 0000000..16e24a6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/mod-decl-9.C
@@ -0,0 +1,13 @@
+// PR c++/121808
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi !foo }
+
+void test();
+
+export module foo;  // { dg-error "module-declaration" }
+
+import foo;
+// { dg-error "failed to read compiled module" "" { target *-*-* } 0 }
+
+// { dg-prune-output "fatal error:" }
+// { dg-prune-output "compilation terminated" }

diff --git a/gcc/testsuite/g++.dg/modules/part-10_a.C b/gcc/testsuite/g++.dg/modules/part-10_a.C
new file mode 100644 (file)
index 0000000..056da71
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/part-10_a.C
@@ -0,0 +1,5 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi foo }
+
+export module foo;
+void foo();

diff --git a/gcc/testsuite/g++.dg/modules/part-10_b.C b/gcc/testsuite/g++.dg/modules/part-10_b.C
new file mode 100644 (file)
index 0000000..62865d1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/part-10_b.C
@@ -0,0 +1,9 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi foo:part }
+
+module foo:part;
+import foo;
+
+void part() {
+  foo();
+}

diff --git a/gcc/testsuite/g++.dg/modules/part-10_c.C b/gcc/testsuite/g++.dg/modules/part-10_c.C
new file mode 100644 (file)
index 0000000..7f0a1ea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/part-10_c.C
@@ -0,0 +1,10 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi foo:trans }
+
+module foo:trans;
+import :part;
+
+void trans() {
+  foo();
+  part();
+}

diff --git a/gcc/testsuite/g++.dg/modules/part-10_d.C b/gcc/testsuite/g++.dg/modules/part-10_d.C
new file mode 100644 (file)
index 0000000..2c0c3ff
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/part-10_d.C
@@ -0,0 +1,10 @@
+// { dg-additional-options "-fmodules" }
+
+module foo;
+import :trans;
+
+void impl() {
+  foo();
+  part();
+  trans();
+}

diff --git a/gcc/testsuite/g++.dg/modules/pr99174.H b/gcc/testsuite/g++.dg/modules/pr99174.H
index 60d01c59c772f1b46cf29fb477a097af8f0061ea..1ad20b71b7ab8c1f88ef8dd4d83a00c9a6985000 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/pr99174.H
+++ b/gcc/testsuite/g++.dg/modules/pr99174.H
@@ -1,3 +1,3 @@
 // { dg-additional-options -fmodule-header }
 // { dg-module-cmi !{} }
-import "pr99174.H"; // { dg-error "cannot import" }
+import "pr99174.H"; // { dg-error "depends on itself" }