Re: [PATCH] c++/modules: more checks for exporting names with using-declarations

[Nathan Sidwell <nathan@acm.org>]

On 11/10/23 17:52, Nathaniel Shead wrote:
> I noticed while fixing PR106849 that we don't currently check validity
> of exporting non-functions via using-declarations either; this patch
> adds those checks factored out into a new function. I also tried to make
> the error messages a bit more descriptive.
> 
> This patch is based on [1] (with the adjustment to use STRIP_TEMPLATE
> Nathan mentioned), but could probably be a replacement for that patch if
> preferred - if so I'm happy to re-send rebased off master instead.
> 
> The ICEs mentioned in the commit message are caused by code such as
> 
>    export module M;
> 
>    namespace {
>      enum e { x };
>    }
>    export using ::e;
> 
> in depset::hash::finalize_dependencies when attempting to get a
> DECL_SOURCE_LOCATION of an OVERLOAD. I haven't fixed that in this patch
> though because after this patch I was no longer able to construct an
> example of that error, but it's maybe something to fix up later as well.
> 
> [1]: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635869.html
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write
> access.

ok thanks

> 
> -- >8 --
> 
> Currently only functions are directly checked for validity when
> exporting via a using-declaration.  This patch also checks exporting
> non-external names of variables, types, and enumerators.  This also
> prevents ICEs with `export using enum` for internal-linkage enums.
> 
> While we're at it we also improve the error messages for these cases to
> provide more context about what went wrong.
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (check_can_export_using_decl): New.
> 	(do_nonmember_using_decl): Use above to check if names can be
> 	exported.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/using-10.C: New test.
> 	* g++.dg/modules/using-enum-2.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/name-lookup.cc                       | 74 +++++++++++++++------
>   gcc/testsuite/g++.dg/modules/using-10.C     | 71 ++++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/using-enum-2.C | 23 +++++++
>   3 files changed, 148 insertions(+), 20 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-10.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-enum-2.C
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index e74084948b6..d19ea5d121c 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -4802,6 +4802,49 @@ pushdecl_outermost_localscope (tree x)
>     return b ? do_pushdecl_with_scope (x, b) : error_mark_node;
>   }
>   
> +/* Checks if BINDING is a binding that we can export.  */
> +
> +static bool
> +check_can_export_using_decl (tree binding)
> +{
> +  tree decl = STRIP_TEMPLATE (binding);
> +
> +  /* Linkage is determined by the owner of an enumerator.  */
> +  if (TREE_CODE (decl) == CONST_DECL)
> +    decl = TYPE_NAME (DECL_CONTEXT (decl));
> +
> +  /* If the using decl is exported, the things it refers
> +     to must also be exported (or not have module attachment).  */
> +  if (!DECL_MODULE_EXPORT_P (decl)
> +      && (DECL_LANG_SPECIFIC (decl)
> +	  && DECL_MODULE_ATTACH_P (decl)))
> +    {
> +      bool internal_p = !TREE_PUBLIC (decl);
> +
> +      /* A template in an anonymous namespace doesn't constrain TREE_PUBLIC
> +	 until it's instantiated, so double-check its context.  */
> +      if (!internal_p && TREE_CODE (binding) == TEMPLATE_DECL)
> +	internal_p = decl_internal_context_p (decl);
> +
> +      auto_diagnostic_group d;
> +      error ("exporting %q#D that does not have external linkage",
> +	     binding);
> +      if (TREE_CODE (decl) == TYPE_DECL && !DECL_IMPLICIT_TYPEDEF_P (decl))
> +	/* An un-exported explicit type alias has no linkage.  */
> +	inform (DECL_SOURCE_LOCATION (binding),
> +		"%q#D declared here with no linkage", binding);
> +      else if (internal_p)
> +	inform (DECL_SOURCE_LOCATION (binding),
> +		"%q#D declared here with internal linkage", binding);
> +      else
> +	inform (DECL_SOURCE_LOCATION (binding),
> +		"%q#D declared here with module linkage", binding);
> +      return false;
> +    }
> +
> +  return true;
> +}
> +
>   /* Process a local-scope or namespace-scope using declaration.  LOOKUP
>      is the result of qualified lookup (both value & type are
>      significant).  FN_SCOPE_P indicates if we're at function-scope (as
> @@ -4845,22 +4888,7 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
>   	  tree new_fn = *usings;
>   	  bool exporting = revealing_p && module_exporting_p ();
>   	  if (exporting)
> -	    {
> -	      tree decl = STRIP_TEMPLATE (new_fn);
> -
> -	      /* If the using decl is exported, the things it refers
> -		 to must also be exported (or not have module attachment).  */
> -	      if (!DECL_MODULE_EXPORT_P (decl)
> -		  && (DECL_LANG_SPECIFIC (decl)
> -		      && DECL_MODULE_ATTACH_P (decl)))
> -		{
> -		  auto_diagnostic_group d;
> -		  error ("%q#D does not have external linkage", new_fn);
> -		  inform (DECL_SOURCE_LOCATION (new_fn),
> -			  "%q#D declared here", new_fn);
> -		  exporting = false;
> -		}
> -	    }
> +	    exporting = check_can_export_using_decl (new_fn);
>   
>   	  /* [namespace.udecl]
>   
> @@ -4938,20 +4966,26 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
>         failed = true;
>       }
>     else if (insert_p)
> -    // FIXME:what if we're newly exporting lookup.value
> -    value = lookup.value;
> +    {
> +      value = lookup.value;
> +      if (revealing_p && module_exporting_p ())
> +	check_can_export_using_decl (value);
> +    }
>     
>     /* Now the type binding.  */
>     if (lookup.type && lookup.type != type)
>       {
> -      // FIXME: What if we're exporting lookup.type?
>         if (type && !decls_match (lookup.type, type))
>   	{
>   	  diagnose_name_conflict (lookup.type, type);
>   	  failed = true;
>   	}
>         else if (insert_p)
> -	type = lookup.type;
> +	{
> +	  type = lookup.type;
> +	  if (revealing_p && module_exporting_p ())
> +	    check_can_export_using_decl (type);
> +	}
>       }
>   
>     if (insert_p)
> diff --git a/gcc/testsuite/g++.dg/modules/using-10.C b/gcc/testsuite/g++.dg/modules/using-10.C
> new file mode 100644
> index 00000000000..5735353ee21
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-10.C
> @@ -0,0 +1,71 @@
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi !bad }
> +
> +export module bad;
> +
> +// internal linkage
> +namespace s {
> +  namespace {
> +    struct a1 {};  // { dg-message "declared here with internal linkage" }
> +
> +    template <typename T>
> +    struct b1;  // { dg-message "declared here with internal linkage" }
> +
> +    int x1;  // { dg-message "declared here with internal linkage" }
> +
> +    template <typename T>
> +    T y1;  // { dg-message "declared here with internal linkage" }
> +
> +    void f1();  // { dg-message "declared here with internal linkage" }
> +
> +    template <typename T>
> +    void g1();  // { dg-message "declared here with internal linkage" }
> +  }
> +}
> +
> +// module linkage
> +namespace m {
> +  struct a2 {};  // { dg-message "declared here with module linkage" }
> +
> +  template <typename T>
> +  struct b2;  // { dg-message "declared here with module linkage" }
> +
> +  int x2;  // { dg-message "declared here with module linkage" }
> +
> +  template <typename T>
> +  T y2;  // { dg-message "declared here with module linkage" }
> +
> +  void f2();  // { dg-message "declared here with module linkage" }
> +
> +  template <typename T>
> +  void g2();  // { dg-message "declared here with module linkage" }
> +}
> +
> +export using s::a1;  // { dg-error "does not have external linkage" }
> +export using s::b1;  // { dg-error "does not have external linkage" }
> +export using s::x1;  // { dg-error "does not have external linkage" }
> +export using s::y1;  // { dg-error "does not have external linkage" }
> +export using s::f1;  // { dg-error "does not have external linkage" }
> +export using s::g1;  // { dg-error "does not have external linkage" }
> +
> +export using m::a2;  // { dg-error "does not have external linkage" }
> +export using m::b2;  // { dg-error "does not have external linkage" }
> +export using m::x2;  // { dg-error "does not have external linkage" }
> +export using m::y2;  // { dg-error "does not have external linkage" }
> +export using m::f2;  // { dg-error "does not have external linkage" }
> +export using m::g2;  // { dg-error "does not have external linkage" }
> +
> +namespace t {
> +  using a = int;  // { dg-message "declared here with no linkage" }
> +
> +  template <typename T>
> +  using b = int;  // { dg-message "declared here with no linkage" }
> +
> +  typedef int c;  // { dg-message "declared here with no linkage" }
> +}
> +
> +export using t::a;  // { dg-error "does not have external linkage" }
> +export using t::b;  // { dg-error "does not have external linkage" }
> +export using t::c;  // { dg-error "does not have external linkage" }
> +
> +// { dg-prune-output "not writing module" }
> diff --git a/gcc/testsuite/g++.dg/modules/using-enum-2.C b/gcc/testsuite/g++.dg/modules/using-enum-2.C
> new file mode 100644
> index 00000000000..813e2f630ab
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-enum-2.C
> @@ -0,0 +1,23 @@
> +// { dg-additional-options "-fmodules-ts -std=c++2a" }
> +// { dg-module-cmi !bad }
> +
> +export module bad;
> +
> +namespace s {
> +  namespace {
> +    enum e1 { x1 };  // { dg-message "declared here with internal linkage" }
> +    enum class e2 { x2 };  // { dg-message "declared here with internal linkage" }
> +  }
> +}
> +
> +namespace m {
> +  enum e3 { x3 };  // { dg-message "declared here with module linkage" }
> +  enum class e4 { x4 };  // { dg-message "declared here with module linkage" }
> +}
> +
> +export using enum s::e1;  // { dg-error "does not have external linkage" }
> +export using enum s::e2;  // { dg-error "does not have external linkage" }
> +export using enum m::e3;  // { dg-error "does not have external linkage" }
> +export using enum m::e4;  // { dg-error "does not have external linkage" }
> +
> +// { dg-prune-output "not writing module" }

-- 
Nathan Sidwell