[gcc r12-3346] c++: shortcut bad convs during overload resolution [PR101904]

[gcc r12-3346] c++: shortcut bad convs during overload resolution [PR101904]

[Patrick Palka]

https://gcc.gnu.org/g:47543e5f9d1fc502be79f91c87cbeb6eda17e641

commit r12-3346-g47543e5f9d1fc502be79f91c87cbeb6eda17e641
Author: Patrick Palka <ppalka@redhat.com>
Date:   Fri Sep 3 11:33:41 2021 -0400

    c++: shortcut bad convs during overload resolution [PR101904]
    
    In the context of overload resolution we have the notion of a "bad"
    argument conversion, which is a conversion that "would be a permitted
    with a bending of the language standards", and we handle such bad
    conversions specially.  In particular, we rank a bad conversion as
    better than no conversion but worse than a good conversion, and a bad
    conversion doesn't necessarily make a candidate unviable.  With the
    flag -fpermissive, we permit the situation where overload resolution
    selects a candidate that contains a bad conversion (which we call a
    non-strictly viable candidate).  And without the flag, the caller
    of overload resolution usually issues a distinct permerror in this
    situation instead.
    
    One consequence of this defacto behavior is that in order to distinguish
    a non-strictly viable candidate from an unviable candidate, if we
    encounter a bad argument conversion during overload resolution we must
    keep converting subsequent arguments because a subsequent conversion
    could render the candidate unviable instead of just non-strictly viable.
    But checking subsequent arguments can force template instantiations and
    result in otherwise avoidable hard errors.  And in particular, all
    'this' conversions are at worst bad, so this means the const/ref-qualifiers
    of a member function can't be used to prune a candidate quickly, which
    is the subject of the mentioned PR.
    
    This patch tries to improve the situation without changing the defacto
    output of add_candidates.  Specifically, when considering a candidate
    during overload resolution this patch makes us shortcut argument
    conversion checking upon encountering the first bad conversion
    (tentatively marking the candidate as non-strictly viable, though it
    could ultimately be unviable) under the assumption that we'll eventually
    find a strictly viable candidate anyway (which renders moot the
    distinction between non-strictly viable and unviable, since both are
    worse than a strictly viable candidate).  If this assumption turns out
    to be false, we'll fully reconsider the candidate under the defacto
    behavior (without the shortcutting) so that all its conversions are
    computed.
    
    So in the best case (there's a strictly viable candidate), we avoid
    some argument conversions and/or template argument deduction that may
    cause a hard error.  In the worst case (there's no such candidate), we
    have to redundantly consider some candidates twice.  (In a previous
    version of the patch, to avoid this redundant checking I created a new
    "deferred" conversion type that represents a conversion that is yet to
    be computed, and instead of reconsidering a candidate I just realized
    its deferred conversions.  But it doesn't seem this redundancy is a
    significant performance issue to justify the added complexity of this
    other approach.)
    
            PR c++/101904
    
    gcc/cp/ChangeLog:
    
            * call.c (build_this_conversion): New function, split out from
            add_function_candidate.
            (add_function_candidate): New parameter shortcut_bad_convs.
            Document it.  Use build_this_conversion.  Stop at the first bad
            argument conversion when shortcut_bad_convs is true.
            (add_template_candidate_real): New parameter shortcut_bad_convs.
            Use build_this_conversion to check the 'this' conversion before
            attempting deduction.  When the rejection reason code is
            rr_bad_arg_conversion, pass -1 instead of 0 as the viable
            parameter to add_candidate.  Pass 'convs' to add_candidate.
            (add_template_candidate): New parameter shortcut_bad_convs.
            (add_template_conv_candidate): Pass false as shortcut_bad_convs
            to add_template_candidate_real.
            (add_candidates): Prefer to shortcut bad conversions during
            overload resolution under the assumption that we'll eventually
            see a strictly viable candidate.  If this assumption turns out
            to be false, re-process the non-strictly viable candidates
            without shortcutting those bad conversions.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/template/conv17.C: New test.

Diff:
---
 gcc/cp/call.c                          | 250 +++++++++++++++++++++++----------
 gcc/testsuite/g++.dg/template/conv17.C |  56 ++++++++
 2 files changed, 233 insertions(+), 73 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 7bbf1342ef8..b6011c1a282 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -175,17 +175,17 @@ static struct z_candidate *splice_viable (struct z_candidate *, bool, bool *);
 static bool any_strictly_viable (struct z_candidate *);
 static struct z_candidate *add_template_candidate
 	(struct z_candidate **, tree, tree, tree, tree, const vec<tree, va_gc> *,
-	 tree, tree, tree, int, unification_kind_t, tsubst_flags_t);
+	 tree, tree, tree, int, unification_kind_t, bool, tsubst_flags_t);
 static struct z_candidate *add_template_candidate_real
 	(struct z_candidate **, tree, tree, tree, tree, const vec<tree, va_gc> *,
-	 tree, tree, tree, int, tree, unification_kind_t, tsubst_flags_t);
+	 tree, tree, tree, int, tree, unification_kind_t, bool, tsubst_flags_t);
 static bool is_complete (tree);
 static struct z_candidate *add_conv_candidate
 	(struct z_candidate **, tree, tree, const vec<tree, va_gc> *, tree,
 	 tree, tsubst_flags_t);
 static struct z_candidate *add_function_candidate
 	(struct z_candidate **, tree, tree, tree, const vec<tree, va_gc> *, tree,
-	 tree, int, conversion**, tsubst_flags_t);
+	 tree, int, conversion**, bool, tsubst_flags_t);
 static conversion *implicit_conversion (tree, tree, tree, bool, int,
 					tsubst_flags_t);
 static conversion *reference_binding (tree, tree, tree, bool, int,
@@ -2242,6 +2242,56 @@ conv_flags (int i, int nargs, tree fn, tree arg, int flags)
   return lflags;
 }
 
+/* Build an appropriate 'this' conversion for the method FN and class
+   type CTYPE from the value ARG (having type ARGTYPE) to the type PARMTYPE.
+   This function modifies PARMTYPE, ARGTYPE and ARG.  */
+
+static conversion *
+build_this_conversion (tree fn, tree ctype,
+		       tree& parmtype, tree& argtype, tree& arg,
+		       int flags, tsubst_flags_t complain)
+{
+  gcc_assert (DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
+	      && !DECL_CONSTRUCTOR_P (fn));
+
+  /* The type of the implicit object parameter ('this') for
+     overload resolution is not always the same as for the
+     function itself; conversion functions are considered to
+     be members of the class being converted, and functions
+     introduced by a using-declaration are considered to be
+     members of the class that uses them.
+
+     Since build_over_call ignores the ICS for the `this'
+     parameter, we can just change the parm type.  */
+  parmtype = cp_build_qualified_type (ctype,
+				      cp_type_quals (TREE_TYPE (parmtype)));
+  bool this_p = true;
+  if (FUNCTION_REF_QUALIFIED (TREE_TYPE (fn)))
+    {
+      /* If the function has a ref-qualifier, the implicit
+	 object parameter has reference type.  */
+      bool rv = FUNCTION_RVALUE_QUALIFIED (TREE_TYPE (fn));
+      parmtype = cp_build_reference_type (parmtype, rv);
+      /* The special handling of 'this' conversions in compare_ics
+	 does not apply if there is a ref-qualifier.  */
+      this_p = false;
+    }
+  else
+    {
+      parmtype = build_pointer_type (parmtype);
+      /* We don't use build_this here because we don't want to
+	 capture the object argument until we've chosen a
+	 non-static member function.  */
+      arg = build_address (arg);
+      argtype = lvalue_type (arg);
+    }
+  flags |= LOOKUP_ONLYCONVERTING;
+  conversion *t = implicit_conversion (parmtype, argtype, arg,
+				       /*c_cast_p=*/false, flags, complain);
+  t->this_p = this_p;
+  return t;
+}
+
 /* Create an overload candidate for the function or method FN called
    with the argument list FIRST_ARG/ARGS and add it to CANDIDATES.
    FLAGS is passed on to implicit_conversion.
@@ -2249,7 +2299,14 @@ conv_flags (int i, int nargs, tree fn, tree arg, int flags)
    This does not change ARGS.
 
    CTYPE, if non-NULL, is the type we want to pretend this function
-   comes from for purposes of overload resolution.  */
+   comes from for purposes of overload resolution.
+
+   SHORTCUT_BAD_CONVS controls how we handle "bad" argument conversions.
+   If true, we stop computing conversions upon seeing the first bad
+   conversion.  This is used by add_candidates to avoid computing
+   more conversions than necessary in the presence of a strictly viable
+   candidate, while preserving the defacto behavior of overload resolution
+   when it turns out there are only non-strictly viable candidates.  */
 
 static struct z_candidate *
 add_function_candidate (struct z_candidate **candidates,
@@ -2257,6 +2314,7 @@ add_function_candidate (struct z_candidate **candidates,
 			const vec<tree, va_gc> *args, tree access_path,
 			tree conversion_path, int flags,
 			conversion **convs,
+			bool shortcut_bad_convs,
 			tsubst_flags_t complain)
 {
   tree parmlist = TYPE_ARG_TYPES (TREE_TYPE (fn));
@@ -2378,8 +2436,6 @@ add_function_candidate (struct z_candidate **candidates,
     {
       tree argtype, to_type;
       tree arg;
-      conversion *t;
-      int is_this;
 
       if (parmnode == void_list_node)
 	break;
@@ -2398,54 +2454,23 @@ add_function_candidate (struct z_candidate **candidates,
 		(*args)[i + skip - (first_arg != NULL_TREE ? 1 : 0)]);
       argtype = lvalue_type (arg);
 
-      is_this = (i == 0 && DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
-		 && ! DECL_CONSTRUCTOR_P (fn));
-
+      conversion *t;
       if (parmnode)
 	{
 	  tree parmtype = TREE_VALUE (parmnode);
-
-	  parmnode = TREE_CHAIN (parmnode);
-
-	  /* The type of the implicit object parameter ('this') for
-	     overload resolution is not always the same as for the
-	     function itself; conversion functions are considered to
-	     be members of the class being converted, and functions
-	     introduced by a using-declaration are considered to be
-	     members of the class that uses them.
-
-	     Since build_over_call ignores the ICS for the `this'
-	     parameter, we can just change the parm type.  */
-	  if (ctype && is_this)
+	  if (i == 0
+	      && DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
+	      && !DECL_CONSTRUCTOR_P (fn))
+	    t = build_this_conversion (fn, ctype, parmtype, argtype, arg,
+				       flags, complain);
+	  else
 	    {
-	      parmtype = cp_build_qualified_type
-		(ctype, cp_type_quals (TREE_TYPE (parmtype)));
-	      if (FUNCTION_REF_QUALIFIED (TREE_TYPE (fn)))
-		{
-		  /* If the function has a ref-qualifier, the implicit
-		     object parameter has reference type.  */
-		  bool rv = FUNCTION_RVALUE_QUALIFIED (TREE_TYPE (fn));
-		  parmtype = cp_build_reference_type (parmtype, rv);
-		  /* The special handling of 'this' conversions in compare_ics
-		     does not apply if there is a ref-qualifier.  */
-		  is_this = false;
-		}
-	      else
-		{
-		  parmtype = build_pointer_type (parmtype);
-		  /* We don't use build_this here because we don't want to
-		     capture the object argument until we've chosen a
-		     non-static member function.  */
-		  arg = build_address (arg);
-		  argtype = lvalue_type (arg);
-		}
+	      int lflags = conv_flags (i, len-skip, fn, arg, flags);
+	      t = implicit_conversion (parmtype, argtype, arg,
+				       /*c_cast_p=*/false, lflags, complain);
 	    }
-
-	  int lflags = conv_flags (i, len-skip, fn, arg, flags);
-
-	  t = implicit_conversion (parmtype, argtype, arg,
-				   /*c_cast_p=*/false, lflags, complain);
 	  to_type = parmtype;
+	  parmnode = TREE_CHAIN (parmnode);
 	}
       else
 	{
@@ -2454,9 +2479,6 @@ add_function_candidate (struct z_candidate **candidates,
 	  to_type = argtype;
 	}
 
-      if (t && is_this)
-	t->this_p = true;
-
       convs[i] = t;
       if (! t)
 	{
@@ -2471,7 +2493,8 @@ add_function_candidate (struct z_candidate **candidates,
 	  viable = -1;
 	  reason = bad_arg_conversion_rejection (first_arg, i, arg, to_type,
 						 EXPR_LOCATION (arg));
-
+	  if (shortcut_bad_convs)
+	    break;
 	}
     }
 
@@ -3355,7 +3378,9 @@ add_builtin_candidates (struct z_candidate **candidates, enum tree_code code,
    This does not change ARGLIST.  The RETURN_TYPE is the desired type
    for conversion operators.  If OBJ is NULL_TREE, FLAGS and CTYPE are
    as for add_function_candidate.  If an OBJ is supplied, FLAGS and
-   CTYPE are ignored, and OBJ is as for add_conv_candidate.  */
+   CTYPE are ignored, and OBJ is as for add_conv_candidate.
+
+   SHORTCUT_BAD_CONVS is as in add_function_candidate.  */
 
 static struct z_candidate*
 add_template_candidate_real (struct z_candidate **candidates, tree tmpl,
@@ -3363,7 +3388,7 @@ add_template_candidate_real (struct z_candidate **candidates, tree tmpl,
 			     const vec<tree, va_gc> *arglist, tree return_type,
 			     tree access_path, tree conversion_path,
 			     int flags, tree obj, unification_kind_t strict,
-			     tsubst_flags_t complain)
+			     bool shortcut_bad_convs, tsubst_flags_t complain)
 {
   int ntparms = DECL_NTPARMS (tmpl);
   tree targs = make_tree_vec (ntparms);
@@ -3493,7 +3518,33 @@ add_template_candidate_real (struct z_candidate **candidates, tree tmpl,
 
   errs = errorcount+sorrycount;
   if (!obj)
-    convs = alloc_conversions (nargs);
+    {
+      convs = alloc_conversions (nargs);
+
+      if (shortcut_bad_convs
+	  && DECL_NONSTATIC_MEMBER_FUNCTION_P (tmpl)
+	  && !DECL_CONSTRUCTOR_P (tmpl))
+	{
+	  /* Check the 'this' conversion before proceeding with deduction.
+	     This is effectively an extension of the DR 1391 resolution
+	     that we perform in check_non_deducible_conversions, though it's
+	     convenient to do this extra check here instead of there.  */
+	  tree parmtype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (tmpl)));
+	  tree argtype = lvalue_type (first_arg);
+	  tree arg = first_arg;
+	  conversion *t = build_this_conversion (tmpl, ctype,
+						 parmtype, argtype, arg,
+						 flags, complain);
+	  convs[0] = t;
+	  if (t->bad_p)
+	    {
+	      reason = bad_arg_conversion_rejection (first_arg, 0,
+						     arg, parmtype,
+						     EXPR_LOCATION (arg));
+	      goto fail;
+	    }
+	}
+    }
   fn = fn_type_unification (tmpl, explicit_targs, targs,
 			    args_without_in_chrg,
 			    nargs_without_in_chrg,
@@ -3540,7 +3591,8 @@ add_template_candidate_real (struct z_candidate **candidates, tree tmpl,
   else
     cand = add_function_candidate (candidates, fn, ctype,
 				   first_arg, arglist, access_path,
-				   conversion_path, flags, convs, complain);
+				   conversion_path, flags, convs,
+				   shortcut_bad_convs, complain);
   if (DECL_TI_TEMPLATE (fn) != tmpl)
     /* This situation can occur if a member template of a template
        class is specialized.  Then, instantiate_template might return
@@ -3566,8 +3618,9 @@ add_template_candidate_real (struct z_candidate **candidates, tree tmpl,
 
   return cand;
  fail:
-  return add_candidate (candidates, tmpl, first_arg, arglist, nargs, NULL,
-			access_path, conversion_path, 0, reason, flags);
+  int viable = (reason->code == rr_bad_arg_conversion ? -1 : 0);
+  return add_candidate (candidates, tmpl, first_arg, arglist, nargs, convs,
+			access_path, conversion_path, viable, reason, flags);
 }
 
 
@@ -3576,13 +3629,15 @@ add_template_candidate (struct z_candidate **candidates, tree tmpl, tree ctype,
 			tree explicit_targs, tree first_arg,
 			const vec<tree, va_gc> *arglist, tree return_type,
 			tree access_path, tree conversion_path, int flags,
-			unification_kind_t strict, tsubst_flags_t complain)
+			unification_kind_t strict, bool shortcut_bad_convs,
+			tsubst_flags_t complain)
 {
   return
     add_template_candidate_real (candidates, tmpl, ctype,
 				 explicit_targs, first_arg, arglist,
 				 return_type, access_path, conversion_path,
-				 flags, NULL_TREE, strict, complain);
+				 flags, NULL_TREE, strict, shortcut_bad_convs,
+				 complain);
 }
 
 /* Create an overload candidate for the conversion function template TMPL,
@@ -3608,7 +3663,7 @@ add_template_conv_candidate (struct z_candidate **candidates, tree tmpl,
     add_template_candidate_real (candidates, tmpl, NULL_TREE, NULL_TREE,
 				 NULL_TREE, arglist, return_type, access_path,
 				 conversion_path, 0, obj, DEDUCE_CALL,
-				 complain);
+				 /*shortcut_bad_convs=*/false, complain);
 }
 
 /* The CANDS are the set of candidates that were considered for
@@ -6060,6 +6115,7 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
     /* Delay creating the implicit this parameter until it is needed.  */
     non_static_args = NULL;
 
+  bool seen_strictly_viable = any_strictly_viable (*candidates);
   /* If there's a non-template perfect match, we don't need to consider
      templates.  So check non-templates first.  This optimization is only
      really needed for the defaulted copy constructor of tuple and the like
@@ -6071,6 +6127,19 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
   else /*if (flags & LOOKUP_DEFAULTED)*/
     which = non_templates;
 
+  /* During overload resolution, we first consider each function under the
+     assumption that we'll eventually find a strictly viable candidate.
+     This allows us to circumvent our defacto behavior when checking
+     argument conversions and shortcut consideration of the candidate
+     upon encountering the first bad conversion.  If this assumption
+     turns out to be false, and all candidates end up being non-strictly
+     viable, then we reconsider such candidates under the defacto behavior.
+     This trick is important for pruning member function overloads according
+     to their const/ref-qualifiers (since all 'this' conversions are at
+     worst bad) without breaking -fpermissive.  */
+  tree bad_fns = NULL_TREE;
+  bool shortcut_bad_convs = true;
+
  again:
   for (tree fn : lkp_range (fns))
     {
@@ -6117,18 +6186,22 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
 	}
 
       if (TREE_CODE (fn) == TEMPLATE_DECL)
-	add_template_candidate (candidates,
-				fn,
-				ctype,
-				explicit_targs,
-				fn_first_arg,
-				fn_args,
-				return_type,
-				access_path,
-				conversion_path,
-				flags,
-				strict,
-				complain);
+	{
+	  if (!add_template_candidate (candidates,
+				       fn,
+				       ctype,
+				       explicit_targs,
+				       fn_first_arg,
+				       fn_args,
+				       return_type,
+				       access_path,
+				       conversion_path,
+				       flags,
+				       strict,
+				       shortcut_bad_convs,
+				       complain))
+	    continue;
+	}
       else
 	{
 	  add_function_candidate (candidates,
@@ -6140,16 +6213,47 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
 				  conversion_path,
 				  flags,
 				  NULL,
+				  shortcut_bad_convs,
 				  complain);
 	  if (perfect_candidate_p (*candidates))
 	    seen_perfect = true;
 	}
+
+      z_candidate *cand = *candidates;
+      if (cand->viable == 1)
+	seen_strictly_viable = true;
+
+      if (cand->viable == -1
+	  && shortcut_bad_convs
+	  && !cand->convs[cand->reversed () ? 0 : cand->num_convs - 1])
+	{
+	  /* This candidate has been tentatively marked non-strictly viable,
+	     and we didn't compute all argument conversions for it (having
+	     stopped at the first bad conversion).  Add the function to BAD_FNS
+	     to fully reconsider later if we don't find any strictly viable
+	     candidates.  */
+	  bad_fns = lookup_add (fn, bad_fns);
+	  *candidates = (*candidates)->next;
+	}
     }
   if (which == non_templates && !seen_perfect)
     {
       which = templates;
       goto again;
     }
+  else if (which == templates
+	   && !seen_strictly_viable
+	   && shortcut_bad_convs
+	   && bad_fns)
+    {
+      /* None of the candidates are strictly viable, so consider again those
+	 functions in BAD_FNS, this time without shortcutting bad conversions
+	 so that all their argument conversions are computed.  */
+      which = either;
+      fns = bad_fns;
+      shortcut_bad_convs = false;
+      goto again;
+    }
 }
 
 /* Returns 1 if P0145R2 says that the LHS of operator CODE is evaluated first,
diff --git a/gcc/testsuite/g++.dg/template/conv17.C b/gcc/testsuite/g++.dg/template/conv17.C
new file mode 100644
index 00000000000..ba012c9d1fa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/conv17.C
@@ -0,0 +1,56 @@
+// PR c++/101904
+// Verify we stop at the first bad argument conversion when considering a
+// candidate during overload resolution.
+
+template<class T>
+struct A { typedef typename T::type type; };
+
+struct B {
+  // A conversion function that always induces a hard error when instantiated.
+  template<class T> B(T, typename A<T>::type = 0);
+};
+
+struct C {
+  template<class T> void f(T, typename A<T>::type); // #1
+  template<class T> void f(T, T) const;             // #2
+
+  static void g(int*, B);                           // #3
+  static void g(int, int);                          // #4
+
+#if __cpp_ref_qualifiers
+  void h(B) &;                                      // #5
+  void h(int) &&;                                   // #6
+#endif
+};
+
+int main() {
+  const C c;
+
+  // The bad conversion for the 'this' argument should preclude us from further
+  // considering the non-const #1 (which would have caused a hard error during
+  // instantiation).  This behavior is essentially DR 1391 extended to the
+  // 'this' argument.
+  c.f(0, 0); // resolves to #2
+  c.f<int>(0, 0);
+
+  // Likewise for the bad conversion for the 1st argument in #3.
+  C::g(42, 42); // resolves to #4
+
+#if __cpp_ref_qualifiers
+  // Likewise for the bad 'this' conversion in #5.
+  C().h(0); // resolves to #6
+#endif
+}
+
+#if __cpp_concepts
+// Test the same calls in a SFINAE context.
+template<class T>
+concept D = requires (const T t) {
+  t.f(0, 0);
+  t.template f<int>(0, 0);
+  T::g(42, 42);
+  T().h(0);
+};
+
+static_assert(D<C>);
+#endif