[committed] v2: C++: underline parameters in mismatching function calls

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2017-09-21 at 09:20 -0700, Nathan Sidwell wrote:
> On 09/20/2017 12:52 PM, David Malcolm wrote:
> > When we have a type mismatch in a C++ function call, e.g.
> > whereas underlining the mismatching things would make the messages
> > easier to comprehend:
> > 
> >    test.c: In function 'int caller(int, int, float)':
> >    test.c:5:38: error: invalid conversion from 'int' to 'const
> > char*'
> >    [-fpermissive]
> >     return callee (first, second, third);
> >                           ^~~~~~
> >    test.c:1:12: note:   initializing argument 2 of 'int callee(int,
> >    const char*, float)'
> >     extern int callee (int one, const char *two, float three);
> 
> Looks nice.  Thanks for addressing what Jason & I said at the summit.
> 
> > OK for trunk?
> 
> A few nits, I think ...
> 
> 
> > +  location_t param_loc;
> > +  if (declarator && declarator->id_loc)
> > +    param_loc = make_location (declarator->id_loc,
> > +			       decl_spec_token_start->location,
> > +			       input_location);
> > +  else
> > +    param_loc = make_location (decl_spec_token_start->location,
> > +			       decl_spec_token_start->location,
> > +			       input_location);
> 
> Are you implicitly relying on UNKNOWN_LOCATION being zero?  Should
> the 
> if condition be
>    if (declarator && declarator->id_loc != UNKNOWN_LOCATION)
> ?
> 
> The only difference between those 2 calls is the first param.  I
> find 
> such idiom confusing.  Can you assign the arg to a temp and then
> call 
> the function exactly once? No need for re-review on that change.
> 
> >   /* A collection of calls where argument 2 is of the wrong type.
> >   
> >      TODO: we should put the caret and underline for the diagnostic
> > -   at the second argument, rather than the close paren.
> > -
> > -   TODO: we should highlight the second parameter of the callee,
> > rather
> > -   than its name.  */
> > +   at the second argument, rather than the close paren.  */
> 
> Yay, x-off a TODO!
> 
> Looks good otherwise.
> 
> nathan

Thanks.

I went ahead and committed the following to trunk (as r253096), which
fixes the issues you pointed out above, and the FUNCTION_FIRST_USER_PARM
one Jason pointed out.

(Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu)

gcc/cp/ChangeLog:
	* call.c (get_fndecl_argument_location): New function.
	(convert_like_real): Use it  when complaining about argument type
	mismatches.
	* cp-tree.h (struct cp_parameter_declarator): Add "loc" field.
	* parser.c (make_parameter_declarator): Add "loc" param and use
	it to initialize the new field.
	(cp_parser_translation_unit): Add UNKNOWN_LOCATION for "loc" of
	the "no_parameters" parameter.
	(cp_parser_parameter_declaration_list): Set the location of the
	result of grokdeclarator to be the parameter's loc, assuming no
	errors.
	(cp_parser_parameter_declaration): Generate a location for the
	parameter and pass to make_parameter_declarator.

gcc/testsuite/ChangeLog:
	* g++.dg/diagnostic/param-type-mismatch.C: Update expected results
	to reflect highlighting of parameters; add test coverage for
	callback parameters.
---
 gcc/cp/call.c                                      | 26 +++++++++-
 gcc/cp/cp-tree.h                                   |  2 +
 gcc/cp/parser.c                                    | 32 +++++++++++-
 .../g++.dg/diagnostic/param-type-mismatch.C        | 57 +++++++++++++++++-----
 4 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 4fa0d03..e83cf99 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6579,6 +6579,30 @@ maybe_print_user_conv_context (conversion *convs)
 	}
 }
 
+/* Locate the parameter with the given index within FNDECL.
+   ARGNUM is zero based, -1 indicates the `this' argument of a method.
+   Return the location of the FNDECL itself if there are problems.  */
+
+static location_t
+get_fndecl_argument_location (tree fndecl, int argnum)
+{
+  int i;
+  tree param;
+
+  /* Locate param by index within DECL_ARGUMENTS (fndecl).  */
+  for (i = 0, param = FUNCTION_FIRST_USER_PARM (fndecl);
+       i < argnum && param;
+       i++, param = TREE_CHAIN (param))
+    ;
+
+  /* If something went wrong (e.g. if we have a builtin and thus no arguments),
+     return the location of FNDECL.  */
+  if (param == NULL)
+    return DECL_SOURCE_LOCATION (fndecl);
+
+  return DECL_SOURCE_LOCATION (param);
+}
+
 /* Perform the conversions in CONVS on the expression EXPR.  FN and
    ARGNUM are used for diagnostics.  ARGNUM is zero based, -1
    indicates the `this' argument of a method.  INNER is nonzero when
@@ -6680,7 +6704,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	complained = permerror (loc, "invalid conversion from %qH to %qI",
 				TREE_TYPE (expr), totype);
       if (complained && fn)
-	inform (DECL_SOURCE_LOCATION (fn),
+	inform (get_fndecl_argument_location (fn, argnum),
 		"  initializing argument %P of %qD", argnum, fn);
 
       return cp_convert (totype, expr, complain);
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index e508598..7c1c54c 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5659,6 +5659,8 @@ struct cp_parameter_declarator {
   tree default_argument;
   /* True iff this is a template parameter pack.  */
   bool template_parameter_pack_p;
+  /* Location within source.  */
+  location_t loc;
 };
 
 /* A declarator.  */
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 25b91df..f9b6f27 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -1691,6 +1691,7 @@ cp_parameter_declarator *
 make_parameter_declarator (cp_decl_specifier_seq *decl_specifiers,
 			   cp_declarator *declarator,
 			   tree default_argument,
+			   location_t loc,
 			   bool template_parameter_pack_p = false)
 {
   cp_parameter_declarator *parameter;
@@ -1705,6 +1706,7 @@ make_parameter_declarator (cp_decl_specifier_seq *decl_specifiers,
   parameter->declarator = declarator;
   parameter->default_argument = default_argument;
   parameter->template_parameter_pack_p = template_parameter_pack_p;
+  parameter->loc = loc;
 
   return parameter;
 }
@@ -4379,7 +4381,8 @@ cp_parser_translation_unit (cp_parser* parser)
       /* Create the error declarator.  */
       cp_error_declarator = make_declarator (cdk_error);
       /* Create the empty parameter list.  */
-      no_parameters = make_parameter_declarator (NULL, NULL, NULL_TREE);
+      no_parameters = make_parameter_declarator (NULL, NULL, NULL_TREE,
+						 UNKNOWN_LOCATION);
       /* Remember where the base of the declarator obstack lies.  */
       declarator_obstack_base = obstack_next_free (&declarator_obstack);
     }
@@ -21218,6 +21221,8 @@ cp_parser_parameter_declaration_list (cp_parser* parser, bool *is_error)
 				 PARM,
 				 parameter->default_argument != NULL_TREE,
 				 &parameter->decl_specifiers.attributes);
+	  if (decl != error_mark_node && parameter->loc != UNKNOWN_LOCATION)
+	    DECL_SOURCE_LOCATION (decl) = parameter->loc;
 	}
 
       deprecated_state = DEPRECATED_NORMAL;
@@ -21371,6 +21376,7 @@ cp_parser_parameter_declaration (cp_parser *parser,
     = G_("types may not be defined in parameter types");
 
   /* Parse the declaration-specifiers.  */
+  cp_token *decl_spec_token_start = cp_lexer_peek_token (parser->lexer);
   cp_parser_decl_specifier_seq (parser,
 				CP_PARSER_FLAGS_NONE,
 				&decl_specifiers,
@@ -21555,9 +21561,33 @@ cp_parser_parameter_declaration (cp_parser *parser,
   else
     default_argument = NULL_TREE;
 
+  /* Generate a location for the parameter, ranging from the start of the
+     initial token to the end of the final token (using input_location for
+     the latter, set up by cp_lexer_set_source_position_from_token when
+     consuming tokens).
+
+     If we have a identifier, then use it for the caret location, e.g.
+
+       extern int callee (int one, int (*two)(int, int), float three);
+                                   ~~~~~~^~~~~~~~~~~~~~
+
+     otherwise, reuse the start location for the caret location e.g.:
+
+       extern int callee (int one, int (*)(int, int), float three);
+                                   ^~~~~~~~~~~~~~~~~
+
+  */
+  location_t caret_loc = (declarator && declarator->id_loc != UNKNOWN_LOCATION
+			  ? declarator->id_loc
+			  : decl_spec_token_start->location);
+  location_t param_loc = make_location (caret_loc,
+					decl_spec_token_start->location,
+					input_location);
+
   return make_parameter_declarator (&decl_specifiers,
 				    declarator,
 				    default_argument,
+				    param_loc,
 				    template_parameter_pack_p);
 }
 
diff --git a/gcc/testsuite/g++.dg/diagnostic/param-type-mismatch.C b/gcc/testsuite/g++.dg/diagnostic/param-type-mismatch.C
index b8833ef..bc3a938 100644
--- a/gcc/testsuite/g++.dg/diagnostic/param-type-mismatch.C
+++ b/gcc/testsuite/g++.dg/diagnostic/param-type-mismatch.C
@@ -3,10 +3,7 @@
 /* A collection of calls where argument 2 is of the wrong type.
 
    TODO: we should put the caret and underline for the diagnostic
-   at the second argument, rather than the close paren.
-
-   TODO: we should highlight the second parameter of the callee, rather
-   than its name.  */
+   at the second argument, rather than the close paren.  */
 
 /* decl, with argname.  */
 
@@ -22,7 +19,7 @@ int test_1 (int first, int second, float third)
   // { dg-message "initializing argument 2 of 'int callee_1\\(int, const char\\*, float\\)'" "" { target *-*-* } callee_1 }
   /* { dg-begin-multiline-output "" }
  extern int callee_1 (int one, const char *two, float three);
-            ^~~~~~~~
+                               ~~~~~~~~~~~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -40,7 +37,7 @@ int test_2 (int first, int second, float third)
   // { dg-message "initializing argument 2 of 'int callee_2\\(int, const char\\*, float\\)'" "" { target *-*-* } callee_2 }
   /* { dg-begin-multiline-output "" }
  extern int callee_2 (int, const char *, float);
-            ^~~~~~~~
+                           ^~~~~~~~~~~~
      { dg-end-multiline-output "" } */
 }
 
@@ -61,7 +58,7 @@ int test_3 (int first, int second, float third)
   // { dg-message "initializing argument 2 of 'int callee_3\\(int, const char\\*, float\\)'" "" { target *-*-* } callee_3 }
   /* { dg-begin-multiline-output "" }
  static int callee_3 (int one, const char *two, float three)
-            ^~~~~~~~
+                               ~~~~~~~~~~~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -78,7 +75,7 @@ int test_4 (int first, int second, float third)
      { dg-end-multiline-output "" } */
   /* { dg-begin-multiline-output "" }
  struct s4 { static int member_1 (int one, const char *two, float three); };
-                        ^~~~~~~~
+                                           ~~~~~~~~~~~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -96,7 +93,7 @@ int test_5 (int first, int second, float third)
      { dg-end-multiline-output "" } */
   /* { dg-begin-multiline-output "" }
  struct s5 { int member_1 (int one, const char *two, float three); };
-                 ^~~~~~~~
+                                    ~~~~~~~~~~~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -113,7 +110,7 @@ int test_6 (int first, int second, float third, s6 *ptr)
      { dg-end-multiline-output "" } */
   /* { dg-begin-multiline-output "" }
  struct s6 { int member_1 (int one, const char *two, float three); };
-                 ^~~~~~~~
+                                    ~~~~~~~~~~~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -153,7 +150,7 @@ int test_8 (int first, int second, float third)
      { dg-end-multiline-output "" } */
   /* { dg-begin-multiline-output "" }
  struct s8 { static int member_1 (int one, T two, float three); };
-                        ^~~~~~~~
+                                           ~~^~~
      { dg-end-multiline-output "" } */
 }
 
@@ -172,7 +169,43 @@ int test_9 (int first, int second, float third)
      { dg-end-multiline-output "" } */
   /* { dg-begin-multiline-output "" }
  struct s9 { int member_1 (int one, T two, float three); };
-                 ^~~~~~~~
+                                    ~~^~~
+     { dg-end-multiline-output "" } */
+}
+
+/* Callback with name.  */
+
+extern int callee_10 (int one, int (*two)(int, int), float three); // { dg-line callee_10 }
+
+int test_10 (int first, int second, float third)
+{
+  return callee_10 (first, second, third); // { dg-error "invalid conversion from 'int' to 'int \\(\\*\\)\\(int, int\\)'" }
+  /* { dg-begin-multiline-output "" }
+   return callee_10 (first, second, third);
+                                         ^
+     { dg-end-multiline-output "" } */
+  // { dg-message "initializing argument 2 of 'int callee_10\\(int, int \\(\\*\\)\\(int, int\\), float\\)'" "" { target *-*-* } callee_10 }
+  /* { dg-begin-multiline-output "" }
+ extern int callee_10 (int one, int (*two)(int, int), float three);
+                                ~~~~~~^~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+}
+
+/* Callback without name.  */
+
+extern int callee_11 (int one, int (*)(int, int), float three); // { dg-line callee_11 }
+
+int test_11 (int first, int second, float third)
+{
+  return callee_11 (first, second, third); // { dg-error "invalid conversion from 'int' to 'int \\(\\*\\)\\(int, int\\)'" }
+  /* { dg-begin-multiline-output "" }
+   return callee_11 (first, second, third);
+                                         ^
+     { dg-end-multiline-output "" } */
+  // { dg-message "initializing argument 2 of 'int callee_11\\(int, int \\(\\*\\)\\(int, int\\), float\\)'" "" { target *-*-* } callee_11 }
+  /* { dg-begin-multiline-output "" }
+ extern int callee_11 (int one, int (*)(int, int), float three);
+                                ^~~~~~~~~~~~~~~~~
      { dg-end-multiline-output "" } */
 }
 
-- 
1.8.5.3