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

[Jason Merrill <jason@redhat.com>]

On 9/6/23 18:09, Patrick Palka wrote:
> 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?

OK.

> -- >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.