On Tue, Sep 09, 2025 at 07:38:46PM +0200, Jason Merrill wrote:
> On 9/9/25 4:49 PM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > (If you prefer I could also split up each fix into separate commits.)
> >
> > -- >8 --
> >
> > 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): Rename check_not_purview.
> > (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 <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/module.cc | 44 +++++++++++++++------
> > gcc/testsuite/g++.dg/modules/circ-1_c.C | 2 +-
> > gcc/testsuite/g++.dg/modules/import-3_a.C | 6 +++
> > gcc/testsuite/g++.dg/modules/import-3_b.C | 8 ++++
> > gcc/testsuite/g++.dg/modules/import-3_c.C | 11 ++++++
> > gcc/testsuite/g++.dg/modules/mod-decl-1.C | 4 +-
> > gcc/testsuite/g++.dg/modules/mod-decl-2_b.C | 2 +-
> > gcc/testsuite/g++.dg/modules/mod-decl-9.C | 13 ++++++
> > gcc/testsuite/g++.dg/modules/part-10_a.C | 5 +++
> > gcc/testsuite/g++.dg/modules/part-10_b.C | 9 +++++
> > gcc/testsuite/g++.dg/modules/part-10_c.C | 10 +++++
> > gcc/testsuite/g++.dg/modules/part-10_d.C | 10 +++++
> > gcc/testsuite/g++.dg/modules/pr99174.H | 2 +-
> > 13 files changed, 108 insertions(+), 18 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/import-3_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/import-3_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/import-3_c.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-9.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/part-10_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/part-10_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/part-10_c.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/part-10_d.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 50fc1af0354..ec9d84f11cc 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -3782,7 +3782,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 +15996,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 +16319,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 +21534,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 +21812,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)
>
> The patch is OK, but could you add a comment to module_state::parent? It's
> not immediately obvious what it means.
>
> Jason
>
Thanks, this is the comment I've added:
--- 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. */
