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

[Jason Merrill <jason@redhat.com>]

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