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

[Patrick Palka <ppalka@redhat.com>]

On Mon, 28 Aug 2023, Jason Merrill wrote:

> On 8/24/23 09:31, Patrick Palka wrote:
> > 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
> > 
> > @@ -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 ... */
> 
> Let's factor this whole block out into another function.

Sounds good.

> 
> Incidentally, CWG1092 is a related problem with defaulted functions, which I
> dealt with in a stricter way: when LOOKUP_DEFAULTED we ignore a conversion
> from the parameter being copied to a non-reference-related type.  As a
> follow-on, it might make sense to use this test there as well?

Interesting, I can look into that.

> 
> > +	  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;
> > +	    }
> 
> I think we also want to set ptr_conv_p if the types are reference_related_p?

Ah, because in that case we know the selected conversion will always be
a derived-to-base conversion?  That sounds like a nice refinement.

> 
> > +	      /* ... conversion functions are considered and the arg's class
> > +		 type has one that is a template or is constrained.  */
> 
> Maybe just check TYPE_HAS_CONVERSION without digging into the actual
> conversions, like with CLASSTYPE_NON_AGGREGATE?
> 

Sounds good, I split out the conversion function caching into a separate
patch.

Like so?

-- >8 --

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

	PR c++/99599

gcc/cp/ChangeLog:

	* 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.
	(conversion_may_instantiate_p): Define.
	(check_non_deducible_conversion): Add bool parameter controlling
	whether to compute only conversions that are guaranteed to
	not induce template instantiation.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-nondep4.C: New test.
---
 gcc/cp/pt.cc                                  | 86 +++++++++++++++++--
 gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C | 21 +++++
 2 files changed, 102 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-nondep4.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 0790bbf948f..b528ae9a0b8 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 *,
@@ -22321,7 +22321,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.  */
@@ -22357,7 +22358,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;
 	}
 
@@ -22657,6 +22658,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))
     {
@@ -22670,7 +22681,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:
@@ -22897,6 +22909,60 @@ maybe_adjust_types_for_deduction (tree tparms,
   return result;
 }
 
+/* Return true if computing a conversion from FROM to TO might
+   induce template instantiation.  Conversely, if this predicate
+   returns false then computing the conversion definitely won't
+   induce template instantiation.  */
+
+static bool
+conversion_may_instantiate_p (tree to, tree from)
+{
+  to = non_reference (to);
+  from = non_reference (from);
+
+  bool ptr_conv_p = false;
+  if (TYPE_PTR_P (to)
+      && TYPE_PTR_P (from))
+    {
+      to = TREE_TYPE (to);
+      from = TREE_TYPE (from);
+      ptr_conv_p = true;
+    }
+
+  /* If one of the types is a not-yet-instantiated class template
+     specialization, then computing a conversion will definitely
+     perform instantiation in order to consider bases, converting
+     constructors and/or conversion functions.  */
+  if ((CLASS_TYPE_P (to)
+       && !COMPLETE_TYPE_P (to)
+       && CLASSTYPE_TEMPLATE_INSTANTIATION (to))
+      || (CLASS_TYPE_P (from)
+	  && !COMPLETE_TYPE_P (from)
+	  && CLASSTYPE_TEMPLATE_INSTANTIATION (from)))
+    return true;
+
+  /* Converting from one pointer type to another, or between
+     reference-related types, always yields a standard conversion.  */
+  if (ptr_conv_p || reference_related_p (to, from))
+    return false;
+
+  /* Converting to a non-aggregate class type will consider its
+     user-declared constructors, one of which might be a template.  */
+  if (CLASS_TYPE_P (to)
+      && CLASSTYPE_NON_AGGREGATE (to))
+    return true;
+
+  /* Converting from a class type will consider its conversion functions,
+     one of which might be a template (or constrained).  */
+  if (CLASS_TYPE_P (from)
+      && TYPE_HAS_CONVERSION (from))
+    return true;
+
+  /* Otherwise, computing this conversion definitely won't induce
+     template instantiation.  */
+  return false;
+}
+
 /* Subroutine of fn_type_unification.  PARM is a function parameter of a
    template which doesn't contain any deducible template parameters; check if
    ARG is a suitable match for it.  STRICT, FLAGS and EXPLAIN_P are as in
@@ -22905,7 +22971,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;
 
@@ -22927,6 +22993,16 @@ 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
+	  && conversion_may_instantiate_p (parm, type))
+	return unify_success (explain_p);
       if (conv_p)
 	/* Avoid recalculating this in add_function_candidate.  */
 	ok = (*conv_p
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.122.g1fc548b2d6