diff mbox series

C++: Fix-it hints for '.' vs '->' (PR c++/84898)

Message ID 1535575020-41466-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series C++: Fix-it hints for '.' vs '->' (PR c++/84898) | expand

Commit Message

David Malcolm Aug. 29, 2018, 8:37 p.m. UTC
This patch implements fix-it hints for "." vs "->" mismatches in the C++
frontend, for the places where the relevant location information is
readily available.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
adds 39 PASS results to g++.sum.

OK for trunk?

gcc/cp/ChangeLog:
	PR c++/84898
	* cp-tree.h (finish_class_member_access_expr): Add location_t
	param.
	(build_x_arrow): Likewise.
	* parser.c (cp_parser_postfix_expression): Retain the location
	of the "." or "->" token and pass it to
	cp_parser_postfix_dot_deref_expression.
	(cp_parser_postfix_dot_deref_expression): Add "loc_dot_or_deref"
	param.  Pass it on to calls to build_x_arrow and
	finish_class_member_access_expr.
	(cp_parser_builtin_offsetof): Update for new param.
	(cp_parser_range_for_member_function): Likewise.
	(cp_parser_omp_var_list_no_open): Likewise.
	* pt.c (tsubst_copy_and_build): Likewise.
	* semantics.c (finish_id_expression): Likewise.
	* typeck.c (finish_class_member_access_expr): Add new param
	"loc_dot_or_deref".  When not UNKNOWN_LOCATION, use it to provide
	a "->" fix-it hint for the pointer type error, and for the error's
	location, rather than input_location, thus moving it from the
	field name to the "." token.
	* typeck2.c: Include "gcc-rich-location.h".
	(build_x_arrow): Add param "loc_deref".  Use it to provide
	a fix-it hint.

gcc/objc/ChangeLog:
	PR c++/84898
	* objc-act.c (objc_build_component_ref): Update for new param.

gcc/testsuite/ChangeLog:
	PR c++/84898
	* g++.dg/diagnostic/fixits.C: New test.
	* g++.dg/parse/error20.C: Update expected column number
	to be on the erroneous ".", rather than the fieldname.

libcc1/ChangeLog:
	PR c++/84898
	* libcp1plugin.cc (plugin_build_binary_expr): Update for new
	parameters of build_x_arrow and finish_class_member_access_expr.
---
 gcc/cp/cp-tree.h                         |  4 +-
 gcc/cp/parser.c                          | 39 +++++++++------
 gcc/cp/pt.c                              |  4 +-
 gcc/cp/semantics.c                       |  3 +-
 gcc/cp/typeck.c                          | 20 ++++++--
 gcc/cp/typeck2.c                         | 13 ++++-
 gcc/objc/objc-act.c                      |  3 +-
 gcc/testsuite/g++.dg/diagnostic/fixits.C | 83 ++++++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/parse/error20.C     |  2 +-
 libcc1/libcp1plugin.cc                   |  5 +-
 10 files changed, 147 insertions(+), 29 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/diagnostic/fixits.C

Comments

Jason Merrill Aug. 30, 2018, 7:18 p.m. UTC | #1
On Wed, Aug 29, 2018 at 4:37 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> This patch implements fix-it hints for "." vs "->" mismatches in the C++
> frontend, for the places where the relevant location information is
> readily available.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> adds 39 PASS results to g++.sum.
>
> OK for trunk?
>
> gcc/cp/ChangeLog:
>         PR c++/84898
>         * cp-tree.h (finish_class_member_access_expr): Add location_t
>         param.
>         (build_x_arrow): Likewise.
>         * parser.c (cp_parser_postfix_expression): Retain the location
>         of the "." or "->" token and pass it to
>         cp_parser_postfix_dot_deref_expression.
>         (cp_parser_postfix_dot_deref_expression): Add "loc_dot_or_deref"
>         param.  Pass it on to calls to build_x_arrow and
>         finish_class_member_access_expr.
>         (cp_parser_builtin_offsetof): Update for new param.
>         (cp_parser_range_for_member_function): Likewise.
>         (cp_parser_omp_var_list_no_open): Likewise.
>         * pt.c (tsubst_copy_and_build): Likewise.
>         * semantics.c (finish_id_expression): Likewise.
>         * typeck.c (finish_class_member_access_expr): Add new param
>         "loc_dot_or_deref".  When not UNKNOWN_LOCATION, use it to provide
>         a "->" fix-it hint for the pointer type error, and for the error's
>         location, rather than input_location, thus moving it from the
>         field name to the "." token.
>         * typeck2.c: Include "gcc-rich-location.h".
>         (build_x_arrow): Add param "loc_deref".  Use it to provide
>         a fix-it hint.
>
> gcc/objc/ChangeLog:
>         PR c++/84898
>         * objc-act.c (objc_build_component_ref): Update for new param.
>
> gcc/testsuite/ChangeLog:
>         PR c++/84898
>         * g++.dg/diagnostic/fixits.C: New test.
>         * g++.dg/parse/error20.C: Update expected column number
>         to be on the erroneous ".", rather than the fieldname.
>
> libcc1/ChangeLog:
>         PR c++/84898
>         * libcp1plugin.cc (plugin_build_binary_expr): Update for new
>         parameters of build_x_arrow and finish_class_member_access_expr.
> ---
>  gcc/cp/cp-tree.h                         |  4 +-
>  gcc/cp/parser.c                          | 39 +++++++++------
>  gcc/cp/pt.c                              |  4 +-
>  gcc/cp/semantics.c                       |  3 +-
>  gcc/cp/typeck.c                          | 20 ++++++--
>  gcc/cp/typeck2.c                         | 13 ++++-
>  gcc/objc/objc-act.c                      |  3 +-
>  gcc/testsuite/g++.dg/diagnostic/fixits.C | 83 ++++++++++++++++++++++++++++++++
>  gcc/testsuite/g++.dg/parse/error20.C     |  2 +-
>  libcc1/libcp1plugin.cc                   |  5 +-
>  10 files changed, 147 insertions(+), 29 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/diagnostic/fixits.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 055f2bc..56e99b2 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7244,7 +7244,7 @@ extern tree decay_conversion                      (tree,
>  extern tree build_class_member_access_expr      (cp_expr, tree, tree, bool,
>                                                  tsubst_flags_t);
>  extern tree finish_class_member_access_expr     (cp_expr, tree, bool,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t, location_t);

Maybe give the new parameter a default argument of UNKNOWN_LOCATION
rather than add it to callers?

>  extern tree build_x_arrow                      (location_t, tree,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t, location_t);
...
>  static tree cp_parser_postfix_dot_deref_expression
> -  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t);
> +  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t,
> +   location_t);

Do we really want to pass two different locations into these
functions?  Currently what we're passing is just the location of the
first token of the expression argument, so it seems redundant; perhaps
we could stick with one location parameter and pass the operator
location instead.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 055f2bc..56e99b2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7244,7 +7244,7 @@  extern tree decay_conversion			(tree,
 extern tree build_class_member_access_expr      (cp_expr, tree, tree, bool,
 						 tsubst_flags_t);
 extern tree finish_class_member_access_expr     (cp_expr, tree, bool,
-						 tsubst_flags_t);
+						 tsubst_flags_t, location_t);
 extern tree build_x_indirect_ref		(location_t, tree,
 						 ref_operator, tsubst_flags_t);
 extern tree cp_build_indirect_ref		(tree, ref_operator,
@@ -7404,7 +7404,7 @@  extern tree digest_init_flags			(tree, tree, int, tsubst_flags_t);
 extern tree digest_nsdmi_init		        (tree, tree, tsubst_flags_t);
 extern tree build_scoped_ref			(tree, tree, tree *);
 extern tree build_x_arrow			(location_t, tree,
-						 tsubst_flags_t);
+						 tsubst_flags_t, location_t);
 extern tree build_m_component_ref		(tree, tree, tsubst_flags_t);
 extern tree build_functional_cast		(tree, tree, tsubst_flags_t);
 extern tree add_exception_specifier		(tree, tree, int);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d8d4301..e869c8e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -2045,7 +2045,8 @@  static cp_expr cp_parser_postfix_expression
 static tree cp_parser_postfix_open_square_expression
   (cp_parser *, tree, bool, bool);
 static tree cp_parser_postfix_dot_deref_expression
-  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t);
+  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t,
+   location_t);
 static vec<tree, va_gc> *cp_parser_parenthesized_expression_list
   (cp_parser *, int, bool, bool, bool *, location_t * = NULL,
    bool = false);
@@ -7281,16 +7282,20 @@  cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	     postfix-expression . pseudo-destructor-name
 	     postfix-expression -> template [opt] id-expression
 	     postfix-expression -> pseudo-destructor-name */
+	  {
+	    location_t loc_dot_or_deref = token->location;
 
-	  /* Consume the `.' or `->' operator.  */
-	  cp_lexer_consume_token (parser->lexer);
+	    /* Consume the `.' or `->' operator.  */
+	    cp_lexer_consume_token (parser->lexer);
 
-	  postfix_expression
-	    = cp_parser_postfix_dot_deref_expression (parser, token->type,
-						      postfix_expression,
-						      false, &idk, loc);
+	    postfix_expression
+	      = cp_parser_postfix_dot_deref_expression (parser, token->type,
+							postfix_expression,
+							false, &idk, loc,
+							loc_dot_or_deref);
 
-          is_member_access = true;
+	    is_member_access = true;
+	  }
 	  break;
 
 	case CPP_PLUS_PLUS:
@@ -7478,7 +7483,8 @@  cp_parser_postfix_dot_deref_expression (cp_parser *parser,
 					enum cpp_ttype token_type,
 					cp_expr postfix_expression,
 					bool for_offsetof, cp_id_kind *idk,
-					location_t location)
+					location_t location,
+					location_t loc_dot_or_deref)
 {
   tree name;
   bool dependent_p;
@@ -7489,7 +7495,8 @@  cp_parser_postfix_dot_deref_expression (cp_parser *parser,
   /* If this is a `->' operator, dereference the pointer.  */
   if (token_type == CPP_DEREF)
     postfix_expression = build_x_arrow (location, postfix_expression,
-					tf_warning_or_error);
+					tf_warning_or_error,
+					loc_dot_or_deref);
   /* Check to see whether or not the expression is type-dependent and
      not the current instantiation.  */
   dependent_p = type_dependent_object_expression_p (postfix_expression);
@@ -7641,7 +7648,8 @@  cp_parser_postfix_dot_deref_expression (cp_parser *parser,
 	  postfix_expression
 	    = finish_class_member_access_expr (postfix_expression, name,
 					       template_p, 
-					       tf_warning_or_error);
+					       tf_warning_or_error,
+					       loc_dot_or_deref);
 	  /* Build a location e.g.:
 	       ptr->access_expr
 	       ~~~^~~~~~~~~~~~~
@@ -9864,7 +9872,8 @@  cp_parser_builtin_offsetof (cp_parser *parser)
 
   /* Parse the offsetof-member-designator.  We begin as if we saw "expr->".  */
   expr = cp_parser_postfix_dot_deref_expression (parser, CPP_DEREF, object_ptr,
-						 true, &dummy, token->location);
+						 true, &dummy, token->location,
+						 UNKNOWN_LOCATION);
   while (true)
     {
       token = cp_lexer_peek_token (parser->lexer);
@@ -9887,6 +9896,7 @@  cp_parser_builtin_offsetof (cp_parser *parser)
 	  cp_lexer_consume_token (parser->lexer);
 	  expr = cp_parser_postfix_dot_deref_expression (parser, CPP_DOT,
 							 expr, true, &dummy,
+							 token->location,
 							 token->location);
 	  break;
 
@@ -12239,7 +12249,8 @@  cp_parser_range_for_member_function (tree range, tree identifier)
   vec<tree, va_gc> *vec;
 
   member = finish_class_member_access_expr (range, identifier,
-					    false, tf_warning_or_error);
+					    false, tf_warning_or_error,
+					    UNKNOWN_LOCATION);
   if (member == error_mark_node)
     return error_mark_node;
 
@@ -31676,7 +31687,7 @@  cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
 		  decl
 		    = cp_parser_postfix_dot_deref_expression (parser, CPP_DOT,
 							      decl, false,
-							      &idk, loc);
+							      &idk, loc, loc);
 		}
 	      /* FALLTHROUGH.  */
 	    case OMP_CLAUSE_DEPEND:
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index a7266e3..a3d17a1 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -18164,7 +18164,7 @@  tsubst_copy_and_build (tree t,
       if (DECL_P (op1)
 	  && !mark_used (op1, complain) && !(complain & tf_error))
 	RETURN (error_mark_node);
-      RETURN (build_x_arrow (input_location, op1, complain));
+      RETURN (build_x_arrow (input_location, op1, complain, UNKNOWN_LOCATION));
 
     case NEW_EXPR:
       {
@@ -18781,7 +18781,7 @@  tsubst_copy_and_build (tree t,
 
 	r = finish_class_member_access_expr (object, member,
 					     /*template_p=*/false,
-					     complain);
+					     complain, UNKNOWN_LOCATION);
 	if (TREE_CODE (r) == COMPONENT_REF)
 	  REF_PARENTHESIZED_P (r) = REF_PARENTHESIZED_P (t);
 	RETURN (r);
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index bfdca50..7edcfde 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -3812,7 +3812,8 @@  finish_id_expression (tree id_expression,
 	      decl = maybe_dummy_object (DECL_CONTEXT (first_fn), 0);
 	      return finish_class_member_access_expr (decl, id_expression,
 						      /*template_p=*/false,
-						      tf_warning_or_error);
+						      tf_warning_or_error,
+						      UNKNOWN_LOCATION);
 	    }
 
 	  decl = baselink_for_fns (decl);
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 122d9dc..f367e53 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2756,7 +2756,8 @@  access_failure_info::maybe_suggest_accessor (bool const_p) const
 
 tree
 finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
-				 tsubst_flags_t complain)
+				 tsubst_flags_t complain,
+				 location_t loc_dot_or_deref)
 {
   tree expr;
   tree object_type;
@@ -2813,9 +2814,20 @@  finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
 	{
 	  if (INDIRECT_TYPE_P (object_type)
 	      && CLASS_TYPE_P (TREE_TYPE (object_type)))
-	    error ("request for member %qD in %qE, which is of pointer "
-		   "type %qT (maybe you meant to use %<->%> ?)",
-		   name, object.get_value (), object_type);
+	    {
+	      location_t loc;
+	      if (loc_dot_or_deref != UNKNOWN_LOCATION)
+		loc = loc_dot_or_deref;
+	      else
+		loc = input_location;
+	      gcc_rich_location richloc (loc);
+	      if (loc_dot_or_deref != UNKNOWN_LOCATION)
+		richloc.add_fixit_replace ("->");
+	      error_at (&richloc,
+			"request for member %qD in %qE, which is of pointer "
+			"type %qT (maybe you meant to use %<->%> ?)",
+			name, object.get_value (), object_type);
+	    }
 	  else
 	    error ("request for member %qD in %qE, which is of non-class "
 		   "type %qT", name, object.get_value (), object_type);
diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index 1e899ab..3611a27 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -32,6 +32,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "stor-layout.h"
 #include "varasm.h"
 #include "intl.h"
+#include "gcc-rich-location.h"
 
 static tree
 process_init_constructor (tree type, tree init, int nested,
@@ -1839,7 +1840,8 @@  build_scoped_ref (tree datum, tree basetype, tree* binfo_p)
    delegation is detected.  */
 
 tree
-build_x_arrow (location_t loc, tree expr, tsubst_flags_t complain)
+build_x_arrow (location_t loc, tree expr, tsubst_flags_t complain,
+	       location_t loc_deref)
 {
   tree orig_expr = expr;
   tree type = TREE_TYPE (expr);
@@ -1897,7 +1899,14 @@  build_x_arrow (location_t loc, tree expr, tsubst_flags_t complain)
       if (last_rval == NULL_TREE)
 	{
 	  if (complain & tf_error)
-	    error ("base operand of %<->%> has non-pointer type %qT", type);
+	    {
+	      gcc_rich_location richloc (loc);
+	      if (loc_deref != UNKNOWN_LOCATION)
+		richloc.add_fixit_replace (loc_deref, ".");
+	      error_at (&richloc,
+			"base operand of %<->%> has non-pointer type %qT",
+			type);
+	    }
 	  return error_mark_node;
 	}
 
diff --git a/gcc/objc/objc-act.c b/gcc/objc/objc-act.c
index d086930..aec87fe 100644
--- a/gcc/objc/objc-act.c
+++ b/gcc/objc/objc-act.c
@@ -2652,7 +2652,8 @@  objc_build_component_ref (tree datum, tree component)
      a worthy substitute.  */
 #ifdef OBJCPLUS
   return finish_class_member_access_expr (datum, component, false,
-                                          tf_warning_or_error);
+                                          tf_warning_or_error,
+					  UNKNOWN_LOCATION);
 #else
   return build_component_ref (input_location, datum, component,
 			      UNKNOWN_LOCATION);
diff --git a/gcc/testsuite/g++.dg/diagnostic/fixits.C b/gcc/testsuite/g++.dg/diagnostic/fixits.C
new file mode 100644
index 0000000..3e01d56
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/fixits.C
@@ -0,0 +1,83 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+struct foo { int field; };
+union u { int field; };
+
+/* Verify that we issue a hint for "." used with a ptr to a struct.  */
+
+int test_1 (struct foo *ptr)
+{
+  return ptr.field; /* { dg-error "maybe you meant to use '->'" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.field;
+             ^
+             ->
+   { dg-end-multiline-output "" } */
+}
+
+/* Likewise for a ptr to a union.  */
+
+int test_2 (union u *ptr)
+{
+  return ptr.field; /* { dg-error "maybe you meant to use '->'" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.field;
+             ^
+             ->
+   { dg-end-multiline-output "" } */
+}
+
+/* Verify that we don't issue a hint for a ptr to something that isn't a
+   struct or union.  */
+
+int test_3 (void **ptr)
+{
+  return ptr.field; /* { dg-error "which is of non-class type" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.field;
+              ^~~~~
+   { dg-end-multiline-output "" } */
+}
+
+int test_4 ()
+{
+  struct foo val;
+  return val->field; /* { dg-error "has non-pointer type" } */
+/* { dg-begin-multiline-output "" }
+   return val->field;
+          ^~~
+             --
+             .
+   { dg-end-multiline-output "" } */
+}
+
+/* Likewise for a ptr to a union.  */
+
+int test_5 ()
+{
+  union u val;
+
+  return val->field; /* { dg-error "has non-pointer type" } */
+/* { dg-begin-multiline-output "" }
+   return val->field;
+          ^~~
+             --
+             .
+   { dg-end-multiline-output "" } */
+}
+
+struct nested
+{
+  struct foo *indirect;
+};
+
+int test_6 ()
+{
+  return __builtin_offsetof (nested, indirect.field); /* { dg-error "maybe you meant to use '->'" } */
+/* { dg-begin-multiline-output "" }
+   return __builtin_offsetof (nested, indirect.field);
+                                              ^
+                                              ->
+   { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/g++.dg/parse/error20.C b/gcc/testsuite/g++.dg/parse/error20.C
index 6119df9..18e981aa 100644
--- a/gcc/testsuite/g++.dg/parse/error20.C
+++ b/gcc/testsuite/g++.dg/parse/error20.C
@@ -12,7 +12,7 @@  struct C {
 };
 int main() {
   C c;
-  A(c.p.i); // { dg-error "9:request for member 'i' in 'c.C::p', which is of pointer type 'B" }
+  A(c.p.i); // { dg-error "8:request for member 'i' in 'c.C::p', which is of pointer type 'B" }
   return 0;
 }
 
diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc
index 1034147..be5e4a2 100644
--- a/libcc1/libcp1plugin.cc
+++ b/libcc1/libcp1plugin.cc
@@ -2940,12 +2940,13 @@  plugin_build_binary_expr (cc1_plugin::connection *self,
   switch (opcode)
     {
     case INDIRECT_REF: // This is actually a "->".
-      op0 = build_x_arrow (/*loc=*/0, op0, tf_error);
+      op0 = build_x_arrow (/*loc=*/0, op0, tf_error, UNKNOWN_LOCATION);
       /* Fall through.  */
     case COMPONENT_REF:
       result = finish_class_member_access_expr (op0, op1,
 						/*template_p=*/false,
-						tf_error);
+						tf_error,
+						UNKNOWN_LOCATION);
       break;
 
     default: