[PATCH] c++: unqual lookup performed twice w/ template-id ADL [PR102670]

[PATCH] c++: unqual lookup performed twice w/ template-id ADL [PR102670]

[Patrick Palka]

Here we're incorrectly performing unqualified lookup of 'adl' again at
substitution time for the call adl<I>(t) (for which name lookup at parse
time found nothing) which causes us to reject the testcase because the
second unqualified lookup finds the later-declared variable template
'adl', leading to confusion.  Fixed thusly.

The testcase concepts-recursive-sat1.C needed to be adjusted use ADL
proper instead of relying on this incorrect behavior.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk and perhaps 11 given it's a C++20 bugfix?

	PR c++/102670

gcc/cp/ChangeLog:

	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: When looking
	for an identifier callee in the koenig_p case, also look through
	TEMPLATE_ID_EXPR.  Use tsubst_copy to substitute through the
	template arguments of the template-id.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-recursive-sat1.C:
	* g++.dg/cpp2a/fn-template23.C: New test.
---
 gcc/cp/pt.c                                   | 11 +++++-
 .../g++.dg/cpp2a/concepts-recursive-sat1.C    | 15 +++++---
 gcc/testsuite/g++.dg/cpp2a/fn-template23.C    | 36 +++++++++++++++++++
 3 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template23.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 66040035b2f..40f84648ed2 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -20256,7 +20256,10 @@ tsubst_copy_and_build (tree t,
 					    /*done=*/false,
 					    /*address_p=*/false);
 	  }
-	else if (koenig_p && identifier_p (function))
+	else if (koenig_p
+		 && (identifier_p (function)
+		     || (TREE_CODE (function) == TEMPLATE_ID_EXPR
+			 && identifier_p (TREE_OPERAND (function, 0)))))
 	  {
 	    /* Do nothing; calling tsubst_copy_and_build on an identifier
 	       would incorrectly perform unqualified lookup again.
@@ -20269,6 +20272,12 @@ tsubst_copy_and_build (tree t,
 	       FIXME but doing that causes c++/15272, so we need to stop
 	       using IDENTIFIER_NODE in that situation.  */
 	    qualified_p = false;
+
+	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
+	      /* Use tsubst_copy to substitute through the template arguments
+		 of the template-id without performing unqualified lookup on
+		 the template name.  */
+	      function = tsubst_copy (function, args, complain, in_decl);
 	  }
 	else
 	  {
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
index 22696c30d81..4c178b77946 100644
--- a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
@@ -3,16 +3,21 @@
 template<int N, typename T>
 concept Foo = requires(T t) { foo<N + 1>(t); }; // { dg-error "template instantiation depth" }
 
-template<int N = 1, typename T = int>
-  requires Foo<N, T>
-int foo(T t)
+namespace ns
 {
-  return foo<N + 1>(t);
+  struct S { };
+
+  template<int N, typename T>
+    requires Foo<N, T>
+  int foo(T t)
+  {
+    return foo<N + 1>(t);
+  }
 }
 
 int main(int, char**)
 {
-  return foo<1>(1);
+  return ns::foo<1>(ns::S{});
 }
 
 // { dg-prune-output "compilation terminated" }
diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template23.C b/gcc/testsuite/g++.dg/cpp2a/fn-template23.C
new file mode 100644
index 00000000000..b85d4c96dab
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/fn-template23.C
@@ -0,0 +1,36 @@
+// PR c++/102670
+// { dg-do compile { target c++20 } }
+
+namespace ns {
+  struct S { };
+
+  template<int I>
+  constexpr int adl(const S &) {
+    return I;
+  }
+}
+
+namespace redirect {
+  template<class T, int I>
+  concept can_call_adl = requires(T t) {
+    adl<I>(t);
+  };
+
+  template<int I>
+  struct adl_fn {
+    template<can_call_adl<I> T>
+    constexpr decltype(auto) operator()(T t) const {
+      return adl<I>(t);
+    }
+  };
+
+  namespace {
+    template<int I>
+    constexpr inline adl_fn<I> adl{};
+  }
+}
+
+int main() {
+  static_assert(redirect::can_call_adl<ns::S, 3>);
+  redirect::adl<3>(ns::S{});
+}
-- 
2.34.0.rc0.19.g0cddd84c9f

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

[Patrick Palka]

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

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk and perhaps 11?

	PR c++/99911

gcc/cp/ChangeLog:

	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Don't diagnose
	name lookup failure if the arguments to an unresolved template
	name are still dependent.  Disable the -fpermissive fallback for
	template-id ADL.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/fn-template24.C: New test.
---
 gcc/cp/pt.c                                |  6 ++++--
 gcc/testsuite/g++.dg/cpp2a/fn-template24.C | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template24.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 991a20a85d4..4beddf9caf8 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -20427,12 +20427,14 @@ tsubst_copy_and_build (tree t,
 	if (function != NULL_TREE
 	    && (identifier_p (function)
 		|| (TREE_CODE (function) == TEMPLATE_ID_EXPR
-		    && identifier_p (TREE_OPERAND (function, 0))))
+		    && identifier_p (TREE_OPERAND (function, 0))
+		    && !any_dependent_template_arguments_p (TREE_OPERAND
+							    (function, 1))))
 	    && !any_type_dependent_arguments_p (call_args))
 	  {
 	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
 	      function = TREE_OPERAND (function, 0);
-	    if (koenig_p && (complain & tf_warning_or_error))
+	    else if (koenig_p && (complain & tf_warning_or_error))
 	      {
 		/* For backwards compatibility and good diagnostics, try
 		   the unqualified lookup again if we aren't in SFINAE
diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template24.C b/gcc/testsuite/g++.dg/cpp2a/fn-template24.C
new file mode 100644
index 00000000000..b444ac6a273
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/fn-template24.C
@@ -0,0 +1,16 @@
+// PR c++/99911
+// { dg-do compile { target c++20 } }
+
+namespace N {
+  struct A { };
+  template<class T> void get(A);
+};
+
+template<class T>
+auto f() {
+  return []<class U>(U) { get<U>(T{}); };
+}
+
+int main() {
+  f<N::A>()(0);
+}
-- 
2.34.0.rc1.14.g88d915a634

Re: [PATCH] c++: unqual lookup performed twice w/ template-id ADL [PR102670]

[Jason Merrill]

On 11/3/21 12:04, Patrick Palka wrote:
> Here we're incorrectly performing unqualified lookup of 'adl' again at
> substitution time for the call adl<I>(t) (for which name lookup at parse
> time found nothing) which causes us to reject the testcase because the
> second unqualified lookup finds the later-declared variable template
> 'adl', leading to confusion.  Fixed thusly.
> 
> The testcase concepts-recursive-sat1.C needed to be adjusted use ADL
> proper instead of relying on this incorrect behavior.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk and perhaps 11 given it's a C++20 bugfix?

OK for trunk.  Not for 11, I think, as it also affects non-C++20 code.

> 	PR c++/102670
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: When looking
> 	for an identifier callee in the koenig_p case, also look through
> 	TEMPLATE_ID_EXPR.  Use tsubst_copy to substitute through the
> 	template arguments of the template-id.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-recursive-sat1.C:
> 	* g++.dg/cpp2a/fn-template23.C: New test.
> ---
>   gcc/cp/pt.c                                   | 11 +++++-
>   .../g++.dg/cpp2a/concepts-recursive-sat1.C    | 15 +++++---
>   gcc/testsuite/g++.dg/cpp2a/fn-template23.C    | 36 +++++++++++++++++++
>   3 files changed, 56 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template23.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 66040035b2f..40f84648ed2 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -20256,7 +20256,10 @@ tsubst_copy_and_build (tree t,
>   					    /*done=*/false,
>   					    /*address_p=*/false);
>   	  }
> -	else if (koenig_p && identifier_p (function))
> +	else if (koenig_p
> +		 && (identifier_p (function)
> +		     || (TREE_CODE (function) == TEMPLATE_ID_EXPR
> +			 && identifier_p (TREE_OPERAND (function, 0)))))
>   	  {
>   	    /* Do nothing; calling tsubst_copy_and_build on an identifier
>   	       would incorrectly perform unqualified lookup again.
> @@ -20269,6 +20272,12 @@ tsubst_copy_and_build (tree t,
>   	       FIXME but doing that causes c++/15272, so we need to stop
>   	       using IDENTIFIER_NODE in that situation.  */
>   	    qualified_p = false;
> +
> +	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
> +	      /* Use tsubst_copy to substitute through the template arguments
> +		 of the template-id without performing unqualified lookup on
> +		 the template name.  */
> +	      function = tsubst_copy (function, args, complain, in_decl);
>   	  }
>   	else
>   	  {
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
> index 22696c30d81..4c178b77946 100644
> --- a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat1.C
> @@ -3,16 +3,21 @@
>   template<int N, typename T>
>   concept Foo = requires(T t) { foo<N + 1>(t); }; // { dg-error "template instantiation depth" }
>   
> -template<int N = 1, typename T = int>
> -  requires Foo<N, T>
> -int foo(T t)
> +namespace ns
>   {
> -  return foo<N + 1>(t);
> +  struct S { };
> +
> +  template<int N, typename T>
> +    requires Foo<N, T>
> +  int foo(T t)
> +  {
> +    return foo<N + 1>(t);
> +  }
>   }
>   
>   int main(int, char**)
>   {
> -  return foo<1>(1);
> +  return ns::foo<1>(ns::S{});
>   }
>   
>   // { dg-prune-output "compilation terminated" }
> diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template23.C b/gcc/testsuite/g++.dg/cpp2a/fn-template23.C
> new file mode 100644
> index 00000000000..b85d4c96dab
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/fn-template23.C
> @@ -0,0 +1,36 @@
> +// PR c++/102670
> +// { dg-do compile { target c++20 } }
> +
> +namespace ns {
> +  struct S { };
> +
> +  template<int I>
> +  constexpr int adl(const S &) {
> +    return I;
> +  }
> +}
> +
> +namespace redirect {
> +  template<class T, int I>
> +  concept can_call_adl = requires(T t) {
> +    adl<I>(t);
> +  };
> +
> +  template<int I>
> +  struct adl_fn {
> +    template<can_call_adl<I> T>
> +    constexpr decltype(auto) operator()(T t) const {
> +      return adl<I>(t);
> +    }
> +  };
> +
> +  namespace {
> +    template<int I>
> +    constexpr inline adl_fn<I> adl{};
> +  }
> +}
> +
> +int main() {
> +  static_assert(redirect::can_call_adl<ns::S, 3>);
> +  redirect::adl<3>(ns::S{});
> +}
> 

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

[Jason Merrill]

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?

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk and perhaps 11?
> 
> 	PR c++/99911
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Don't diagnose
> 	name lookup failure if the arguments to an unresolved template
> 	name are still dependent.  Disable the -fpermissive fallback for
> 	template-id ADL.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/fn-template24.C: New test.
> ---
>   gcc/cp/pt.c                                |  6 ++++--
>   gcc/testsuite/g++.dg/cpp2a/fn-template24.C | 16 ++++++++++++++++
>   2 files changed, 20 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template24.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 991a20a85d4..4beddf9caf8 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -20427,12 +20427,14 @@ tsubst_copy_and_build (tree t,
>   	if (function != NULL_TREE
>   	    && (identifier_p (function)
>   		|| (TREE_CODE (function) == TEMPLATE_ID_EXPR
> -		    && identifier_p (TREE_OPERAND (function, 0))))
> +		    && identifier_p (TREE_OPERAND (function, 0))
> +		    && !any_dependent_template_arguments_p (TREE_OPERAND
> +							    (function, 1))))
>   	    && !any_type_dependent_arguments_p (call_args))
>   	  {
>   	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
>   	      function = TREE_OPERAND (function, 0);
> -	    if (koenig_p && (complain & tf_warning_or_error))
> +	    else if (koenig_p && (complain & tf_warning_or_error))
>   	      {
>   		/* For backwards compatibility and good diagnostics, try
>   		   the unqualified lookup again if we aren't in SFINAE
> diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template24.C b/gcc/testsuite/g++.dg/cpp2a/fn-template24.C
> new file mode 100644
> index 00000000000..b444ac6a273
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/fn-template24.C
> @@ -0,0 +1,16 @@
> +// PR c++/99911
> +// { dg-do compile { target c++20 } }
> +
> +namespace N {
> +  struct A { };
> +  template<class T> void get(A);
> +};
> +
> +template<class T>
> +auto f() {
> +  return []<class U>(U) { get<U>(T{}); };
> +}
> +
> +int main() {
> +  f<N::A>()(0);
> +}
> 

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

[Patrick Palka]

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);
 		      }
 

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

[Patrick Palka]

On Thu, 18 Nov 2021, Patrick Palka wrote:

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

Now in patch form:

-- >8 --

Subject: [PATCH] c++: error recovery during C++20 template-id ADL failure

When diagnosing ADL failure we perform a second unqualified lookup for
backwards compatibility with legacy code, and for better diagnostics.

For C++20 template-id ADL however, the backwards compatibility logic
causes confusing subsequent diagnostics, such as in the testcase below
where we report deduction failure following the useful "no declarations
were found by ADL" diagnostic because we've discarded the arguments of
the template-id when replacing it with the later-declared template.

So for C++20 template-id ADL, this patch just disables the backwards
compatibility code while keeping the useful diagnostic.

gcc/cp/ChangeLog:

	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Disable the
	-fpermissive fallback for template-id ADL, but keep the
	diagnostic.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/fn-template25.C: New test.
---
 gcc/cp/pt.c                                | 23 +++++++++++++---------
 gcc/testsuite/g++.dg/cpp2a/fn-template25.C | 14 +++++++++++++
 2 files changed, 28 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template25.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ad51c07347b..3f1550a17ad 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -20439,8 +20439,12 @@ tsubst_copy_and_build (tree t,
 							    (function, 1))))
 	    && !any_type_dependent_arguments_p (call_args))
 	  {
+	    bool template_id_p = false;
 	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
-	      function = TREE_OPERAND (function, 0);
+	      {
+		function = TREE_OPERAND (function, 0);
+		template_id_p = true;
+	      }
 	    if (koenig_p && (complain & tf_warning_or_error))
 	      {
 		/* For backwards compatibility and good diagnostics, try
@@ -20454,20 +20458,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
@@ -20503,7 +20508,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);
 		      }
 
diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template25.C b/gcc/testsuite/g++.dg/cpp2a/fn-template25.C
new file mode 100644
index 00000000000..a8888af2023
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/fn-template25.C
@@ -0,0 +1,14 @@
+// { dg-do compile { target c++20 } }
+
+template<class T>
+void f() {
+  g<int>(T{}); // { dg-error "argument-dependent lookup" }
+	       // { dg-bogus "no match" "" { target *-*-* } .-1 }
+}
+
+template<class T>
+void g(int);   // { dg-message "declared here, later" }
+
+int main() {
+  f<int>();
+}
-- 
2.34.0

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

[Jason Merrill]

On 11/18/21 10:31, Patrick Palka wrote:
> On Thu, 18 Nov 2021, Patrick Palka wrote:
> 
>> 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?
> 
> Now in patch form:
> 
> -- >8 --
> 
> Subject: [PATCH] c++: error recovery during C++20 template-id ADL failure
> 
> When diagnosing ADL failure we perform a second unqualified lookup for
> backwards compatibility with legacy code, and for better diagnostics.
> 
> For C++20 template-id ADL however, the backwards compatibility logic
> causes confusing subsequent diagnostics, such as in the testcase below
> where we report deduction failure following the useful "no declarations
> were found by ADL" diagnostic because we've discarded the arguments of
> the template-id when replacing it with the later-declared template.
> 
> So for C++20 template-id ADL, this patch just disables the backwards
> compatibility code while keeping the useful diagnostic.
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Disable the
> 	-fpermissive fallback for template-id ADL, but keep the
> 	diagnostic.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/fn-template25.C: New test.
> ---
>   gcc/cp/pt.c                                | 23 +++++++++++++---------
>   gcc/testsuite/g++.dg/cpp2a/fn-template25.C | 14 +++++++++++++
>   2 files changed, 28 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/fn-template25.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index ad51c07347b..3f1550a17ad 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -20439,8 +20439,12 @@ tsubst_copy_and_build (tree t,
>   							    (function, 1))))
>   	    && !any_type_dependent_arguments_p (call_args))
>   	  {
> +	    bool template_id_p = false;
>   	    if (TREE_CODE (function) == TEMPLATE_ID_EXPR)
> -	      function = TREE_OPERAND (function, 0);
> +	      {
> +		function = TREE_OPERAND (function, 0);
> +		template_id_p = true;
> +	      }

I think

   bool template_id_p = TREE_CODE (function) == TEMPLATE_ID_EXPR;

would simplify this.  OK either way.

>   	    if (koenig_p && (complain & tf_warning_or_error))
>   	      {
>   		/* For backwards compatibility and good diagnostics, try
> @@ -20454,20 +20458,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
> @@ -20503,7 +20508,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);
>   		      }
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/fn-template25.C b/gcc/testsuite/g++.dg/cpp2a/fn-template25.C
> new file mode 100644
> index 00000000000..a8888af2023
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/fn-template25.C
> @@ -0,0 +1,14 @@
> +// { dg-do compile { target c++20 } }
> +
> +template<class T>
> +void f() {
> +  g<int>(T{}); // { dg-error "argument-dependent lookup" }
> +	       // { dg-bogus "no match" "" { target *-*-* } .-1 }
> +}
> +
> +template<class T>
> +void g(int);   // { dg-message "declared here, later" }
> +
> +int main() {
> +  f<int>();
> +}
>