[PATCH] c: Improve build_component_ref diagnostics [PR91134]

[Jakub Jelinek <jakub@redhat.com>]

Hi!

On the following testcase (the first dg-error line) we emit a weird
diagnostics and even fixit on pointerpointer->member
where pointerpointer is pointer to pointer to struct and we say
'pointerpointer' is a pointer; did you mean to use '->'?
The first part is indeed true, but suggesting -> when the code already
does use -> is confusing.
The following patch adjusts callers so that they tell it if it is from
. parsing or from -> parsing and in the latter case suggests to dereference
the left operand instead by adding (* before it and ) after it (before ->).
Or would a suggestion to add [0] before -> be better?

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-05-24  Jakub Jelinek  <jakub@redhat.com>

	PR c/91134
gcc/c/
	* c-tree.h (build_component_ref): Add ARROW_LOC location_t argument.
	* c-typeck.cc (build_component_ref): Likewise.  If DATUM is
	INDIRECT_REF and ARROW_LOC isn't UNKNOWN_LOCATION, print a different
	diagnostics and fixit hint if DATUM has pointer type.
	* c-parser.cc (c_parser_postfix_expression,
	c_parser_omp_variable_list): Adjust build_component_ref callers.
	* gimple-parser.cc (c_parser_gimple_postfix_expression_after_primary):
	Likewise.
gcc/objc/
	* objc-act.cc (objc_build_component_ref): Adjust build_component_ref
	caller.
gcc/testsuite/
	* gcc.dg/pr91134.c: New test.

--- gcc/c/c-tree.h.jj	2022-05-19 11:48:56.058291437 +0200
+++ gcc/c/c-tree.h	2022-05-23 20:22:05.669515990 +0200
@@ -699,7 +699,8 @@ extern struct c_expr convert_lvalue_to_r
 extern tree decl_constant_value_1 (tree, bool);
 extern void mark_exp_read (tree);
 extern tree composite_type (tree, tree);
-extern tree build_component_ref (location_t, tree, tree, location_t);
+extern tree build_component_ref (location_t, tree, tree, location_t,
+				 location_t);
 extern tree build_array_ref (location_t, tree, tree);
 extern tree build_external_ref (location_t, tree, bool, tree *);
 extern void pop_maybe_used (bool);
--- gcc/c/c-typeck.cc.jj	2022-05-19 11:48:56.077291176 +0200
+++ gcc/c/c-typeck.cc	2022-05-23 20:23:44.713515875 +0200
@@ -2457,11 +2457,12 @@ should_suggest_deref_p (tree datum_type)
 /* Make an expression to refer to the COMPONENT field of structure or
    union value DATUM.  COMPONENT is an IDENTIFIER_NODE.  LOC is the
    location of the COMPONENT_REF.  COMPONENT_LOC is the location
-   of COMPONENT.  */
+   of COMPONENT.  ARROW_LOC is the location of first -> operand if
+   it is from -> operator.  */
 
 tree
 build_component_ref (location_t loc, tree datum, tree component,
-		     location_t component_loc)
+		     location_t component_loc, location_t arrow_loc)
 {
   tree type = TREE_TYPE (datum);
   enum tree_code code = TREE_CODE (type);
@@ -2577,11 +2578,23 @@ build_component_ref (location_t loc, tre
       /* Special-case the error message for "ptr.field" for the case
 	 where the user has confused "." vs "->".  */
       rich_location richloc (line_table, loc);
-      /* "loc" should be the "." token.  */
-      richloc.add_fixit_replace ("->");
-      error_at (&richloc,
-		"%qE is a pointer; did you mean to use %<->%>?",
-		datum);
+      if (TREE_CODE (datum) == INDIRECT_REF && arrow_loc != UNKNOWN_LOCATION)
+	{
+	  richloc.add_fixit_insert_before (arrow_loc, "(*");
+	  richloc.add_fixit_insert_after (arrow_loc, ")");
+	  error_at (&richloc,
+		    "%qE is a pointer to pointer; did you mean to dereference "
+		    "it before applying %<->%> to it?",
+		    TREE_OPERAND (datum, 0));
+	}
+      else
+	{
+	  /* "loc" should be the "." token.  */
+	  richloc.add_fixit_replace ("->");
+	  error_at (&richloc,
+		    "%qE is a pointer; did you mean to use %<->%>?",
+		    datum);
+	}
       return error_mark_node;
     }
   else if (code != ERROR_MARK)
--- gcc/c/c-parser.cc.jj	2022-05-23 16:16:30.360856580 +0200
+++ gcc/c/c-parser.cc	2022-05-23 20:33:36.683537409 +0200
@@ -9235,8 +9235,9 @@ c_parser_postfix_expression (c_parser *p
 	    if (c_parser_next_token_is (parser, CPP_NAME))
 	      {
 		c_token *comp_tok = c_parser_peek_token (parser);
-		offsetof_ref = build_component_ref
-		  (loc, offsetof_ref, comp_tok->value, comp_tok->location);
+		offsetof_ref
+		  = build_component_ref (loc, offsetof_ref, comp_tok->value,
+					 comp_tok->location, UNKNOWN_LOCATION);
 		c_parser_consume_token (parser);
 		while (c_parser_next_token_is (parser, CPP_DOT)
 		       || c_parser_next_token_is (parser,
@@ -9263,9 +9264,11 @@ c_parser_postfix_expression (c_parser *p
 			    break;
 			  }
 			c_token *comp_tok = c_parser_peek_token (parser);
-			offsetof_ref = build_component_ref
-			  (loc, offsetof_ref, comp_tok->value,
-			   comp_tok->location);
+			offsetof_ref
+			  = build_component_ref (loc, offsetof_ref,
+						 comp_tok->value,
+						 comp_tok->location,
+						 UNKNOWN_LOCATION);
 			c_parser_consume_token (parser);
 		      }
 		    else
@@ -10612,7 +10615,7 @@ c_parser_postfix_expression_after_primar
 	  finish = c_parser_peek_token (parser)->get_finish ();
 	  c_parser_consume_token (parser);
 	  expr.value = build_component_ref (op_loc, expr.value, ident,
-					    comp_loc);
+					    comp_loc, UNKNOWN_LOCATION);
 	  set_c_expr_source_range (&expr, start, finish);
 	  expr.original_code = ERROR_MARK;
 	  if (TREE_CODE (expr.value) != COMPONENT_REF)
@@ -10652,7 +10655,8 @@ c_parser_postfix_expression_after_primar
 					    build_indirect_ref (op_loc,
 								expr.value,
 								RO_ARROW),
-					    ident, comp_loc);
+					    ident, comp_loc,
+					    expr.get_location ());
 	  set_c_expr_source_range (&expr, start, finish);
 	  expr.original_code = ERROR_MARK;
 	  if (TREE_CODE (expr.value) != COMPONENT_REF)
@@ -13171,6 +13175,7 @@ c_parser_omp_variable_list (c_parser *pa
 			 && c_parser_next_token_is (parser, CPP_DEREF)))
 		{
 		  location_t op_loc = c_parser_peek_token (parser)->location;
+		  location_t arrow_loc = UNKNOWN_LOCATION;
 		  if (c_parser_next_token_is (parser, CPP_DEREF))
 		    {
 		      c_expr t_expr;
@@ -13181,6 +13186,7 @@ c_parser_omp_variable_list (c_parser *pa
 		      t_expr = convert_lvalue_to_rvalue (op_loc, t_expr,
 							 true, false);
 		      t = build_indirect_ref (op_loc, t_expr.value, RO_ARROW);
+		      arrow_loc = t_expr.get_location ();
 		    }
 		  c_parser_consume_token (parser);
 		  if (!c_parser_next_token_is (parser, CPP_NAME))
@@ -13194,7 +13200,8 @@ c_parser_omp_variable_list (c_parser *pa
 		  tree ident = comp_tok->value;
 		  location_t comp_loc = comp_tok->location;
 		  c_parser_consume_token (parser);
-		  t = build_component_ref (op_loc, t, ident, comp_loc);
+		  t = build_component_ref (op_loc, t, ident, comp_loc,
+					   arrow_loc);
 		}
 	      /* FALLTHROUGH  */
 	    case OMP_CLAUSE_AFFINITY:
--- gcc/c/gimple-parser.cc.jj	2022-02-24 15:27:14.718744040 +0100
+++ gcc/c/gimple-parser.cc	2022-05-23 20:27:34.538195175 +0200
@@ -1800,7 +1800,7 @@ c_parser_gimple_postfix_expression_after
 	    finish = c_parser_peek_token (parser)->get_finish ();
 	    c_parser_consume_token (parser);
 	    expr.value = build_component_ref (op_loc, expr.value, ident,
-					      comp_loc);
+					      comp_loc, UNKNOWN_LOCATION);
 	    set_c_expr_source_range (&expr, start, finish);
 	    expr.original_code = ERROR_MARK;
 	    if (TREE_CODE (expr.value) != COMPONENT_REF)
@@ -1848,7 +1848,8 @@ c_parser_gimple_postfix_expression_after
 	    expr.value = build_component_ref (op_loc,
 					      build_simple_mem_ref_loc
 					        (op_loc, expr.value),
-					      ident, comp_loc);
+					      ident, comp_loc,
+					      expr.get_location ());
 	    set_c_expr_source_range (&expr, start, finish);
 	    expr.original_code = ERROR_MARK;
 	    if (TREE_CODE (expr.value) != COMPONENT_REF)
--- gcc/objc/objc-act.cc.jj	2022-01-18 00:18:02.827743339 +0100
+++ gcc/objc/objc-act.cc	2022-05-23 23:59:48.134923529 +0200
@@ -2812,7 +2812,7 @@ objc_build_component_ref (tree datum, tr
                                           tf_warning_or_error);
 #else
   return build_component_ref (input_location, datum, component,
-			      UNKNOWN_LOCATION);
+			      UNKNOWN_LOCATION, UNKNOWN_LOCATION);
 #endif
 }
 
--- gcc/testsuite/gcc.dg/pr91134.c.jj	2022-05-23 20:31:11.751001817 +0200
+++ gcc/testsuite/gcc.dg/pr91134.c	2022-05-23 20:30:45.291268997 +0200
@@ -0,0 +1,13 @@
+/* PR c/91134 */
+
+struct X { int member; } x;
+
+int
+foo (void)
+{
+  struct X *pointer = &x;
+  struct X **pointerpointer = &pointer;
+  int i = *pointerpointer->member;	/* { dg-error "'pointerpointer' is a pointer to pointer; did you mean to dereference it before applying '->' to it\\\?" } */
+  int j = pointer.member;		/* { dg-error "'pointer' is a pointer; did you mean to use '->'\\\?" } */
+  return i + j;
+}

	Jakub