[2/3] c++/modules: Handle redefinitions of using-decls

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

This fixes an ICE exposed by supporting exported non-function
using-decls.  Sometimes when preparing to define a class, xref_tag will
find a using-decl belonging to a different namespace, which triggers the
checking_assert in modules handling.

Ideally I feel that 'lookup_and_check_tag' should be told whether we're
about to define the type and handle erroring on redefinitions itself to
avoid this issue (and provide better diagnostics by acknowledging the
using-declaration), but this is complicated with the current
fragmentation of definition checking.  So for this patch we just fixup
the assertion and ensure that pushdecl properly errors on the
conflicting declaration later.

gcc/cp/ChangeLog:

	* decl.cc (xref_tag): Move assertion into condition.
	* name-lookup.cc (check_module_override): Check for conflicting
	types and using-decls.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/using-19_a.C: New test.
	* g++.dg/modules/using-19_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/decl.cc                            |  6 +++--
 gcc/cp/name-lookup.cc                     | 32 ++++++++++++++++-------
 gcc/testsuite/g++.dg/modules/using-19_a.C | 18 +++++++++++++
 gcc/testsuite/g++.dg/modules/using-19_b.C | 10 +++++++
 4 files changed, 54 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/using-19_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/using-19_b.C

On 7/6/24 10:07 PM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> This fixes an ICE exposed by supporting exported non-function
> using-decls.  Sometimes when preparing to define a class, xref_tag will
> find a using-decl belonging to a different namespace, which triggers the
> checking_assert in modules handling.
> 
> Ideally I feel that 'lookup_and_check_tag' should be told whether we're
> about to define the type and handle erroring on redefinitions itself to
> avoid this issue (and provide better diagnostics by acknowledging the
> using-declaration), but this is complicated with the current
> fragmentation of definition checking.  So for this patch we just fixup
> the assertion and ensure that pushdecl properly errors on the
> conflicting declaration later.

Or perhaps teaching duplicate_decls to handle using-decls better, so you 
don't need to strip them in the caller?

The patch is OK.

> gcc/cp/ChangeLog:
> 
> 	* decl.cc (xref_tag): Move assertion into condition.
> 	* name-lookup.cc (check_module_override): Check for conflicting
> 	types and using-decls.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/using-19_a.C: New test.
> 	* g++.dg/modules/using-19_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/decl.cc                            |  6 +++--
>   gcc/cp/name-lookup.cc                     | 32 ++++++++++++++++-------
>   gcc/testsuite/g++.dg/modules/using-19_a.C | 18 +++++++++++++
>   gcc/testsuite/g++.dg/modules/using-19_b.C | 10 +++++++
>   4 files changed, 54 insertions(+), 12 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-19_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-19_b.C
> 
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 5a823e2f94c..586dda19604 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -16737,12 +16737,14 @@ xref_tag (enum tag_types tag_code, tree name,
>   	  if (CLASS_TYPE_P (t) && CLASSTYPE_IS_TEMPLATE (t))
>   	    maybe_tmpl = CLASSTYPE_TI_TEMPLATE (t);
>   
> +	  /* FIXME: we should do a more precise check for redefinitions
> +	     of a conflicting using-declaration here, as these diagnostics
> +	     are not ideal.  */
>   	  if (DECL_LANG_SPECIFIC (decl)
>   	      && DECL_MODULE_IMPORT_P (decl)
> -	      && TREE_CODE (CP_DECL_CONTEXT (decl)) == NAMESPACE_DECL)
> +	      && CP_DECL_CONTEXT (decl) == current_namespace)
>   	    {
>   	      /* Push it into this TU's symbol slot.  */
> -	      gcc_checking_assert (current_namespace == CP_DECL_CONTEXT (decl));
>   	      if (maybe_tmpl != decl)
>   		/* We're in the template parm binding level.
>   		   Pushtag has logic to slide under that, but we're
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index ddb4c0fe1c8..96d7f938162 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -3782,18 +3782,30 @@ check_module_override (tree decl, tree mvec, bool hiding,
>   	  /* Errors could cause there to be nothing.  */
>   	  continue;
>   
> +	tree type = NULL_TREE;
>   	if (STAT_HACK_P (bind))
> -	  /* We do not have to check STAT_TYPE here, the xref_tag
> -	     machinery deals with that problem. */
> -	  bind = STAT_VISIBLE (bind);
> +	  {
> +	    /* If there was a matching STAT_TYPE here then xref_tag
> +	       should have found it, but we need to check anyway because
> +	       a conflicting using-declaration may exist.  */
> +	    if (STAT_TYPE_VISIBLE_P (bind))
> +	      type = STAT_TYPE (bind);
> +	    bind = STAT_VISIBLE (bind);
> +	  }
>   
> -	for (ovl_iterator iter (bind); iter; ++iter)
> -	  if (!iter.using_p ())
> -	    {
> -	      match = duplicate_decls (decl, *iter, hiding);
> -	      if (match)
> -		goto matched;
> -	    }
> +	if (type)
> +	  {
> +	    match = duplicate_decls (decl, strip_using_decl (type), hiding);
> +	    if (match)
> +	      goto matched;
> +	  }
> +
> +	for (ovl_iterator iter (strip_using_decl (bind)); iter; ++iter)
> +	  {
> +	    match = duplicate_decls (decl, *iter, hiding);
> +	    if (match)
> +	      goto matched;
> +	  }
>         }
>   
>     if (TREE_PUBLIC (scope) && TREE_PUBLIC (STRIP_TEMPLATE (decl))
> diff --git a/gcc/testsuite/g++.dg/modules/using-19_a.C b/gcc/testsuite/g++.dg/modules/using-19_a.C
> new file mode 100644
> index 00000000000..693a70ce7d4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-19_a.C
> @@ -0,0 +1,18 @@
> +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> +// { dg-module-cmi M }
> +
> +module;
> +
> +namespace hidden {
> +  struct S {};
> +  enum E { e };
> +  void f();
> +}
> +
> +export module M;
> +export namespace exposed {
> +  using hidden::S;
> +  using hidden::E;
> +  using hidden::e;
> +  using hidden::f;
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/using-19_b.C b/gcc/testsuite/g++.dg/modules/using-19_b.C
> new file mode 100644
> index 00000000000..dbe8d9f3c01
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-19_b.C
> @@ -0,0 +1,10 @@
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import M;
> +
> +namespace exposed {
> +  struct S {};  // { dg-error "redefinition" }
> +  enum E { x };  // { dg-error "multiple definition" }
> +  int e();  // { dg-error "redeclared" }
> +  int f;  // { dg-error "redeclared" }
> +}

On Mon, Jul 08, 2024 at 02:21:41PM -0400, Jason Merrill wrote:
> On 7/6/24 10:07 PM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > -- >8 --
> > 
> > This fixes an ICE exposed by supporting exported non-function
> > using-decls.  Sometimes when preparing to define a class, xref_tag will
> > find a using-decl belonging to a different namespace, which triggers the
> > checking_assert in modules handling.
> > 
> > Ideally I feel that 'lookup_and_check_tag' should be told whether we're
> > about to define the type and handle erroring on redefinitions itself to
> > avoid this issue (and provide better diagnostics by acknowledging the
> > using-declaration), but this is complicated with the current
> > fragmentation of definition checking.  So for this patch we just fixup
> > the assertion and ensure that pushdecl properly errors on the
> > conflicting declaration later.
> 
> Or perhaps teaching duplicate_decls to handle using-decls better, so you
> don't need to strip them in the caller?
> 

Yup, I have another patch I'd been working on which does this, but this
is a much more targeted approach without all the churn in diagnostics
I'm working through with that change.

But either way I think the change for 'lookup_and_check_tag' is
necessary, because under normal circumstances we don't actually go
through duplicate_decls here until much later, because we always just
work with the existing type name found as an elaborated type specifier.
I imagine we might need to rework this somehow to handle textual
redefinitions too, but I haven't yet really thought through what the
best way to approach that would be.

> The patch is OK.

Thanks.

Nathaniel

> 
> > gcc/cp/ChangeLog:
> > 
> > 	* decl.cc (xref_tag): Move assertion into condition.
> > 	* name-lookup.cc (check_module_override): Check for conflicting
> > 	types and using-decls.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/using-19_a.C: New test.
> > 	* g++.dg/modules/using-19_b.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >   gcc/cp/decl.cc                            |  6 +++--
> >   gcc/cp/name-lookup.cc                     | 32 ++++++++++++++++-------
> >   gcc/testsuite/g++.dg/modules/using-19_a.C | 18 +++++++++++++
> >   gcc/testsuite/g++.dg/modules/using-19_b.C | 10 +++++++
> >   4 files changed, 54 insertions(+), 12 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/modules/using-19_a.C
> >   create mode 100644 gcc/testsuite/g++.dg/modules/using-19_b.C
> > 
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index 5a823e2f94c..586dda19604 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -16737,12 +16737,14 @@ xref_tag (enum tag_types tag_code, tree name,
> >   	  if (CLASS_TYPE_P (t) && CLASSTYPE_IS_TEMPLATE (t))
> >   	    maybe_tmpl = CLASSTYPE_TI_TEMPLATE (t);
> > +	  /* FIXME: we should do a more precise check for redefinitions
> > +	     of a conflicting using-declaration here, as these diagnostics
> > +	     are not ideal.  */
> >   	  if (DECL_LANG_SPECIFIC (decl)
> >   	      && DECL_MODULE_IMPORT_P (decl)
> > -	      && TREE_CODE (CP_DECL_CONTEXT (decl)) == NAMESPACE_DECL)
> > +	      && CP_DECL_CONTEXT (decl) == current_namespace)
> >   	    {
> >   	      /* Push it into this TU's symbol slot.  */
> > -	      gcc_checking_assert (current_namespace == CP_DECL_CONTEXT (decl));
> >   	      if (maybe_tmpl != decl)
> >   		/* We're in the template parm binding level.
> >   		   Pushtag has logic to slide under that, but we're
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index ddb4c0fe1c8..96d7f938162 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -3782,18 +3782,30 @@ check_module_override (tree decl, tree mvec, bool hiding,
> >   	  /* Errors could cause there to be nothing.  */
> >   	  continue;
> > +	tree type = NULL_TREE;
> >   	if (STAT_HACK_P (bind))
> > -	  /* We do not have to check STAT_TYPE here, the xref_tag
> > -	     machinery deals with that problem. */
> > -	  bind = STAT_VISIBLE (bind);
> > +	  {
> > +	    /* If there was a matching STAT_TYPE here then xref_tag
> > +	       should have found it, but we need to check anyway because
> > +	       a conflicting using-declaration may exist.  */
> > +	    if (STAT_TYPE_VISIBLE_P (bind))
> > +	      type = STAT_TYPE (bind);
> > +	    bind = STAT_VISIBLE (bind);
> > +	  }
> > -	for (ovl_iterator iter (bind); iter; ++iter)
> > -	  if (!iter.using_p ())
> > -	    {
> > -	      match = duplicate_decls (decl, *iter, hiding);
> > -	      if (match)
> > -		goto matched;
> > -	    }
> > +	if (type)
> > +	  {
> > +	    match = duplicate_decls (decl, strip_using_decl (type), hiding);
> > +	    if (match)
> > +	      goto matched;
> > +	  }
> > +
> > +	for (ovl_iterator iter (strip_using_decl (bind)); iter; ++iter)
> > +	  {
> > +	    match = duplicate_decls (decl, *iter, hiding);
> > +	    if (match)
> > +	      goto matched;
> > +	  }
> >         }
> >     if (TREE_PUBLIC (scope) && TREE_PUBLIC (STRIP_TEMPLATE (decl))
> > diff --git a/gcc/testsuite/g++.dg/modules/using-19_a.C b/gcc/testsuite/g++.dg/modules/using-19_a.C
> > new file mode 100644
> > index 00000000000..693a70ce7d4
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-19_a.C
> > @@ -0,0 +1,18 @@
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> > +// { dg-module-cmi M }
> > +
> > +module;
> > +
> > +namespace hidden {
> > +  struct S {};
> > +  enum E { e };
> > +  void f();
> > +}
> > +
> > +export module M;
> > +export namespace exposed {
> > +  using hidden::S;
> > +  using hidden::E;
> > +  using hidden::e;
> > +  using hidden::f;
> > +}
> > diff --git a/gcc/testsuite/g++.dg/modules/using-19_b.C b/gcc/testsuite/g++.dg/modules/using-19_b.C
> > new file mode 100644
> > index 00000000000..dbe8d9f3c01
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-19_b.C
> > @@ -0,0 +1,10 @@
> > +// { dg-additional-options "-fmodules-ts" }
> > +
> > +import M;
> > +
> > +namespace exposed {
> > +  struct S {};  // { dg-error "redefinition" }
> > +  enum E { x };  // { dg-error "multiple definition" }
> > +  int e();  // { dg-error "redeclared" }
> > +  int f;  // { dg-error "redeclared" }
> > +}
>