Re: [PATCH] c++: refine CWG 2369 satisfaction vs non-dep convs [PR99599]

[Patrick Palka <ppalka@redhat.com>]

On Wed, 23 Aug 2023, Jason Merrill wrote:

> On 8/21/23 21:51, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look like
> > a reasonable approach?  I didn't observe any compile time/memory impact
> > of this change.
> > 
> > -- >8 --
> > 
> > As described in detail in the PR, CWG 2369 has the surprising
> > consequence of introducing constraint recursion in seemingly valid and
> > innocent code.
> > 
> > This patch attempts to fix this surpising behavior for the majority
> > of problematic use cases.  Rather than checking satisfaction before
> > _all_ non-dependent conversions, as specified by the CWG issue,
> > this patch makes us first check "safe" non-dependent conversions,
> > then satisfaction, then followed by "unsafe" non-dependent conversions.
> > In this case, a conversion is "safe" if computing it is guaranteed
> > to not induce template instantiation.  This patch heuristically
> > determines "safety" by checking for a constructor template or conversion
> > function template in the (class) parm or arg types respectively.
> > If neither type has such a member, then computing the conversion
> > should not induce instantiation (modulo satisfaction checking of
> > non-template constructor and conversion functions I suppose).
> > 
> > +	  /* We're checking only non-instantiating conversions.
> > +	     A conversion may instantiate only if it's to/from a
> > +	     class type that has a constructor template/conversion
> > +	     function template.  */
> > +	  tree parm_nonref = non_reference (parm);
> > +	  tree type_nonref = non_reference (type);
> > +
> > +	  if (CLASS_TYPE_P (parm_nonref))
> > +	    {
> > +	      if (!COMPLETE_TYPE_P (parm_nonref)
> > +		  && CLASSTYPE_TEMPLATE_INSTANTIATION (parm_nonref))
> > +		return unify_success (explain_p);
> > +
> > +	      tree ctors = get_class_binding (parm_nonref,
> > +					      complete_ctor_identifier);
> > +	      for (tree ctor : lkp_range (ctors))
> > +		if (TREE_CODE (ctor) == TEMPLATE_DECL)
> > +		  return unify_success (explain_p);
> 
> Today we discussed maybe checking CLASSTYPE_NON_AGGREGATE?

Done; all dups of this PR seem to use tag types that are aggregates, so this
seems like a good simplification.  I also made us punt if the arg type has a
constrained non-template conversion function.

> 
> Also, instantiation can also happen when checking for conversion to a pointer
> or reference to base class.

Oops, I suppose we just need to strip pointer types upfront as well.  The
!COMPLETE_TYPE_P && CLASSTYPE_TEMPLATE_INSTANTIATION tests will then make
sure we deem a potential derived-to-base conversion unsafe if appropriate
IIUC.

How does the following look?

-- >8 --

Subject: [PATCH] c++: refine CWG 2369 satisfaction vs non-dep convs [PR99599]

	PR c++/99599

gcc/cp/ChangeLog:

	* config-lang.in (gtfiles): Add search.cc.
	* pt.cc (check_non_deducible_conversions): Add bool parameter
	passed down to check_non_deducible_conversion.
	(fn_type_unification): Call check_non_deducible_conversions
	an extra time before satisfaction with noninst_only_p=true.
	(check_non_deducible_conversion): Add bool parameter controlling
	whether to compute only conversions that are guaranteed to
	not induce template instantiation.
	* search.cc (conversions_cache): Define.
	(lookup_conversions): Use it to cache the lookup.  Improve cache
	rate by considering TYPE_MAIN_VARIANT of the type.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-nondep4.C: New test.
---
 gcc/cp/config-lang.in                         |  1 +
 gcc/cp/pt.cc                                  | 81 +++++++++++++++++--
 gcc/cp/search.cc                              | 14 +++-
 gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C | 21 +++++
 4 files changed, 110 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C

diff --git a/gcc/cp/config-lang.in b/gcc/cp/config-lang.in
index a6c7883cc24..e34c392d208 100644
--- a/gcc/cp/config-lang.in
+++ b/gcc/cp/config-lang.in
@@ -52,6 +52,7 @@ gtfiles="\
 \$(srcdir)/cp/name-lookup.cc \
 \$(srcdir)/cp/parser.cc \$(srcdir)/cp/pt.cc \
 \$(srcdir)/cp/rtti.cc \
+\$(srcdir)/cp/search.cc \
 \$(srcdir)/cp/semantics.cc \
 \$(srcdir)/cp/tree.cc \$(srcdir)/cp/typeck2.cc \
 \$(srcdir)/cp/vtable-class-hierarchy.cc \
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index a4809f034dc..3c77d2466eb 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -151,7 +151,7 @@ static tree get_partial_spec_bindings (tree, tree, tree);
 static void tsubst_enum	(tree, tree, tree);
 static bool check_instantiated_args (tree, tree, tsubst_flags_t);
 static int check_non_deducible_conversion (tree, tree, unification_kind_t, int,
-					   struct conversion **, bool);
+					   struct conversion **, bool, bool);
 static int maybe_adjust_types_for_deduction (tree, unification_kind_t,
 					     tree*, tree*, tree);
 static int type_unification_real (tree, tree, tree, const tree *,
@@ -22315,7 +22315,8 @@ pack_deducible_p (tree parm, tree fn)
 static int
 check_non_deducible_conversions (tree parms, const tree *args, unsigned nargs,
 				 tree fn, unification_kind_t strict, int flags,
-				 struct conversion **convs, bool explain_p)
+				 struct conversion **convs, bool explain_p,
+				 bool noninst_only_p)
 {
   /* Non-constructor methods need to leave a conversion for 'this', which
      isn't included in nargs here.  */
@@ -22351,7 +22352,7 @@ check_non_deducible_conversions (tree parms, const tree *args, unsigned nargs,
 	  int lflags = conv_flags (ia, nargs, fn, arg, flags);
 
 	  if (check_non_deducible_conversion (parm, arg, strict, lflags,
-					      conv_p, explain_p))
+					      conv_p, explain_p, noninst_only_p))
 	    return 1;
 	}
 
@@ -22651,6 +22652,16 @@ fn_type_unification (tree fn,
 
  deduced:
 
+  /* As a refinement of CWG2369, check first and foremost non-dependent
+     conversions that we know are not going to induce template instantiation
+     (PR99599).  */
+  if (strict == DEDUCE_CALL
+      && incomplete
+      && check_non_deducible_conversions (parms, args, nargs, fn, strict, flags,
+					  convs, explain_p,
+					  /*noninst_only_p=*/true))
+    goto fail;
+
   /* CWG2369: Check satisfaction before non-deducible conversions.  */
   if (!constraints_satisfied_p (fn, targs))
     {
@@ -22664,7 +22675,8 @@ fn_type_unification (tree fn,
      as the standard says that we substitute explicit args immediately.  */
   if (incomplete
       && check_non_deducible_conversions (parms, args, nargs, fn, strict, flags,
-					  convs, explain_p))
+					  convs, explain_p,
+					  /*noninst_only_p=*/false))
     goto fail;
 
   /* All is well so far.  Now, check:
@@ -22899,7 +22911,7 @@ maybe_adjust_types_for_deduction (tree tparms,
 static int
 check_non_deducible_conversion (tree parm, tree arg, unification_kind_t strict,
 				int flags, struct conversion **conv_p,
-				bool explain_p)
+				bool explain_p, bool noninst_only_p)
 {
   tree type;
 
@@ -22921,6 +22933,65 @@ check_non_deducible_conversion (tree parm, tree arg, unification_kind_t strict,
     {
       bool ok = false;
       tree conv_arg = TYPE_P (arg) ? NULL_TREE : arg;
+      if (conv_p && *conv_p)
+	{
+	  /* This conversion was already computed earlier (when
+	     computing only non-instantiating conversions).  */
+	  gcc_checking_assert (!noninst_only_p);
+	  return unify_success (explain_p);
+	}
+      if (noninst_only_p)
+	{
+	  /* We're checking only non-instantiating conversions.
+	     Computing a conversion may induce template instantiation
+	     only if ... */
+	  tree parm_inner = non_reference (parm);
+	  tree type_inner = non_reference (type);
+	  bool ptr_conv_p = false;
+	  if (TYPE_PTR_P (parm_inner)
+	      && TYPE_PTR_P (type_inner))
+	    {
+	      parm_inner = TREE_TYPE (parm_inner);
+	      type_inner = TREE_TYPE (type_inner);
+	      ptr_conv_p = true;
+	    }
+
+	  if (CLASS_TYPE_P (parm_inner))
+	    {
+	      /* ... the parm's class type isn't yet instantiated, or ...  */
+	      if (!COMPLETE_TYPE_P (parm_inner)
+		  && CLASSTYPE_TEMPLATE_INSTANTIATION (parm_inner))
+		return unify_success (explain_p);
+
+	      /* constructors are considered and the parm type is a
+		 non-aggregate (and therefore has user-declared
+		 constructors), or ... */
+	      if (!ptr_conv_p && CLASSTYPE_NON_AGGREGATE (parm_inner))
+		return unify_success (explain_p);
+	    }
+
+	  if (CLASS_TYPE_P (type_inner))
+	    {
+	      /* ... the arg's class type isn't yet instantiated, or ... */
+	      if (!COMPLETE_TYPE_P (type_inner)
+		  && CLASSTYPE_TEMPLATE_INSTANTIATION (type_inner))
+		return unify_success (explain_p);
+
+	      /* ... conversion functions are considered and the arg's class
+		 type has one that is a template or is constrained.  */
+	      if (!ptr_conv_p)
+		{
+		  tree convs = lookup_conversions (type_inner);
+		  for (; convs; convs = TREE_CHAIN (convs))
+		    if (TREE_CODE (TREE_VALUE (convs)) == TEMPLATE_DECL
+			|| get_constraints (TREE_VALUE (convs)) != NULL_TREE)
+		      return unify_success (explain_p);
+		}
+	    }
+
+	  /* Otherwise, computing this conversion definitely won't induce
+	     template instantiation.  */
+	}
       if (conv_p)
 	/* Avoid recalculating this in add_function_candidate.  */
 	ok = (*conv_p
diff --git a/gcc/cp/search.cc b/gcc/cp/search.cc
index cd80f285ac9..931e083580a 100644
--- a/gcc/cp/search.cc
+++ b/gcc/cp/search.cc
@@ -2582,6 +2582,10 @@ lookup_conversions_r (tree binfo, int virtual_depth, int virtualness,
   return my_virtualness;
 }
 
+/* A cache of the result of lookup_conversions.  */
+
+static GTY((cache)) type_tree_cache_map *lookup_conversions_cache;
+
 /* Return a TREE_LIST containing all the non-hidden user-defined
    conversion functions for TYPE (and its base-classes).  The
    TREE_VALUE of each node is the FUNCTION_DECL of the conversion
@@ -2594,12 +2598,15 @@ lookup_conversions_r (tree binfo, int virtual_depth, int virtualness,
 tree
 lookup_conversions (tree type)
 {
-  tree convs;
-
+  type = TYPE_MAIN_VARIANT (type);
   complete_type (type);
   if (!CLASS_TYPE_P (type) || !TYPE_BINFO (type))
     return NULL_TREE;
 
+  if (tree *c = hash_map_safe_get (lookup_conversions_cache, type))
+    return *c;
+
+  tree convs;
   lookup_conversions_r (TYPE_BINFO (type), 0, 0, NULL_TREE, NULL_TREE, &convs);
 
   tree list = NULL_TREE;
@@ -2618,6 +2625,7 @@ lookup_conversions (tree type)
 	}
     }
 
+  hash_map_safe_put<hm_ggc> (lookup_conversions_cache, type, list);
   return list;
 }
 
@@ -2798,3 +2806,5 @@ any_dependent_bases_p (tree type)
 
   return false;
 }
+
+#include "gt-cp-search.h"
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C
new file mode 100644
index 00000000000..d26783524f7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C
@@ -0,0 +1,21 @@
+// PR c++/99599
+// { dg-do compile { target c++20 } }
+
+struct foo_tag { };
+struct bar_tag { };
+
+template <class T>
+concept fooable = requires(T it) {
+  invoke_tag(foo_tag{}, it);
+};
+
+template<class T>
+void invoke_tag(foo_tag, T in);
+
+template<fooable T>
+void invoke_tag(bar_tag, T it);
+
+int main() {
+  invoke_tag(foo_tag{}, 2);
+  invoke_tag(bar_tag{}, 2);
+}
-- 
2.42.0.rc2