Re: [PATCH v2 1/2] c++: Standardise errors for module_may_redeclare

[Patrick Palka <ppalka@redhat.com>]

On Mon, 15 Apr 2024, Nathaniel Shead wrote:

> I took another look at this patch and have split it into two, one (this
> one) to standardise the error messages used and prepare
> 'module_may_redeclare' for use with temploid friends, and another
> followup patch to actually handle them correctly.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

LGTM

> 
> -- >8 --
> 
> Currently different places calling 'module_may_redeclare' all emit very
> similar but slightly different error messages, and handle different
> kinds of declarations differently.  This patch makes the function
> perform its own error messages so that they're all in one place, and
> prepares it for use with temploid friends (PR c++/114275).
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (module_may_redeclare): Add default parameter.
> 	* decl.cc (duplicate_decls): Don't emit errors for failed
> 	module_may_redeclare.
> 	(xref_tag): Likewise.
> 	(start_enum): Likewise.
> 	* semantics.cc (begin_class_definition): Likewise.
> 	* module.cc (module_may_redeclare): Clean up logic. Emit error
> 	messages on failure.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/enum-12.C: Update error message.
> 	* g++.dg/modules/friend-5_b.C: Likewise.
> 	* g++.dg/modules/shadow-1_b.C: Likewise.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/cp-tree.h                          |   2 +-
>  gcc/cp/decl.cc                            |  28 +----
>  gcc/cp/module.cc                          | 120 ++++++++++++++--------
>  gcc/cp/semantics.cc                       |   6 +-
>  gcc/testsuite/g++.dg/modules/enum-12.C    |   2 +-
>  gcc/testsuite/g++.dg/modules/friend-5_b.C |   2 +-
>  gcc/testsuite/g++.dg/modules/shadow-1_b.C |   5 +-
>  7 files changed, 89 insertions(+), 76 deletions(-)
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 1dbb577a38d..faa7a0052a5 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7401,7 +7401,7 @@ inline bool module_exporting_p ()
>  
>  extern module_state *get_module (tree name, module_state *parent = NULL,
>  				 bool partition = false);
> -extern bool module_may_redeclare (tree decl);
> +extern bool module_may_redeclare (tree olddecl, tree newdecl = NULL);
>  
>  extern bool module_global_init_needed ();
>  extern bool module_determine_import_inits ();
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 65ab64885ff..aa66da4829d 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -2279,18 +2279,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
>        && TREE_CODE (olddecl) != NAMESPACE_DECL
>        && !hiding)
>      {
> -      if (!module_may_redeclare (olddecl))
> -	{
> -	  if (DECL_ARTIFICIAL (olddecl))
> -	    error ("declaration %qD conflicts with builtin", newdecl);
> -	  else
> -	    {
> -	      error ("declaration %qD conflicts with import", newdecl);
> -	      inform (olddecl_loc, "import declared %q#D here", olddecl);
> -	    }
> -
> -	  return error_mark_node;
> -	}
> +      if (!module_may_redeclare (olddecl, newdecl))
> +	return error_mark_node;
>  
>        tree not_tmpl = STRIP_TEMPLATE (olddecl);
>        if (DECL_LANG_SPECIFIC (not_tmpl)
> @@ -16620,12 +16610,7 @@ xref_tag (enum tag_types tag_code, tree name,
>  	{
>  	  tree decl = TYPE_NAME (t);
>  	  if (!module_may_redeclare (decl))
> -	    {
> -	      auto_diagnostic_group d;
> -	      error ("cannot declare %qD in a different module", decl);
> -	      inform (DECL_SOURCE_LOCATION (decl), "previously declared here");
> -	      return error_mark_node;
> -	    }
> +	    return error_mark_node;
>  
>  	  tree not_tmpl = STRIP_TEMPLATE (decl);
>  	  if (DECL_LANG_SPECIFIC (not_tmpl)
> @@ -16973,12 +16958,7 @@ start_enum (tree name, tree enumtype, tree underlying_type,
>  	{
>  	  tree decl = TYPE_NAME (enumtype);
>  	  if (!module_may_redeclare (decl))
> -	    {
> -	      auto_diagnostic_group d;
> -	      error ("cannot declare %qD in different module", decl);
> -	      inform (DECL_SOURCE_LOCATION (decl), "previously declared here");
> -	      enumtype = error_mark_node;
> -	    }
> +	    enumtype = error_mark_node;
>  	  else
>  	    set_instantiating_module (decl);
>  	}
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 001430a4a8f..e2d2910ae48 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -18992,11 +18992,15 @@ get_importing_module (tree decl, bool flexible)
>    return module->mod;
>  }
>  
> -/* Is it permissible to redeclare DECL.  */
> +/* Is it permissible to redeclare OLDDECL with NEWDECL.
> +
> +   If NEWDECL is NULL, assumes that OLDDECL will be redeclared using
> +   the current scope's module and attachment.  */
>  
>  bool
> -module_may_redeclare (tree decl)
> +module_may_redeclare (tree olddecl, tree newdecl)
>  {
> +  tree decl = olddecl;
>    for (;;)
>      {
>        tree ctx = CP_DECL_CONTEXT (decl);
> @@ -19010,58 +19014,94 @@ module_may_redeclare (tree decl)
>        decl = TYPE_NAME (ctx);
>      }
>  
> -  tree not_tmpl = STRIP_TEMPLATE (decl);
> -
>    int use_tpl = 0;
> -  if (node_template_info (not_tmpl, use_tpl) && use_tpl)
> +  if (node_template_info (STRIP_TEMPLATE (decl), use_tpl) && use_tpl)
>      // Specializations of any kind can be redeclared anywhere.
>      // FIXME: Should we be checking this in more places on the scope chain?
>      return true;
>  
> -  if (!DECL_LANG_SPECIFIC (not_tmpl) || !DECL_MODULE_ATTACH_P (not_tmpl))
> -    // Decl is attached to global module.  Current scope needs to be too.
> -    return !module_attach_p ();
> +  module_state *old_mod = (*modules)[0];
> +  module_state *new_mod = old_mod;
>  
> -  module_state *me = (*modules)[0];
> -  module_state *them = me;
> +  tree old_origin = get_originating_module_decl (decl);
> +  tree old_inner = STRIP_TEMPLATE (old_origin);
> +  bool olddecl_attached_p = (DECL_LANG_SPECIFIC (old_inner)
> +			     && DECL_MODULE_ATTACH_P (old_inner));
> +  if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P (old_inner))
> +    {
> +      unsigned index = import_entity_index (old_origin);
> +      old_mod = import_entity_module (index);
> +    }
>  
> -  if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl))
> +  bool newdecl_attached_p = module_attach_p ();
> +  if (newdecl)
>      {
> -      /* We can be given the TEMPLATE_RESULT.  We want the
> -	 TEMPLATE_DECL.  */
> -      int use_tpl = -1;
> -      if (tree ti = node_template_info (decl, use_tpl))
> +      tree new_origin = get_originating_module_decl (newdecl);
> +      tree new_inner = STRIP_TEMPLATE (new_origin);
> +      newdecl_attached_p = (DECL_LANG_SPECIFIC (new_inner)
> +			    && DECL_MODULE_ATTACH_P (new_inner));
> +      if (DECL_LANG_SPECIFIC (new_inner) && DECL_MODULE_IMPORT_P (new_inner))
>  	{
> -	  tree tmpl = TI_TEMPLATE (ti);
> -	  if (use_tpl == 2)
> -	    {
> -	      /* A partial specialization.  Find that specialization's
> -		 template_decl.  */
> -	      for (tree list = DECL_TEMPLATE_SPECIALIZATIONS (tmpl);
> -		   list; list = TREE_CHAIN (list))
> -		if (DECL_TEMPLATE_RESULT (TREE_VALUE (list)) == decl)
> -		  {
> -		    decl = TREE_VALUE (list);
> -		    break;
> -		}
> -	    }
> -	  else if (DECL_TEMPLATE_RESULT (tmpl) == decl)
> -	    decl = tmpl;
> +	  unsigned index = import_entity_index (new_origin);
> +	  new_mod = import_entity_module (index);
>  	}
> -      unsigned index = import_entity_index (decl);
> -      them = import_entity_module (index);
>      }
>  
> -  // Decl is attached to named module.  Current scope needs to be
> -  // attaching to the same module.
> -  if (!module_attach_p ())
> -    return false;
> +  /* Module attachment needs to match.  */
> +  if (olddecl_attached_p == newdecl_attached_p)
> +    {
> +      if (!olddecl_attached_p)
> +	/* Both are GM entities, OK.  */
> +	return true;
>  
> -  // Both attached to named module.
> -  if (me == them)
> -    return true;
> +      if (new_mod == old_mod
> +	  || (new_mod && get_primary (new_mod) == get_primary (old_mod)))
> +	/* Both attached to same named module, OK.  */
> +	return true;
> +    }
> +
> +  /* Attached to different modules, error.  */
> +  decl = newdecl ? newdecl : olddecl;
> +  location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
> +  if (DECL_ARTIFICIAL (olddecl) && !DECL_IMPLICIT_TYPEDEF_P (olddecl))
> +    error_at (loc, "declaration %qD conflicts with builtin", decl);
> +  else if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P (old_inner))
> +    {
> +      auto_diagnostic_group d;
> +      if (newdecl_attached_p)
> +	error_at (loc, "redeclaring %qD in module %qs conflicts with import",
> +		  decl, new_mod->get_flatname ());
> +      else
> +	error_at (loc, "redeclaring %qD in global module conflicts with import",
> +		  decl);
>  
> -  return me && get_primary (them) == get_primary (me);
> +      if (olddecl_attached_p)
> +	inform (DECL_SOURCE_LOCATION (olddecl),
> +		"import declared attached to module %qs",
> +		old_mod->get_flatname ());
> +      else
> +	inform (DECL_SOURCE_LOCATION (olddecl),
> +		"import declared in global module");
> +    }
> +  else
> +    {
> +      auto_diagnostic_group d;
> +      if (newdecl_attached_p)
> +	error_at (loc, "conflicting declaration of %qD in module %qs",
> +		  decl, new_mod->get_flatname ());
> +      else
> +	error_at (loc, "conflicting declaration of %qD in global module",
> +		  decl);
> +
> +      if (olddecl_attached_p)
> +	inform (DECL_SOURCE_LOCATION (olddecl),
> +		"previously declared in module %qs",
> +		old_mod->get_flatname ());
> +      else
> +	inform (DECL_SOURCE_LOCATION (olddecl),
> +		"previously declared in global module");
> +    }
> +  return false;
>  }
>  
>  /* DECL is being created by this TU.  Record it came from here.  We
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 02c7c1bf5a4..2dde65a970b 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -3777,11 +3777,7 @@ begin_class_definition (tree t)
>    if (modules_p ())
>      {
>        if (!module_may_redeclare (TYPE_NAME (t)))
> -	{
> -	  error ("cannot declare %qD in a different module", TYPE_NAME (t));
> -	  inform (DECL_SOURCE_LOCATION (TYPE_NAME (t)), "declared here");
> -	  return error_mark_node;
> -	}
> +	return error_mark_node;
>        set_instantiating_module (TYPE_NAME (t));
>        set_defining_module (TYPE_NAME (t));
>      }
> diff --git a/gcc/testsuite/g++.dg/modules/enum-12.C b/gcc/testsuite/g++.dg/modules/enum-12.C
> index 57eeb85d92a..019c3da4218 100644
> --- a/gcc/testsuite/g++.dg/modules/enum-12.C
> +++ b/gcc/testsuite/g++.dg/modules/enum-12.C
> @@ -4,7 +4,7 @@
>  
>  export module foo;
>  namespace std {
> -  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "different module" }
> +  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "conflicting declaration" }
>  }
>  
>  // { dg-prune-output "not writing module" }
> diff --git a/gcc/testsuite/g++.dg/modules/friend-5_b.C b/gcc/testsuite/g++.dg/modules/friend-5_b.C
> index f043d7a340d..6b561265155 100644
> --- a/gcc/testsuite/g++.dg/modules/friend-5_b.C
> +++ b/gcc/testsuite/g++.dg/modules/friend-5_b.C
> @@ -4,7 +4,7 @@
>  export module bar;
>  import foo;
>  
> -class B { // { dg-error "in a different module" }
> +class B { // { dg-error "conflicts with import" }
>    B() { object.value = 42; }
>    A object;
>  };
> diff --git a/gcc/testsuite/g++.dg/modules/shadow-1_b.C b/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> index 646381237ac..7f6a3182998 100644
> --- a/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> +++ b/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> @@ -1,8 +1,5 @@
>  // { dg-additional-options -fmodules-ts }
>  import shadow;
>  
> -// unfortunately not the exact same diagnostic in both cases :(
> -
>  void stat (); // { dg-error "conflicts with import" }
> -
> -struct stat {}; // { dg-error "in a different module" }
> +struct stat {}; // { dg-error "conflicts with import" }
> -- 
> 2.43.2
> 
>