[C++] Fix weird addr_expr not supported by dump_expr messages (PR c++/90767)

Hi!

The following patch is a minimal fix to avoid
cannot convert ‘‘addr_expr’ not supported by dump_type<type error>’ to ‘X*’
and similar messages.  The recently added complain_about_bad_argument
function expects a from_type argument, but conv->from isn't necessarily a
type, it can be an expression too.

With this patch one gets error like:
cannot convert ‘const X*’ to ‘X*’
and note like
initializing argument 'this' of ‘void X::foo()’
Still, perhaps what GCC 8 and earlier used to emit might be clearer:
pr90767-1.C: In member function ‘X::operator T() const’:
pr90767-1.C:12:7: error: no matching function for call to ‘X::foo() const’
pr90767-1.C:6:8: note: candidate: ‘void X::foo()’ <near match>
pr90767-1.C:6:8: note:   passing ‘const X*’ as ‘this’ argument discards qualifiers
There is the print_conversion_rejection function that handles the various
cases, like this vs. other arguments, conv->from with expr type vs. type
etc.
Though, I must say I don't understand the reasons why complain_about_bad_argument
has been added and whether we'd want to emit there what
print_conversion_rejection prints as notes with 2 leading spaces instead as
errors with no leading spaces.

In any case, I think the patch below is a step in the right direction.

2019-11-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/90767
	* call.c (complain_about_no_candidates_for_method_call): If
	conv->from is not a type, pass to complain_about_bad_argument
	lvalue_type of conv->from.

	* g++.dg/diagnostic/pr90767-1.C: New test.
	* g++.dg/diagnostic/pr90767-2.C: New test.


	Jakub

On 11/19/19 11:38 PM, Jakub Jelinek wrote:
> Hi!
> 
> The following patch is a minimal fix to avoid
> cannot convert ‘‘addr_expr’ not supported by dump_type<type error>’ to ‘X*’
> and similar messages.  The recently added complain_about_bad_argument
> function expects a from_type argument, but conv->from isn't necessarily a
> type, it can be an expression too.
> 
> With this patch one gets error like:
> cannot convert ‘const X*’ to ‘X*’
> and note like
> initializing argument 'this' of ‘void X::foo()’
> Still, perhaps what GCC 8 and earlier used to emit might be clearer:
> pr90767-1.C: In member function ‘X::operator T() const’:
> pr90767-1.C:12:7: error: no matching function for call to ‘X::foo() const’
> pr90767-1.C:6:8: note: candidate: ‘void X::foo()’ <near match>
> pr90767-1.C:6:8: note:   passing ‘const X*’ as ‘this’ argument discards qualifiers
> There is the print_conversion_rejection function that handles the various
> cases, like this vs. other arguments, conv->from with expr type vs. type
> etc.
> Though, I must say I don't understand the reasons why complain_about_bad_argument
> has been added and whether we'd want to emit there what
> print_conversion_rejection prints as notes with 2 leading spaces instead as
> errors with no leading spaces.

Historically, when we have a single candidate we assume it's chosen by 
overload resolution and try to call it, so we often get different 
diagnostics.  Sometimes better, sometimes worse.  In this case it seems 
about even.  I'm surprised this case was different in GCC 8.

> In any case, I think the patch below is a step in the right direction.

Agreed, the patch is OK.

> 2019-11-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/90767
> 	* call.c (complain_about_no_candidates_for_method_call): If
> 	conv->from is not a type, pass to complain_about_bad_argument
> 	lvalue_type of conv->from.
> 
> 	* g++.dg/diagnostic/pr90767-1.C: New test.
> 	* g++.dg/diagnostic/pr90767-2.C: New test.
> 
> --- gcc/cp/call.c.jj	2019-11-18 18:49:14.461880924 +0100
> +++ gcc/cp/call.c	2019-11-19 14:40:19.121937148 +0100
> @@ -9861,8 +9861,11 @@ complain_about_no_candidates_for_method_
>   	  if (const conversion_info *conv
>   		= maybe_get_bad_conversion_for_unmatched_call (candidate))
>   	    {
> +	      tree from_type = conv->from;
> +	      if (!TYPE_P (conv->from))
> +		from_type = lvalue_type (conv->from);
>   	      complain_about_bad_argument (conv->loc,
> -					   conv->from, conv->to_type,
> +					   from_type, conv->to_type,
>   					   candidate->fn, conv->n_arg);
>   	      return;
>   	    }
> --- gcc/testsuite/g++.dg/diagnostic/pr90767-1.C.jj	2019-11-19 14:48:00.386041586 +0100
> +++ gcc/testsuite/g++.dg/diagnostic/pr90767-1.C	2019-11-19 14:46:53.395043036 +0100
> @@ -0,0 +1,15 @@
> +// PR c++/90767
> +// { dg-do compile }
> +
> +struct X {
> +  int n;
> +  void foo ();	// { dg-message "initializing argument 'this'" }
> +
> +  template<typename T>
> +  operator T () const
> +    {
> +      if (n == 0)
> +	foo ();	// { dg-error "cannot convert 'const X\\*' to 'X\\*'" }
> +      return n;
> +    }
> +};
> --- gcc/testsuite/g++.dg/diagnostic/pr90767-2.C.jj	2019-11-19 14:50:48.923522136 +0100
> +++ gcc/testsuite/g++.dg/diagnostic/pr90767-2.C	2019-11-19 14:52:27.324051149 +0100
> @@ -0,0 +1,15 @@
> +// PR c++/90767
> +// { dg-do compile }
> +
> +struct A {
> +  struct B { B (int) {} };
> +
> +  template <typename T>
> +  void foo ()
> +  {
> +    int x = 0;
> +    bar (x);	// { dg-error "cannot convert 'int' to 'A::B&'" }
> +  }
> +
> +  void bar (B &arg) {}	// { dg-message "initializing argument 1" }
> +};
> 
> 	Jakub
>

On Wed, 2019-11-20 at 00:38 +0100, Jakub Jelinek wrote:
> Hi!
> 
> The following patch is a minimal fix to avoid
> cannot convert ‘‘addr_expr’ not supported by dump_type<type error>’
> to ‘X*’
> and similar messages.  The recently added complain_about_bad_argument
> function expects a from_type argument, but conv->from isn't
> necessarily a
> type, it can be an expression too.
> 
> With this patch one gets error like:
> cannot convert ‘const X*’ to ‘X*’
> and note like
> initializing argument 'this' of ‘void X::foo()’
> Still, perhaps what GCC 8 and earlier used to emit might be clearer:
> pr90767-1.C: In member function ‘X::operator T() const’:
> pr90767-1.C:12:7: error: no matching function for call to ‘X::foo()
> const’
> pr90767-1.C:6:8: note: candidate: ‘void X::foo()’ <near match>
> pr90767-1.C:6:8: note:   passing ‘const X*’ as ‘this’ argument
> discards qualifiers
> There is the print_conversion_rejection function that handles the
> various
> cases, like this vs. other arguments, conv->from with expr type vs.
> type
> etc.
> Though, I must say I don't understand the reasons why
> complain_about_bad_argument
> has been added and whether we'd want to emit there what
> print_conversion_rejection prints as notes with 2 leading spaces
> instead as
> errors with no leading spaces.

I added complain_about_bad_argument in r264250 (aka
b78e49d1ddf1a09e16152353b41bf7c0b44d6405); the intent is to special-
case when there's a single candidate due to a bad argument, underlining
the pertinent bogus argument at the callsite, rather than having the
caret at the final close-paren of the call.

But it looks like I forgot about the "this" case.  Does it make more
sense for pr90767-1.C to reject the special-casing if conv->n_arg is -1
for the "this"?  (either in
complain_about_no_candidates_for_method_call, or in
maybe_get_bad_conversion_for_unmatched_call); perhaps like this:

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 0034c1c..eefb7ab 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -9855,16 +9855,18 @@ complain_about_no_candidates_for_method_call (tree instance,
   else
     {
       /* Special-case for when there's a single candidate that's failing
-        due to a bad argument type.  */
+        due to a bad argument type (but not for the "this" argument),
+        so that we can underline the pertinent argument at the callsite.  */
       if (z_candidate *candidate = single_z_candidate (candidates))
          if (const conversion_info *conv
                = maybe_get_bad_conversion_for_unmatched_call (candidate))
-           {
-             complain_about_bad_argument (conv->loc,
-                                          conv->from, conv->to_type,
-                                          candidate->fn, conv->n_arg);
-             return;
-           }
+           if (conv->n_arg != -1)
+             {
+               complain_about_bad_argument (conv->loc,
+                                            conv->from, conv->to_type,
+                                            candidate->fn, conv->n_arg);
+               return;
+             }
 
       tree arglist = build_tree_list_vec (user_args);
       tree errname = name;


(caveat: barely tested)

Dave

> In any case, I think the patch below is a step in the right
> direction.
> 
> 2019-11-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/90767
> 	* call.c (complain_about_no_candidates_for_method_call): If
> 	conv->from is not a type, pass to complain_about_bad_argument
> 	lvalue_type of conv->from.
> 
> 	* g++.dg/diagnostic/pr90767-1.C: New test.
> 	* g++.dg/diagnostic/pr90767-2.C: New test.
> 
> --- gcc/cp/call.c.jj	2019-11-18 18:49:14.461880924 +0100
> +++ gcc/cp/call.c	2019-11-19 14:40:19.121937148 +0100
> @@ -9861,8 +9861,11 @@ complain_about_no_candidates_for_method_
>  	  if (const conversion_info *conv
>  		= maybe_get_bad_conversion_for_unmatched_call
> (candidate))
>  	    {
> +	      tree from_type = conv->from;
> +	      if (!TYPE_P (conv->from))
> +		from_type = lvalue_type (conv->from);
>  	      complain_about_bad_argument (conv->loc,
> -					   conv->from, conv->to_type,
> +					   from_type, conv->to_type,
>  					   candidate->fn, conv->n_arg);
>  	      return;
>  	    }
> --- gcc/testsuite/g++.dg/diagnostic/pr90767-1.C.jj	2019-11-19
> 14:48:00.386041586 +0100
> +++ gcc/testsuite/g++.dg/diagnostic/pr90767-1.C	2019-11-19
> 14:46:53.395043036 +0100
> @@ -0,0 +1,15 @@
> +// PR c++/90767
> +// { dg-do compile }
> +
> +struct X {
> +  int n;
> +  void foo ();	// { dg-message "initializing argument 'this'"
> }
> +
> +  template<typename T>
> +  operator T () const
> +    {
> +      if (n == 0)
> +	foo ();	// { dg-error "cannot convert 'const X\\*' to 'X\\*'"
> }
> +      return n;
> +    }
> +};
> --- gcc/testsuite/g++.dg/diagnostic/pr90767-2.C.jj	2019-11-19
> 14:50:48.923522136 +0100
> +++ gcc/testsuite/g++.dg/diagnostic/pr90767-2.C	2019-11-19
> 14:52:27.324051149 +0100
> @@ -0,0 +1,15 @@
> +// PR c++/90767
> +// { dg-do compile }
> +
> +struct A {
> +  struct B { B (int) {} };
> +
> +  template <typename T>
> +  void foo ()
> +  {
> +    int x = 0;
> +    bar (x);	// { dg-error "cannot convert 'int' to 'A::B&'" }
> +  }
> +
> +  void bar (B &arg) {}	// { dg-message "initializing argument
> 1" }
> +};
> 
> 	Jakub

On Wed, Nov 20, 2019 at 05:54:48AM -0500, David Malcolm wrote:
> I added complain_about_bad_argument in r264250 (aka
> b78e49d1ddf1a09e16152353b41bf7c0b44d6405); the intent is to special-
> case when there's a single candidate due to a bad argument, underlining
> the pertinent bogus argument at the callsite, rather than having the
> caret at the final close-paren of the call.
> 
> But it looks like I forgot about the "this" case.  Does it make more
> sense for pr90767-1.C to reject the special-casing if conv->n_arg is -1
> for the "this"?  (either in
> complain_about_no_candidates_for_method_call, or in
> maybe_get_bad_conversion_for_unmatched_call); perhaps like this:

I don't know.  Either something like that, or handle those cases in
complain_about_bad_argument specially too.
Note, print_conversion_rejection apparently has special cases for
n_arg == -1 (TYPE_P conv->from vs. non-TYPE_P)
!TYPE_P conv->from
n_arg == -2
the rest
I really don't know what exactly they mean, appart from n_arg == -1
being this, n_arg seems to be return value, but dunno when that triggers,
and it is unclear what TYPE_P conv->from vs. non-TYPE_P means.

	Jakub

On 11/20/19 10:54 AM, David Malcolm wrote:
> On Wed, 2019-11-20 at 00:38 +0100, Jakub Jelinek wrote:
>> Hi!
>>
>> The following patch is a minimal fix to avoid
>> cannot convert ‘‘addr_expr’ not supported by dump_type<type error>’
>> to ‘X*’
>> and similar messages.  The recently added complain_about_bad_argument
>> function expects a from_type argument, but conv->from isn't
>> necessarily a
>> type, it can be an expression too.
>>
>> With this patch one gets error like:
>> cannot convert ‘const X*’ to ‘X*’
>> and note like
>> initializing argument 'this' of ‘void X::foo()’
>> Still, perhaps what GCC 8 and earlier used to emit might be clearer:
>> pr90767-1.C: In member function ‘X::operator T() const’:
>> pr90767-1.C:12:7: error: no matching function for call to ‘X::foo()
>> const’
>> pr90767-1.C:6:8: note: candidate: ‘void X::foo()’ <near match>
>> pr90767-1.C:6:8: note:   passing ‘const X*’ as ‘this’ argument
>> discards qualifiers
>> There is the print_conversion_rejection function that handles the
>> various
>> cases, like this vs. other arguments, conv->from with expr type vs.
>> type
>> etc.
>> Though, I must say I don't understand the reasons why
>> complain_about_bad_argument
>> has been added and whether we'd want to emit there what
>> print_conversion_rejection prints as notes with 2 leading spaces
>> instead as
>> errors with no leading spaces.
> 
> I added complain_about_bad_argument in r264250 (aka
> b78e49d1ddf1a09e16152353b41bf7c0b44d6405); the intent is to special-
> case when there's a single candidate due to a bad argument, underlining
> the pertinent bogus argument at the callsite, rather than having the
> caret at the final close-paren of the call.

Since apparently we already have the relevant location in 
conversion_info, could we achieve that by improving 
print_conversion_rejection rather than using a separate 
complain_about_bad_argument function?

Jason