c++: Implement dependent ADL for use with modules

[module.global.frag] p3.3 says "A declaration D is decl-reachable from a
declaration S in the same translation unit if ... S contains a dependent
call E ([temp.dep]) and D is found by any name lookup performed for an
expression synthesized from E by replacing each type-dependent argument
or operand with a value of a placeholder type with no associated
namespaces or entities".

This requires doing partial ADL ondependent calls, in case there are
non-dependent arguments that would cause new functions to become
decl-reachable.  This patch implements this with an additional lookup
during modules streaming to find any such entities.

This causes us to do ADL in more circumstances; this means also that we
might instantiate templates in cases we didn't use to.  This could cause
issues given we have already started our modules walk at this point, or
break any otherwise valid existing code.  To fix this patch adds a flag
to do a "tentative" ADL pass which doesn't attempt to complete any types
(and hence cause instantiations to occur); this means that we might miss
some associated entities however.  During a tentative walk we can also
skip entities that we know won't contribute to the missing
decl-reachable set, as an optimisation.

One implementation limitation is that both modules tree walking and
name lookup marks tree nodes as TREE_VISITED for different purposes; to
avoid conflicts this patch caches calls that will require lookup in a
separate worklist to be processed after the walk is done.

	PR c++/122712

gcc/cp/ChangeLog:

	* module.cc (depset::hash::dep_adl_info): New type.
	(depset::hash::dep_adl_entity_list): New work list.
	(depset::hash::hash): Create it.
	(depset::hash::~hash): Release it.
	(trees_out::tree_value): Cache possibly dependent
	calls during tree walk.
	(depset::hash::add_dependent_adl_entities): New function.
	(depset::hash::find_dependencies): Process cached entities.
	* name-lookup.cc (name_lookup::tentative): New member.
	(name_lookup::name_lookup): Initialize it.
	(name_lookup::preserve_state): Propagate tentative from previous
	lookup.
	(name_lookup::adl_namespace_fns): Don't search imported bindings
	during tentative lookup.
	(name_lookup::adl_class): Don't attempt to complete class types
	during tentative lookup.
	(name_lookup::search_adl): Skip type-dependent args and avoid
	unnecessary work during tentative lookup.
	(lookup_arg_dependent): Add tentative parameter.
	* name-lookup.h (lookup_arg_dependent): Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/adl-12_a.C: New test.
	* g++.dg/modules/adl-12_b.C: New test.

Signed-off-by: Nathaniel Shead <[email protected]>
Reviewed-by: Jason Merrill <[email protected]>

Author: wreien

gcc/cp/module.cc

@@ -2628,17 +2628,33 @@ class depset {
     bool ignore_tu_local;    /* In a context where referencing a TU-local
 				entity is not an exposure.  */
 
+  private:
+    /* Information needed to do dependent ADL for discovering
+       more decl-reachable entities.  Cached during walking to
+       prevent tree marking from interfering with lookup.  */
+    struct dep_adl_info {
+      /* The name of the call or operator.  */
+      tree name = NULL_TREE;
+      /* If not ERROR_MARK, a rewrite candidate for this operator.  */
+      tree_code rewrite = ERROR_MARK;
+      /* Argument list for the call.  */
+      vec<tree, va_gc>* args = make_tree_vector ();
+    };
+    vec<dep_adl_info> dep_adl_entity_list;
+
   public:
     hash (size_t size, hash *c = NULL)
       : parent (size), chain (c), current (NULL), section (0),
 	reached_unreached (false), writing_merge_key (false),
 	ignore_tu_local (false)
     {
       worklist.create (size);
+      dep_adl_entity_list.create (16);
     }
     ~hash ()
     {
       worklist.release ();
+      dep_adl_entity_list.release ();
     }
 
   public:
@@ -2671,6 +2687,7 @@ class depset {
     void add_specializations (bool decl_p);
     void add_partial_entities (vec<tree, va_gc> *);
     void add_class_entities (vec<tree, va_gc> *);
+    void add_dependent_adl_entities (tree expr);
 
   private:
     void add_deduction_guides (tree decl);
@@ -9803,6 +9820,9 @@ trees_out::tree_value (tree t)
 				  && !DECL_CONTEXT (t)))
 			  && TREE_CODE (t) != FUNCTION_DECL));
 
+  if (is_initial_scan () && EXPR_P (t))
+    dep_hash->add_dependent_adl_entities (t);
+
   if (streaming_p ())
     {
       /* A new node -> tt_node.  */
@@ -14980,6 +15000,90 @@ depset::hash::add_class_entities (vec<tree, va_gc> *class_members)
     }
 }
 
+/* Add any entities found via dependent ADL.  */
+
+void
+depset::hash::add_dependent_adl_entities (tree expr)
+{
+  gcc_checking_assert (!is_key_order ());
+  if (TREE_CODE (current->get_entity ()) != TEMPLATE_DECL)
+    return;
+
+  dep_adl_info info;
+  switch (TREE_CODE (expr))
+    {
+    case CALL_EXPR:
+      if (!KOENIG_LOOKUP_P (expr))
+	return;
+      info.name = CALL_EXPR_FN (expr);
+      if (!info.name)
+	return;
+      if (TREE_CODE (info.name) == TEMPLATE_ID_EXPR)
+	info.name = TREE_OPERAND (info.name, 0);
+      if (TREE_CODE (info.name) == TU_LOCAL_ENTITY)
+	return;
+      if (!identifier_p (info.name))
+	info.name = OVL_NAME (info.name);
+      for (int ix = 0; ix < call_expr_nargs (expr); ix++)
+	vec_safe_push (info.args, CALL_EXPR_ARG (expr, ix));
+      break;
+
+    case LE_EXPR:
+    case GE_EXPR:
+    case LT_EXPR:
+    case GT_EXPR:
+      info.rewrite = SPACESHIP_EXPR;
+      goto overloadable_expr;
+
+    case NE_EXPR:
+      info.rewrite = EQ_EXPR;
+      goto overloadable_expr;
+
+    case EQ_EXPR:
+      /* Not strictly a rewrite candidate, but we need to ensure
+	 that lookup of a matching NE_EXPR can succeed if that
+	 would inhibit a rewrite with reversed parameters.  */
+      info.rewrite = NE_EXPR;
+      goto overloadable_expr;
+
+    case COMPOUND_EXPR:
+    case MEMBER_REF:
+    case MULT_EXPR:
+    case TRUNC_DIV_EXPR:
+    case TRUNC_MOD_EXPR:
+    case PLUS_EXPR:
+    case MINUS_EXPR:
+    case LSHIFT_EXPR:
+    case RSHIFT_EXPR:
+    case SPACESHIP_EXPR:
+    case BIT_AND_EXPR:
+    case BIT_XOR_EXPR:
+    case BIT_IOR_EXPR:
+    case TRUTH_ANDIF_EXPR:
+    case TRUTH_ORIF_EXPR:
+    overloadable_expr:
+      info.name = ovl_op_identifier (TREE_CODE (expr));
+      gcc_checking_assert (tree_operand_length (expr) == 2);
+      vec_safe_push (info.args, TREE_OPERAND (expr, 0));
+      vec_safe_push (info.args, TREE_OPERAND (expr, 1));
+      break;
+
+    default:
+      return;
+    }
+
+  /* If all arguments are type-dependent we don't need to do
+     anything further, we won't find new entities.  */
+  processing_template_decl_sentinel ptds;
+  ++processing_template_decl;
+  if (!any_type_dependent_arguments_p (info.args))
+    return;
+
+  /* We need to defer name lookup until after walking, otherwise
+     we get confused by stray TREE_VISITEDs.  */
+  dep_adl_entity_list.safe_push (info);
+}
+
 /* We add the partial & explicit specializations, and the explicit
    instantiations.  */
 
@@ -15341,6 +15445,31 @@ depset::hash::find_dependencies (module_state *module)
 		  && deduction_guide_p (decl))
 		add_deduction_guides (TYPE_NAME (TREE_TYPE (TREE_TYPE (decl))));
 
+	      /* Handle dependent ADL for [module.global.frag] p3.3.  */
+	      if (!is_key_order () && !dep_adl_entity_list.is_empty ())
+		{
+		  processing_template_decl_sentinel ptds;
+		  ++processing_template_decl;
+		  for (auto &info : dep_adl_entity_list)
+		    {
+		      tree lookup = lookup_arg_dependent (info.name, NULL_TREE,
+							  info.args, true);
+		      for (tree fn : lkp_range (lookup))
+			add_dependency (make_dependency (fn, EK_DECL));
+
+		      if (info.rewrite)
+			{
+			  tree rewrite_name = ovl_op_identifier (info.rewrite);
+			  lookup = lookup_arg_dependent (rewrite_name, NULL_TREE,
+							 info.args, true);
+			  for (tree fn : lkp_range (lookup))
+			    add_dependency (make_dependency (fn, EK_DECL));
+			}
+		      release_tree_vector (info.args);
+		    }
+		  dep_adl_entity_list.truncate (0);
+		}
+
 	      if (!is_key_order ()
 		  && TREE_CODE (decl) == TEMPLATE_DECL
 		  && !DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl))

gcc/cp/name-lookup.cc

@@ -449,6 +449,8 @@ class name_lookup
 
   LOOK_want want;  /* What kind of entity we want.  */
 
+  bool tentative;  /* The lookup is just to find additional decl-reachable
+		      entities in this TU during modules streaming.  */
   bool deduping; /* Full deduping is needed because using declarations
 		    are in play.  */
   vec<tree, va_heap, vl_embed> *scopes;
@@ -463,7 +465,7 @@ class name_lookup
 public:
   name_lookup (tree n, LOOK_want w = LOOK_want::NORMAL)
   : name (n), value (NULL_TREE), type (NULL_TREE),
-    want (w),
+    want (w), tentative (false),
     deduping (false), scopes (NULL), previous (NULL)
   {
     preserve_state ();
@@ -602,6 +604,8 @@ name_lookup::preserve_state ()
 	    }
 	}
 
+      tentative = previous->tentative;
+
       /* Unmark the outer partial lookup.  */
       if (previous->deduping)
 	lookup_mark (previous->value, false);
@@ -1252,6 +1256,11 @@ name_lookup::adl_namespace_fns (tree scope, bitmap imports,
 	      add_fns (ovl_skip_hidden (MAYBE_STAT_DECL (bind)));
 	    }
 
+	  /* When doing tentative name lookup we only care about entities
+	     in the current TU.  */
+	  if (tentative)
+	    return;
+
 	  /* Scan the imported bindings.  */
 	  unsigned ix = BINDING_VECTOR_NUM_CLUSTERS (val);
 	  if (BINDING_VECTOR_SLOTS_PER_CLUSTER == BINDING_SLOTS_FIXED)
@@ -1484,7 +1493,12 @@ name_lookup::adl_class (tree type)
   if (found_p (type))
     return;
 
-  complete_type (type);
+  /* Don't instantiate if we don't have to so we don't unnecessarily error
+     on incomplete types during modules streaming.  This does however mean
+     we incorrectly miss some decl-reachable entities (PR c++/123235).  */
+  if (!tentative)
+    complete_type (type);
+
   adl_bases (type);
   mark_found (type);
 
@@ -1679,7 +1693,10 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> *args)
        first arg. */
     if (TYPE_P (arg))
       adl_type (arg);
-    else
+    /* When processing a module CMI we might get a type-dependent
+       argument: treat as a placeholder with no associated namespace
+       or entities.  */
+    else if (!tentative || !type_dependent_expression_p (arg))
       adl_expr (arg);
 
   if (vec_safe_length (scopes))
@@ -1690,10 +1707,11 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> *args)
 	dedup (true);
 
       /* First get the attached modules for each innermost non-inline
-	 namespace of an associated entity.  */
+	 namespace of an associated entity.  This isn't needed for
+	 tentative lookup, as we're only interested in the current TU.  */
       bitmap_obstack_initialize (NULL);
       hash_map<tree, bitmap> ns_mod_assocs;
-      if (modules_p ())
+      if (modules_p () && !tentative)
 	{
 	  for (tree scope : scopes)
 	    if (TYPE_P (scope))
@@ -1732,7 +1750,10 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> *args)
 	      adl_namespace_fns (scope, visible, inst_path,
 				 assocs ? *assocs : NULL);
 	    }
-	  else if (RECORD_OR_UNION_TYPE_P (scope))
+	  else if (RECORD_OR_UNION_TYPE_P (scope) && !tentative)
+	    /* We don't need to look at friends when searching for
+	       new decl-reachable entities as they will already be
+	       considered reachable by importers.  */
 	    adl_class_fns (scope);
 	}
 
@@ -1756,13 +1777,17 @@ static void consider_binding_level (tree name,
 
 /* ADL lookup of NAME.  FNS is the result of regular lookup, and we
    don't add duplicates to it.  ARGS is the vector of call
-   arguments (which will not be empty).  */
+   arguments (which will not be empty).  TENTATIVE is true when
+   this is early lookup only for the purpose of finding more
+   decl-reachable declarations.  */
 
 tree
-lookup_arg_dependent (tree name, tree fns, vec<tree, va_gc> *args)
+lookup_arg_dependent (tree name, tree fns, vec<tree, va_gc> *args,
+		      bool tentative/*=false*/)
 {
   auto_cond_timevar tv (TV_NAME_LOOKUP);
   name_lookup lookup (name);
+  lookup.tentative = tentative;
   return lookup.search_adl (fns, args);
 }
 

gcc/cp/name-lookup.h

@@ -461,7 +461,8 @@ extern void push_decl_namespace (tree);
 extern void pop_decl_namespace (void);
 extern void do_namespace_alias (location_t, tree, tree);
 extern tree do_class_using_decl (tree, tree);
-extern tree lookup_arg_dependent (tree, tree, vec<tree, va_gc> *);
+extern tree lookup_arg_dependent (tree, tree, vec<tree, va_gc> *,
+				  bool tentative = false);
 extern tree search_anon_aggr (tree, tree, bool = false);
 extern tree get_class_binding_direct (tree, tree, bool want_type = false);
 extern tree get_class_binding (tree, tree, bool want_type = false);

gcc/testsuite/g++.dg/modules/adl-12_a.C

@@ -0,0 +1,94 @@
+// { dg-additional-options "-fmodules -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+// Check we implement [module.global.frag] p2.3:
+// A declaration D is decl-reachable from a declaration S in the same
+// translation unit if: [...] S contains a dependent call E ([temp.dep]) and
+// D is found by any name lookup performed for an expression synthesized from
+// E by replacing each type-dependent argument or operand with a value of a
+// placeholder type with no associated namespaces or entities.
+
+module;
+namespace Q {
+  struct X {};
+  void g_impl(X, X);
+  void operator-(X, X);
+  void go_partial(X, int);
+  void operator/(X, int);
+}
+namespace P {
+  struct X {};
+}
+#if __cpp_impl_three_way_comparison >= 201907L
+namespace ops1 {
+  struct Y {};
+  int operator<=>(Y, int);
+  bool operator==(Y, int);
+}
+namespace ops2 {
+  struct Y {};
+  bool operator==(Y, int);
+  bool operator!=(Y, int);
+}
+#endif
+namespace Incomplete {
+  template <typename T> struct Holder { T t; };
+  struct Z;
+  void go(int, void*);
+}
+namespace C {
+  struct G {};
+  void qux(int, void*);
+}
+template <typename T> struct H : C::G {};
+export module M;
+
+export template <typename T>
+void g(T t) {
+  g_impl(t, Q::X{});  // ADL in definition finds Q::g_impl, g_impl not discarded
+  // { dg-final { scan-lang-dump "Bindings '::Q::g_impl'" module } }
+
+  t - Q::X{};  // Similarly for Q::operator-
+  // { dg-final { scan-lang-dump "Bindings '::Q::operator-'" module } }
+}
+
+export template <typename T> struct Partial {
+  template <typename U> static decltype(go_partial(T{}, U{})) f();
+  template <typename U> static decltype(T{} / U{}) o();
+};
+// The instantantiation of Partial<Q::X> should emit go_partial and operator/
+template struct Partial<Q::X>;
+// { dg-final { scan-lang-dump "Bindings '::Q::go_partial'" module } }
+// { dg-final { scan-lang-dump "Bindings '::Q::operator/'" module } }
+
+#if __cpp_impl_three_way_comparison >= 201907L
+export template <typename T>
+void rewrite_ops(T t) {
+  // Rewritten operators should also look for the ops they rewrite to.
+  t < ops1::Y{};
+  t != ops1::Y{};
+  // { dg-final { scan-lang-dump {Bindings '::ops1::operator<=>'} module { target c++20 } } }
+  // { dg-final { scan-lang-dump {Bindings '::ops1::operator=='} module { target c++20 } } }
+}
+export template <typename T>
+void rewrite_ops_error(T t) {
+  // Test we bind != to prevent considering == as a rewrite target.
+  t == ops2::Y{};
+  // { dg-final { scan-lang-dump "Bindings '::ops2::operator!='" module { target c++20 } } }
+}
+#endif
+
+export template <typename T>
+void incomplete(T t) {
+  Incomplete::Holder<Incomplete::Z>* holder;
+  go(t, holder);  // Shouldn't attempt to instantiate unnecessarily here
+}
+
+export template <typename T>
+void needs_completion(T t) {
+  H<int>* holder;
+  // C::qux should be found via H<T>'s base, C::G.
+  // But we don't see this because we don't attempt to complete the type.
+  qux(t, holder);
+  // { dg-final { scan-lang-dump "Bindings '::C::qux'" module { xfail *-*-* } } }
+}