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.
> 
> 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?
> 
> > +	  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?
> 
> > +	      /* ... 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?
> 
> Jason
> 
>