Re: [PATCH] c++: shortcut bad reference bindings [PR94894]

[Jason Merrill <jason@redhat.com>]

On 7/18/22 12:59, Patrick Palka wrote:
> In case of l/rvalue or cv-qual mismatch during reference binding, we try
> to give more helpful diagnostics by attempting a bad conversion that
> ignores the mismatch.  But in doing so, we may end up instantiating an
> ill-formed conversion function, something that would otherwise be
> avoided if we didn't try for the better diagnostic in the first place.
> We could just give up on producing a good diagnostic in this case, but
> ideally we could keep the good diagnostics while avoiding unnecessary
> template instantiations.
> 
> To that end, this patch adapts the bad conversion shortcutting mechanism
> from r12-3346-g47543e5f9d1fc5 to handle this situation as well.  The main
> observation from there applies here as well: when there's a strictly
> viable candidate, we don't care about the distinction between an unviable
> and non-strictly viable candidate.  And in turn it also means we don't
> really care about the distinction between an invalid and bad conversion.
> Of course, we don't know whether there's a strictly viable candidate until
> after the fact, so we still need to keep track of when we "shortcutted"
> distinguishing between an invalid and bad conversion.  This patch adds a
> new kind of conversion, ck_shortcutted, to represent such conversions.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> 	PR c++/94894
> 	PR c++/105766
> 	PR c++/106201
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (enum conversion_kind): Add ck_shortcutted enumerator.
> 	(has_next): Return false for it.
> 	(reference_binding): Return a ck_shortcutted conversion instead
> 	of an actual bad conversion when LOOKUP_SHORTCUT_BAD_CONVS is set.
> 	Remove now obsolete early exit for the incomplete target type case.
> 	(implicit_conversion_1): Don't mask out LOOKUP_SHORTCUT_BAD_CONVS.
> 	(add_function_candidate): Set LOOKUP_SHORTCUT_BAD_CONVS iff
> 	shortcut_bad_convs.
> 	(missing_conversion_p): Return true for a ck_shortcutted
> 	conversion.
> 	* cp-tree.h (LOOKUP_SHORTCUT_BAD_CONVS): Define.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/conversion/ref8.C: New test.
> 	* g++.dg/conversion/ref9.C: New test.
> ---
>   gcc/cp/call.cc                         | 87 ++++++++++++++++----------
>   gcc/cp/cp-tree.h                       |  5 ++
>   gcc/testsuite/g++.dg/conversion/ref8.C | 22 +++++++
>   gcc/testsuite/g++.dg/conversion/ref9.C | 21 +++++++
>   4 files changed, 103 insertions(+), 32 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/conversion/ref8.C
>   create mode 100644 gcc/testsuite/g++.dg/conversion/ref9.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index fc98552fda2..ca2f5fbca39 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -59,7 +59,8 @@ enum conversion_kind {
>     ck_ambig,
>     ck_list,
>     ck_aggr,
> -  ck_rvalue
> +  ck_rvalue,
> +  ck_shortcutted,

Please give this a comment explaining how it's used.  OK with that change.

Maybe ck_deferred_bad?

>   };
>   
>   /* The rank of the conversion.  Order of the enumerals matters; better
> @@ -775,7 +776,8 @@ has_next (conversion_kind code)
>     return !(code == ck_identity
>   	   || code == ck_ambig
>   	   || code == ck_list
> -	   || code == ck_aggr);
> +	   || code == ck_aggr
> +	   || code == ck_shortcutted);
>   }
>   
>   static conversion *
> @@ -1912,18 +1914,38 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags,
>        difference in top-level cv-qualification is subsumed by the
>        initialization itself and does not constitute a conversion.  */
>   
> +  bool maybe_valid_p = true;
> +
>     /* [dcl.init.ref]
>   
>        Otherwise, the reference shall be an lvalue reference to a
>        non-volatile const type, or the reference shall be an rvalue
> -     reference.
> +     reference.  */
> +  if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto))
> +    maybe_valid_p = false;
>   
> -     We try below to treat this as a bad conversion to improve diagnostics,
> -     but if TO is an incomplete class, we need to reject this conversion
> -     now to avoid unnecessary instantiation.  */
> -  if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto)
> -      && !COMPLETE_TYPE_P (to))
> -    return NULL;
> +  /* [dcl.init.ref]
> +
> +     Otherwise, a temporary of type "cv1 T1" is created and
> +     initialized from the initializer expression using the rules for a
> +     non-reference copy initialization.  If T1 is reference-related to
> +     T2, cv1 must be the same cv-qualification as, or greater
> +     cv-qualification than, cv2; otherwise, the program is ill-formed.  */
> +  if (related_p && !at_least_as_qualified_p (to, from))
> +    maybe_valid_p = false;
> +
> +  /* We try below to treat an invalid reference binding as a bad conversion
> +     to improve diagnostics, but doing so may cause otherwise unnecessary
> +     instantiations that can lead to hard error.  So during the first pass
> +     of overload resolution wherein we shortcut bad conversions, instead just
> +     produce a special conversion that we'll use to indicate we might need to
> +     perform a second pass if there's no strictly viable candidate.  */
> +  if (!maybe_valid_p && (flags & LOOKUP_SHORTCUT_BAD_CONVS))
> +    {
> +      conv = alloc_conversion (ck_shortcutted);
> +      conv->bad_p = true;
> +      return conv;
> +    }
>   
>     /* We're generating a temporary now, but don't bind any more in the
>        conversion (specifically, don't slice the temporary returned by a
> @@ -1967,7 +1989,9 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags,
>   				   sflags, complain);
>   	    if (!new_second)
>   	      return NULL;
> -	    return merge_conversion_sequences (t, new_second);
> +	    conv = merge_conversion_sequences (t, new_second);
> +	    gcc_assert (maybe_valid_p || conv->bad_p);
> +	    return conv;
>   	  }
>       }
>   
> @@ -1976,24 +2000,7 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags,
>        creation of a temporary.  */
>     conv->need_temporary_p = true;
>     conv->rvaluedness_matches_p = TYPE_REF_IS_RVALUE (rto);
> -
> -  /* [dcl.init.ref]
> -
> -     Otherwise, the reference shall be an lvalue reference to a
> -     non-volatile const type, or the reference shall be an rvalue
> -     reference.  */
> -  if (!CP_TYPE_CONST_NON_VOLATILE_P (to) && !TYPE_REF_IS_RVALUE (rto))
> -    conv->bad_p = true;
> -
> -  /* [dcl.init.ref]
> -
> -     Otherwise, a temporary of type "cv1 T1" is created and
> -     initialized from the initializer expression using the rules for a
> -     non-reference copy initialization.  If T1 is reference-related to
> -     T2, cv1 must be the same cv-qualification as, or greater
> -     cv-qualification than, cv2; otherwise, the program is ill-formed.  */
> -  if (related_p && !at_least_as_qualified_p (to, from))
> -    conv->bad_p = true;
> +  conv->bad_p |= !maybe_valid_p;
>   
>     return conv;
>   }
> @@ -2015,7 +2022,8 @@ implicit_conversion_1 (tree to, tree from, tree expr, bool c_cast_p,
>        resolution, or after we've chosen one.  */
>     flags &= (LOOKUP_ONLYCONVERTING|LOOKUP_NO_CONVERSION|LOOKUP_COPY_PARM
>   	    |LOOKUP_NO_TEMP_BIND|LOOKUP_NO_RVAL_BIND|LOOKUP_PREFER_RVALUE
> -	    |LOOKUP_NO_NARROWING|LOOKUP_PROTECT|LOOKUP_NO_NON_INTEGRAL);
> +	    |LOOKUP_NO_NARROWING|LOOKUP_PROTECT|LOOKUP_NO_NON_INTEGRAL
> +	    |LOOKUP_SHORTCUT_BAD_CONVS);
>   
>     /* FIXME: actually we don't want warnings either, but we can't just
>        have 'complain &= ~(tf_warning|tf_error)' because it would cause
> @@ -2433,6 +2441,11 @@ add_function_candidate (struct z_candidate **candidates,
>     if (! viable)
>       goto out;
>   
> +  if (shortcut_bad_convs)
> +    flags |= LOOKUP_SHORTCUT_BAD_CONVS;
> +  else
> +    flags &= ~LOOKUP_SHORTCUT_BAD_CONVS;
> +
>     /* Third, for F to be a viable function, there shall exist for each
>        argument an implicit conversion sequence that converts that argument
>        to the corresponding parameter of F.  */
> @@ -6038,14 +6051,24 @@ perfect_candidate_p (z_candidate *cand)
>     return true;
>   }
>   
> -/* True iff one of CAND's argument conversions is NULL.  */
> +/* True iff one of CAND's argument conversions is missing.  */
>   
>   static bool
>   missing_conversion_p (const z_candidate *cand)
>   {
>     for (unsigned i = 0; i < cand->num_convs; ++i)
> -    if (!cand->convs[i])
> -      return true;
> +    {
> +      conversion *conv = cand->convs[i];
> +      if (!conv)
> +	return true;
> +      if (conv->kind == ck_shortcutted)
> +	{
> +	  /* We don't know whether this conversion is outright invalid or
> +	     just bad, so be conservative.  */
> +	  gcc_checking_assert (conv->bad_p);
> +	  return true;
> +	}
> +    }
>     return false;
>   }
>   
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index bec98aa2ac3..f3b7da2549c 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5874,6 +5874,11 @@ enum overload_flags { NO_SPECIAL = 0, DTOR_FLAG, TYPENAME_FLAG };
>   #define LOOKUP_REVERSED (LOOKUP_REWRITTEN << 1)
>   /* We're initializing an aggregate from a parenthesized list of values.  */
>   #define LOOKUP_AGGREGATE_PAREN_INIT (LOOKUP_REVERSED << 1)
> +/* We're computing conversions as part of a first pass of overload resolution
> +   wherein we don't try to distinguish an unviable candidate from a
> +   non-strictly viable candidate and thus can avoid computing unnecessary
> +   conversions.  */
> +#define LOOKUP_SHORTCUT_BAD_CONVS (LOOKUP_AGGREGATE_PAREN_INIT << 1)
>   
>   /* These flags are used by the conversion code.
>      CONV_IMPLICIT   :  Perform implicit conversions (standard and user-defined).
> diff --git a/gcc/testsuite/g++.dg/conversion/ref8.C b/gcc/testsuite/g++.dg/conversion/ref8.C
> new file mode 100644
> index 00000000000..e27f642690c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/conversion/ref8.C
> @@ -0,0 +1,22 @@
> +// PR c++/105766
> +// { dg-do compile { target c++20 } }
> +
> +template<class T>
> +struct baz {
> +  baz() = default;
> +  baz(int) requires __is_constructible(T, int);
> +};
> +
> +struct foo;
> +
> +struct bar {
> +  bar() = default;
> +  bar(foo&);
> +  bar(int);
> +};
> +
> +struct foo {
> +  baz<bar> m_bars;
> +};
> +
> +foo a;
> diff --git a/gcc/testsuite/g++.dg/conversion/ref9.C b/gcc/testsuite/g++.dg/conversion/ref9.C
> new file mode 100644
> index 00000000000..e6dfc03cd7f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/conversion/ref9.C
> @@ -0,0 +1,21 @@
> +// PR c++/106201
> +// { dg-do compile { target c++11 } }
> +
> +struct A {
> +  template<class T, class = decltype(f(*(T*)nullptr))>
> +  A(const T&);
> +};
> +
> +struct B {
> +  template<class T> B(const T&);
> +};
> +
> +void f(A&);
> +void f(B);
> +
> +struct C { };
> +
> +int main() {
> +  C c;
> +  f(c);
> +}