Re: [PATCH] c++: template-id ADL and partial instantiation [PR99911]

[Patrick Palka <ppalka@redhat.com>]

On Thu, 18 Nov 2021, Jason Merrill wrote:

> On 11/10/21 11:53, Patrick Palka wrote:
> > Here when partially instantiating the call get<U>(T{}) with T=N::A
> > (for which earlier unqualified name lookup for 'get' found nothing)
> > the arguments after substitution are no longer dependent but the callee
> > still is, so perform_koenig_lookup postpones ADL.  But then we go on to
> > diagnose the unresolved template name anyway, as if ADL was already
> > performed and failed.
> > 
> > This patch fixes this by avoiding the error path in question when the
> > template arguments of an unresolved template-id are dependent, which
> > mirrors the dependence check in perform_koenig_lookup.
> 
> This change is OK.
> 
> > In passing, this
> > patch also disables the -fpermissive fallback that performs a second
> > unqualified lookup in the template-id ADL case; this fallback seems to be
> > intended for legacy code and shouldn't be used for C++20 template-id ADL.
> 
> Why wouldn't we want the more helpful diagnostic?

The "no declarations were found by ADL" diagnostic is helpful, but the
backwards compatibility logic doesn't correctly handle the template-id
case.  E.g. for

  template<class T>
  void f() {
    g<int>(T{});
  }

  template<class T>
  void g(int); // #1

  int main() {
    f<int>();
  }

we get the helpful diagnostic followed by a confusing one because we
didn't incorporate the template-id's template arguments when replacing
the callee with the later-declared template #1:

<stdin>: In instantiation of ‘void f() [with T = int]’:
<stdin>:10:9:   required from here
<stdin>:3:6: error: ‘g’ was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
<stdin>:7:6: note: ‘template<class T> void g(int)’ declared here, later in the translation unit
<stdin>:3:6: error: no matching function for call to ‘g(int)’
<stdin>:7:6: note: candidate: ‘template<class T> void g(int)’
<stdin>:7:6: note:   template argument deduction/substitution failed:
<stdin>:3:6: note:   couldn’t deduce template parameter ‘T’


We also ignores template-ness of the name being looked up, so e.g. for:

  template<class T>
  void f() {
    g<>(T{});
  }

  void g(int); // #1

  int main() {
    f<int>();
  }

the secondary unqualified lookup finds the later-declared non-template #1:

<stdin>: In instantiation of ‘void f() [with T = int]’:
<stdin>:9:9:   required from here
<stdin>:3:6: error: ‘g’ was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
<stdin>:6:6: note: ‘void g(int)’ declared here, later in the translation unit

which doesn't seem right.

To fix the first issue, rather than disabling the diagnostic perhaps we
should just disable the backwards compatibility logic in the template-id
case, as in the below?

-- >8 --

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 82bf7dc26f6..e2d04a52894 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -20428,7 +20428,8 @@ tsubst_copy_and_build (tree t,
 		    && identifier_p (TREE_OPERAND (function, 0))))
 	    && !any_type_dependent_arguments_p (call_args))
 	  {
-	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
+	    bool template_id_p = (TREE_CODE (function) == TEMPLATE_ID_EXPR);
+	    if (template_id_p)
 	      function = TREE_OPERAND (function, 0);
 	    if (koenig_p && (complain & tf_warning_or_error))
 	      {
@@ -20443,20 +20444,21 @@ tsubst_copy_and_build (tree t,
 
 		if (unq != function)
 		  {
-		    /* In a lambda fn, we have to be careful to not
-		       introduce new this captures.  Legacy code can't
-		       be using lambdas anyway, so it's ok to be
-		       stricter.  */
-		    bool in_lambda = (current_class_type
-				      && LAMBDA_TYPE_P (current_class_type));
 		    char const *const msg
 		      = G_("%qD was not declared in this scope, "
 			   "and no declarations were found by "
 			   "argument-dependent lookup at the point "
 			   "of instantiation");
 
+		    bool in_lambda = (current_class_type
+				      && LAMBDA_TYPE_P (current_class_type));
+		    /* In a lambda fn, we have to be careful to not
+		       introduce new this captures.  Legacy code can't
+		       be using lambdas anyway, so it's ok to be
+		       stricter.  Be strict with C++20 template-id ADL too.  */
+		    bool strict = in_lambda || template_id_p;
 		    bool diag = true;
-		    if (in_lambda)
+		    if (strict)
 		      error_at (cp_expr_loc_or_input_loc (t),
 				msg, function);
 		    else
@@ -20492,7 +20494,7 @@ tsubst_copy_and_build (tree t,
 			  inform (DECL_SOURCE_LOCATION (fn),
 				  "%qD declared here, later in the "
 				  "translation unit", fn);
-			if (in_lambda)
+			if (strict)
 			  RETURN (error_mark_node);
 		      }