[RFC] c++: modules and using-directives

  Tested x86_64-pc-linux-gnu.

Does this approach make sense to you?  Any other ideas?

-- 8< --

We weren't representing 'using namespace' at all in modules, which broke
some of the <chrono> literals tests.

I experimented with various approaches to representing them, and ended up
with emitting them as a pseudo-binding for "using", which as a keyword can't
have any real bindings.  Then reading this pseudo-binding adds it to
using_directives instead of the usual handling.

gcc/cp/ChangeLog:

	* module.cc (depset::hash::add_binding_entity): Handle
	using-directive as a binding of "using".
	(depset::hash::add_namespace_entities): Record using-directives.
	(module_state::read_cluster): Read using-directive.
	* name-lookup.cc (name_lookup::search_namespace_only):
	Look up "using" first.
	(add_using_namespace): New overload.
	* name-lookup.h (add_using_namespace): Declare.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/namespace-8_a.C: New test.
	* g++.dg/modules/namespace-8_b.C: New test.
---
 gcc/cp/name-lookup.h                         |  1 +
 gcc/cp/module.cc                             | 37 +++++++++++++++++---
 gcc/cp/name-lookup.cc                        | 12 +++++++
 gcc/testsuite/g++.dg/modules/namespace-8_a.C | 12 +++++++
 gcc/testsuite/g++.dg/modules/namespace-8_b.C |  8 +++++
 5 files changed, 65 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/namespace-8_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/namespace-8_b.C


base-commit: 2fd9aef1db1a4260ee823bc3a3d4cfc22e95c543
  

On Wed, Nov 27, 2024 at 12:03:23AM -0500, Jason Merrill wrote:
> Tested x86_64-pc-linux-gnu.
> 
> Does this approach make sense to you?  Any other ideas?
> 
> -- 8< --
> 
> We weren't representing 'using namespace' at all in modules, which broke
> some of the <chrono> literals tests.
> 
> I experimented with various approaches to representing them, and ended up
> with emitting them as a pseudo-binding for "using", which as a keyword can't
> have any real bindings.  Then reading this pseudo-binding adds it to
> using_directives instead of the usual handling.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (depset::hash::add_binding_entity): Handle
> 	using-directive as a binding of "using".
> 	(depset::hash::add_namespace_entities): Record using-directives.
> 	(module_state::read_cluster): Read using-directive.
> 	* name-lookup.cc (name_lookup::search_namespace_only):
> 	Look up "using" first.
> 	(add_using_namespace): New overload.
> 	* name-lookup.h (add_using_namespace): Declare.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/namespace-8_a.C: New test.
> 	* g++.dg/modules/namespace-8_b.C: New test.
> ---
>  gcc/cp/name-lookup.h                         |  1 +
>  gcc/cp/module.cc                             | 37 +++++++++++++++++---
>  gcc/cp/name-lookup.cc                        | 12 +++++++
>  gcc/testsuite/g++.dg/modules/namespace-8_a.C | 12 +++++++
>  gcc/testsuite/g++.dg/modules/namespace-8_b.C |  8 +++++
>  5 files changed, 65 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/namespace-8_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/namespace-8_b.C
> 
> diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
> index 54edadeed7f..50f5fe8d8c7 100644
> --- a/gcc/cp/name-lookup.h
> +++ b/gcc/cp/name-lookup.h
> @@ -462,6 +462,7 @@ extern cxx_binding *outer_binding (tree, cxx_binding *, bool);
>  extern void cp_emit_debug_info_for_using (tree, tree);
>  
>  extern void finish_nonmember_using_decl (tree scope, tree name);
> +extern void add_using_namespace (tree, tree);
>  extern void finish_using_directive (tree target, tree attribs);
>  void push_local_extern_decl_alias (tree decl);
>  extern tree pushdecl (tree, bool hiding = false);
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ddede0fdd43..72ded1062d0 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -13310,7 +13310,11 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
>    auto data = static_cast <add_binding_data *> (data_);
>    decl = strip_using_decl (decl);
>  
> -  if (!(TREE_CODE (decl) == NAMESPACE_DECL && !DECL_NAMESPACE_ALIAS (decl)))
> +  bool using_directive = (TREE_CODE (decl) == NAMESPACE_DECL
> +			  && (flags & WMB_Using));
> +
> +  if (!(TREE_CODE (decl) == NAMESPACE_DECL && !DECL_NAMESPACE_ALIAS (decl))
> +      || using_directive)
>      {
>        tree inner = decl;
>  
> @@ -13408,11 +13412,13 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
>        /* We're adding something.  */
>        if (!data->binding)
>  	{
> -	  data->binding = make_binding (data->ns, DECL_NAME (decl));
> +	  tree name = DECL_NAME (decl);
> +	  if (using_directive)
> +	    name = get_identifier ("using");
> +	  data->binding = make_binding (data->ns, name);
>  	  data->hash->add_namespace_context (data->binding, data->ns);
>  
> -	  depset **slot = data->hash->binding_slot (data->ns,
> -						    DECL_NAME (decl), true);
> +	  depset **slot = data->hash->binding_slot (data->ns, name, true);
>  	  gcc_checking_assert (!*slot);
>  	  *slot = data->binding;
>  	}
> @@ -13492,6 +13498,21 @@ depset::hash::add_namespace_entities (tree ns, bitmap partitions)
>  	count++;
>      }
>  
> +  /* Record using-directives.  */
> +  for (auto used: NAMESPACE_LEVEL (ns)->using_directives)
> +    {
> +      data.binding = nullptr;
> +      if (!TREE_PUBLIC (used))
> +	/* Anonymous namespaces are TU-local.  */;
> +      else if (DECL_NAMESPACE_INLINE_P (used)
> +	       && is_nested_namespace (ns, used, /*inline only*/true))
> +	/* Avoid redundant using of inline namespace.  */;
> +      else
> +	/* ??? should we try to distinguish whether the using-directive
> +	   is purview/exported?  */
> +	add_binding_entity (used, WMB_Flags(WMB_Using|WMB_Purview), &data);

I don't think the standard is entirely clear about how using-directives
should interact with modules; they don't declare names, and before P2615
were in fact forbidden from being explicitly exported, which implies to
me that the intention was for them to not be considered outside of the
declaring module.

That said, if we were to do this I would think the logic should match
what we do for any other name, in terms of requiring it to be explicitly
exported/purview as required; in particular, I would hope that something
like this doesn't happen:

  // m.cpp
  export module M;
  using namespace std;

  // test.cpp
  #include <iostream>
  import M;
  int main() {
    cout << "hello\n";  // using-directive "inherited" from M?
  }

> +    }
> +
>    if (count)
>      dump () && dump ("Found %u entries", count);
>    dump.outdent ();
> @@ -15584,7 +15605,13 @@ module_state::read_cluster (unsigned snum)
>  		else
>  		  {
>  		    if ((flags & cbf_using) &&
> -			!DECL_DECLARES_FUNCTION_P (decl))
> +			TREE_CODE (decl) == NAMESPACE_DECL)
> +		      {
> +			add_using_namespace (ns, decl);
> +			decls = void_node;
> +		      }
> +		    else if ((flags & cbf_using) &&
> +			     !DECL_DECLARES_FUNCTION_P (decl))
>  		      {
>  			/* We should only see a single non-function using-decl
>  			   for a binding; more than that would clash.  */
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 30dfbfe2e48..45ae933bdc1 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -847,6 +847,11 @@ bool
>  name_lookup::search_namespace_only (tree scope)
>  {
>    bool found = false;
> +  if (modules_p () && name && !id_equal (name, "using"))
> +    {
> +      name_lookup u (get_identifier ("using"));
> +      u.search_namespace_only (scope);
> +    }

Could we just add to the list of using-directives within read_namespaces
perhaps?  Probably as a second pass after all namespaces have been
created so that we don't run into issues with circular directives.
That would mean we wouldn't need to do this in every lookup.

Nathaniel

>    if (tree *binding = find_namespace_slot (scope, name))
>      {
>        tree val = *binding;
> @@ -8912,6 +8917,13 @@ add_using_namespace (vec<tree, va_gc> *&usings, tree target)
>    vec_safe_push (usings, target);
>  }
>  
> +void
> +add_using_namespace (tree ns, tree target)
> +{
> +  add_using_namespace (NAMESPACE_LEVEL (ns)->using_directives,
> +		       ORIGINAL_NAMESPACE (target));
> +}
> +
>  /* Tell the debug system of a using directive.  */
>  
>  static void
> diff --git a/gcc/testsuite/g++.dg/modules/namespace-8_a.C b/gcc/testsuite/g++.dg/modules/namespace-8_a.C
> new file mode 100644
> index 00000000000..67ffc6a8bfa
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/namespace-8_a.C
> @@ -0,0 +1,12 @@
> +// { dg-additional-options "-fmodules" }
> +
> +export module M;
> +
> +export namespace B
> +{
> +  int i;
> +}
> +export namespace C
> +{
> +  using namespace B;
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/namespace-8_b.C b/gcc/testsuite/g++.dg/modules/namespace-8_b.C
> new file mode 100644
> index 00000000000..7db35bf955e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/namespace-8_b.C
> @@ -0,0 +1,8 @@
> +// { dg-additional-options "-fmodules" }
> +
> +import M;
> +
> +int main()
> +{
> +  C::i = 42;
> +}
> 
> base-commit: 2fd9aef1db1a4260ee823bc3a3d4cfc22e95c543
> -- 
> 2.47.0
>
  

On 11/27/24 1:43 AM, Nathaniel Shead wrote:
> On Wed, Nov 27, 2024 at 12:03:23AM -0500, Jason Merrill wrote:
>> Tested x86_64-pc-linux-gnu.
>>
>> Does this approach make sense to you?  Any other ideas?
>>
>> -- 8< --
>>
>> We weren't representing 'using namespace' at all in modules, which broke
>> some of the <chrono> literals tests.
>>
>> I experimented with various approaches to representing them, and ended up
>> with emitting them as a pseudo-binding for "using", which as a keyword can't
>> have any real bindings.  Then reading this pseudo-binding adds it to
>> using_directives instead of the usual handling.
>>
>> +	/* ??? should we try to distinguish whether the using-directive
>> +	   is purview/exported?  */
>> +	add_binding_entity (used, WMB_Flags(WMB_Using|WMB_Purview), &data);
> 
> I don't think the standard is entirely clear about how using-directives
> should interact with modules; they don't declare names, and before P2615
> were in fact forbidden from being explicitly exported, which implies to
> me that the intention was for them to not be considered outside of the
> declaring module.

P2615 is certainly clear about allowing them.  Given that, I think the 
general rules of [module.interface] apply, so it should be found by name 
lookup in an importing TU.

> That said, if we were to do this I would think the logic should match
> what we do for any other name, in terms of requiring it to be explicitly
> exported/purview as required; in particular, I would hope that something
> like this doesn't happen:
> 
>    // m.cpp
>    export module M;
>    using namespace std;
> 
>    // test.cpp
>    #include <iostream>
>    import M;
>    int main() {
>      cout << "hello\n";  // using-directive "inherited" from M?
>    }

Good point, I have more work to do.

I think that since ADL doesn't consider using-directives, we only need 
to represent the exported ones?

>>   name_lookup::search_namespace_only (tree scope)
>>   {
>>     bool found = false;
>> +  if (modules_p () && name && !id_equal (name, "using"))
>> +    {
>> +      name_lookup u (get_identifier ("using"));
>> +      u.search_namespace_only (scope);
>> +    }
> 
> Could we just add to the list of using-directives within read_namespaces
> perhaps?  Probably as a second pass after all namespaces have been
> created so that we don't run into issues with circular directives.
> That would mean we wouldn't need to do this in every lookup.

That was my first thought, but I had trouble figuring out how.  Perhaps 
I'll try again.

Jason