Re: [PATCH] c: Improve build_component_ref diagnostics [PR91134]

[Marek Polacek <polacek@redhat.com>]

On Tue, May 24, 2022 at 09:25:57AM +0200, Jakub Jelinek wrote:
> 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.

s/diagnostic/, missing "than" before if?

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

"of the first"?

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

Consider extending the test like

--- pr91134.c	2022-05-24 09:33:17.807520253 -0400
+++ pr911342.c	2022-05-24 09:36:09.908217139 -0400
@@ -1,13 +1,16 @@
 /* PR c/91134 */

 struct X { int member; } x;
+struct Y { struct X **x; } y;

 int
 foo (void)
 {
   struct X *pointer = &x;
+  struct Y *yp = &y;
   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;
+  int k = yp->x->member; // dg-error
+  return i + j + k;
 }

but I think the patch is OK, thanks.

Marek