[PATCH] Fix location of typeid() (PR c++/80014)

[PATCH] Fix location of typeid() (PR c++/80014)

[David Malcolm]

PR c++/80014 reports an issue where no caret is printed when issuing an
error for this bogus code:
  !typeid(void);

Root cause is that we're not setting up the location for the cp_expr for
the typeid expression, so that
  !typeid(void)
has start == caret at the '!', but finish == UNKNOWN_LOCATION, and so
the layout ctor within diagnostic-show-locus.c filters out the primary
range, and hence layout::should_print_annotation_line_p returns false.

This patch fixes things by setting up a location for the typeid
expression when parsing it.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk now, or should this wait until stage 1?

gcc/cp/ChangeLog:
	PR c++/80014
	* parser.c (cp_parser_postfix_expression): Construct a location
	for typeid expressions.

gcc/testsuite/ChangeLog:
	PR c++/80014
	* g++.dg/plugin/diagnostic-test-expressions-1.C (std::type_info):
	Add declaration.
	(test_typeid): New test function.
---
 gcc/cp/parser.c                                    | 17 ++++++++++++--
 .../g++.dg/plugin/diagnostic-test-expressions-1.C  | 26 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index c1b6496..aecf9a5 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6513,7 +6513,10 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	/* Look for the `)' token.  Otherwise, we can't be sure that
 	   we're not looking at an expression: consider `typeid (int
 	   (3))', for example.  */
-	cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
+	cp_token *close_paren = cp_parser_require (parser, CPP_CLOSE_PAREN,
+						   RT_CLOSE_PAREN);
+	location_t end_loc = close_paren ?
+	  close_paren->location : UNKNOWN_LOCATION;
 	/* If all went well, simply lookup the type-id.  */
 	if (cp_parser_parse_definitely (parser))
 	  postfix_expression = get_typeid (type, tf_warning_or_error);
@@ -6527,13 +6530,23 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	    /* Compute its typeid.  */
 	    postfix_expression = build_typeid (expression, tf_warning_or_error);
 	    /* Look for the `)' token.  */
-	    cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
+	    close_paren
+	      = cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
+	    end_loc = close_paren ? close_paren->location : UNKNOWN_LOCATION;
 	  }
 	/* Restore the saved message.  */
 	parser->type_definition_forbidden_message = saved_message;
 	/* `typeid' may not appear in an integral constant expression.  */
 	if (cp_parser_non_integral_constant_expression (parser, NIC_TYPEID))
 	  postfix_expression = error_mark_node;
+
+	/* Construct a location e.g. :
+	     typeid (expr)
+	     ^~~~~~~~~~~~~
+	   ranging from the start of the "typeid" token to the final closing
+	   paren, with the caret at the start.  */
+	location_t typeid_loc = make_location (start_loc, start_loc, end_loc);
+	postfix_expression.set_location (typeid_loc);
       }
       break;
 
diff --git a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
index 4d3b07c..3554086 100644
--- a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
+++ b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
@@ -782,3 +782,29 @@ tests::test_method_calls ()
                                ~~~~~~~~~~~~~~~~~~^~
    { dg-end-multiline-output "" } */
 }
+
+namespace std
+{
+  class type_info { public: int foo; };
+}
+
+void test_typeid (int i)
+{
+  __emit_expression_range (0, &typeid(i)); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, &typeid(i));
+                               ^~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+  __emit_expression_range (0, &typeid(i * 2)); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, &typeid(i * 2));
+                               ^~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+  __emit_expression_range (0, typeid(int).foo); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, typeid(int).foo);
+                               ~~~~~~~~~~~~^~~
+   { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

Re: [PATCH] Fix location of typeid() (PR c++/80014)

[Jason Merrill]

On Tue, Mar 14, 2017 at 9:05 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> OK for trunk now, or should this wait until stage 1?

Stage 1.

> +       cp_token *close_paren = cp_parser_require (parser, CPP_CLOSE_PAREN,
> +                                                  RT_CLOSE_PAREN);
> +       location_t end_loc = close_paren ?
> +         close_paren->location : UNKNOWN_LOCATION;
>         /* If all went well, simply lookup the type-id.  */
>         if (cp_parser_parse_definitely (parser))
>           postfix_expression = get_typeid (type, tf_warning_or_error);
> @@ -6527,13 +6530,23 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>             /* Compute its typeid.  */
>             postfix_expression = build_typeid (expression, tf_warning_or_error);
>             /* Look for the `)' token.  */
> -           cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
> +           close_paren
> +             = cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
> +           end_loc = close_paren ? close_paren->location : UNKNOWN_LOCATION;

In both cases you're setting end_loc from close_paren, so how about
only computing it once, closer to where it's used?

Jason

[PATCH] v2: Fix location of typeid() (PR c++/80014)

[David Malcolm]

On Wed, 2017-03-15 at 16:35 -0400, Jason Merrill wrote:
> On Tue, Mar 14, 2017 at 9:05 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > OK for trunk now, or should this wait until stage 1?
> 
> Stage 1.
> 
> > +       cp_token *close_paren = cp_parser_require (parser,
> > CPP_CLOSE_PAREN,
> > +                                                  RT_CLOSE_PAREN);
> > +       location_t end_loc = close_paren ?
> > +         close_paren->location : UNKNOWN_LOCATION;
> >         /* If all went well, simply lookup the type-id.  */
> >         if (cp_parser_parse_definitely (parser))
> >           postfix_expression = get_typeid (type,
> > tf_warning_or_error);
> > @@ -6527,13 +6530,23 @@ cp_parser_postfix_expression (cp_parser
> > *parser, bool address_p, bool cast_p,
> >             /* Compute its typeid.  */
> >             postfix_expression = build_typeid (expression,
> > tf_warning_or_error);
> >             /* Look for the `)' token.  */
> > -           cp_parser_require (parser, CPP_CLOSE_PAREN,
> > RT_CLOSE_PAREN);
> > +           close_paren
> > +             = cp_parser_require (parser, CPP_CLOSE_PAREN,
> > RT_CLOSE_PAREN);
> > +           end_loc = close_paren ? close_paren->location :
> > UNKNOWN_LOCATION;
> 
> In both cases you're setting end_loc from close_paren, so how about
> only computing it once, closer to where it's used?
> 
> Jason

Here's an updated version of the patch; it only looks up the location
of the close paren once (and adds another test case).  I also replaced
the ?: with an if () to avoid setting finish to UNKNOWN_LOCATION.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for trunk?

Blurb from v1:

PR c++/80014 reports an issue where no caret is printed when issuing an
error for this bogus code:
  !typeid(void);

Root cause is that we're not setting up the location for the cp_expr for
the typeid expression, so that
  !typeid(void)
has start == caret at the '!', but finish == UNKNOWN_LOCATION, and so
the layout ctor within diagnostic-show-locus.c filters out the primary
range, and hence layout::should_print_annotation_line_p returns false.

This patch fixes things by setting up a location for the typeid
expression when parsing it.

gcc/cp/ChangeLog:
	PR c++/80014
	* parser.c (cp_parser_postfix_expression): Construct a location
	for typeid expressions.

gcc/testsuite/ChangeLog:
	PR c++/80014
	* g++.dg/plugin/diagnostic-test-expressions-1.C (std::type_info):
	Add declaration.
	(test_typeid): New test function.
---
 gcc/cp/parser.c                                    | 18 ++++++++++--
 .../g++.dg/plugin/diagnostic-test-expressions-1.C  | 32 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4adf9aa..49003e4 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6517,7 +6517,8 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	/* Look for the `)' token.  Otherwise, we can't be sure that
 	   we're not looking at an expression: consider `typeid (int
 	   (3))', for example.  */
-	cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
+	cp_token *close_paren = cp_parser_require (parser, CPP_CLOSE_PAREN,
+						   RT_CLOSE_PAREN);
 	/* If all went well, simply lookup the type-id.  */
 	if (cp_parser_parse_definitely (parser))
 	  postfix_expression = get_typeid (type, tf_warning_or_error);
@@ -6531,13 +6532,26 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	    /* Compute its typeid.  */
 	    postfix_expression = build_typeid (expression, tf_warning_or_error);
 	    /* Look for the `)' token.  */
-	    cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
+	    close_paren
+	      = cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
 	  }
 	/* Restore the saved message.  */
 	parser->type_definition_forbidden_message = saved_message;
 	/* `typeid' may not appear in an integral constant expression.  */
 	if (cp_parser_non_integral_constant_expression (parser, NIC_TYPEID))
 	  postfix_expression = error_mark_node;
+
+	/* Construct a location e.g. :
+	     typeid (expr)
+	     ^~~~~~~~~~~~~
+	   ranging from the start of the "typeid" token to the final closing
+	   paren, with the caret at the start.  */
+	if (close_paren)
+	  {
+	    location_t typeid_loc
+	      = make_location (start_loc, start_loc, close_paren->location);
+	    postfix_expression.set_location (typeid_loc);
+	  }
       }
       break;
 
diff --git a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
index 2c004f3..62d3c36 100644
--- a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
+++ b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
@@ -848,3 +848,35 @@ tests::test_method_calls ()
                                ~~~~~~~~~~~~~~~~~~^~
    { dg-end-multiline-output "" } */
 }
+
+namespace std
+{
+  class type_info { public: int foo; };
+}
+
+void test_typeid (int i)
+{
+  __emit_expression_range (0, &typeid(i)); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, &typeid(i));
+                               ^~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+  __emit_expression_range (0, &typeid(int)); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, &typeid(int));
+                               ^~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+  __emit_expression_range (0, &typeid(i * 2)); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, &typeid(i * 2));
+                               ^~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+  __emit_expression_range (0, typeid(int).foo); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, typeid(int).foo);
+                               ~~~~~~~~~~~~^~~
+   { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

Re: [PATCH] v2: Fix location of typeid() (PR c++/80014)

[Jason Merrill]

OK.

On Wed, Jun 28, 2017 at 7:13 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2017-03-15 at 16:35 -0400, Jason Merrill wrote:
>> On Tue, Mar 14, 2017 at 9:05 PM, David Malcolm <dmalcolm@redhat.com>
>> wrote:
>> > OK for trunk now, or should this wait until stage 1?
>>
>> Stage 1.
>>
>> > +       cp_token *close_paren = cp_parser_require (parser,
>> > CPP_CLOSE_PAREN,
>> > +                                                  RT_CLOSE_PAREN);
>> > +       location_t end_loc = close_paren ?
>> > +         close_paren->location : UNKNOWN_LOCATION;
>> >         /* If all went well, simply lookup the type-id.  */
>> >         if (cp_parser_parse_definitely (parser))
>> >           postfix_expression = get_typeid (type,
>> > tf_warning_or_error);
>> > @@ -6527,13 +6530,23 @@ cp_parser_postfix_expression (cp_parser
>> > *parser, bool address_p, bool cast_p,
>> >             /* Compute its typeid.  */
>> >             postfix_expression = build_typeid (expression,
>> > tf_warning_or_error);
>> >             /* Look for the `)' token.  */
>> > -           cp_parser_require (parser, CPP_CLOSE_PAREN,
>> > RT_CLOSE_PAREN);
>> > +           close_paren
>> > +             = cp_parser_require (parser, CPP_CLOSE_PAREN,
>> > RT_CLOSE_PAREN);
>> > +           end_loc = close_paren ? close_paren->location :
>> > UNKNOWN_LOCATION;
>>
>> In both cases you're setting end_loc from close_paren, so how about
>> only computing it once, closer to where it's used?
>>
>> Jason
>
> Here's an updated version of the patch; it only looks up the location
> of the close paren once (and adds another test case).  I also replaced
> the ?: with an if () to avoid setting finish to UNKNOWN_LOCATION.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> Blurb from v1:
>
> PR c++/80014 reports an issue where no caret is printed when issuing an
> error for this bogus code:
>   !typeid(void);
>
> Root cause is that we're not setting up the location for the cp_expr for
> the typeid expression, so that
>   !typeid(void)
> has start == caret at the '!', but finish == UNKNOWN_LOCATION, and so
> the layout ctor within diagnostic-show-locus.c filters out the primary
> range, and hence layout::should_print_annotation_line_p returns false.
>
> This patch fixes things by setting up a location for the typeid
> expression when parsing it.
>
> gcc/cp/ChangeLog:
>         PR c++/80014
>         * parser.c (cp_parser_postfix_expression): Construct a location
>         for typeid expressions.
>
> gcc/testsuite/ChangeLog:
>         PR c++/80014
>         * g++.dg/plugin/diagnostic-test-expressions-1.C (std::type_info):
>         Add declaration.
>         (test_typeid): New test function.
> ---
>  gcc/cp/parser.c                                    | 18 ++++++++++--
>  .../g++.dg/plugin/diagnostic-test-expressions-1.C  | 32 ++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 4adf9aa..49003e4 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -6517,7 +6517,8 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>         /* Look for the `)' token.  Otherwise, we can't be sure that
>            we're not looking at an expression: consider `typeid (int
>            (3))', for example.  */
> -       cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
> +       cp_token *close_paren = cp_parser_require (parser, CPP_CLOSE_PAREN,
> +                                                  RT_CLOSE_PAREN);
>         /* If all went well, simply lookup the type-id.  */
>         if (cp_parser_parse_definitely (parser))
>           postfix_expression = get_typeid (type, tf_warning_or_error);
> @@ -6531,13 +6532,26 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>             /* Compute its typeid.  */
>             postfix_expression = build_typeid (expression, tf_warning_or_error);
>             /* Look for the `)' token.  */
> -           cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
> +           close_paren
> +             = cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
>           }
>         /* Restore the saved message.  */
>         parser->type_definition_forbidden_message = saved_message;
>         /* `typeid' may not appear in an integral constant expression.  */
>         if (cp_parser_non_integral_constant_expression (parser, NIC_TYPEID))
>           postfix_expression = error_mark_node;
> +
> +       /* Construct a location e.g. :
> +            typeid (expr)
> +            ^~~~~~~~~~~~~
> +          ranging from the start of the "typeid" token to the final closing
> +          paren, with the caret at the start.  */
> +       if (close_paren)
> +         {
> +           location_t typeid_loc
> +             = make_location (start_loc, start_loc, close_paren->location);
> +           postfix_expression.set_location (typeid_loc);
> +         }
>        }
>        break;
>
> diff --git a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
> index 2c004f3..62d3c36 100644
> --- a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
> +++ b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C
> @@ -848,3 +848,35 @@ tests::test_method_calls ()
>                                 ~~~~~~~~~~~~~~~~~~^~
>     { dg-end-multiline-output "" } */
>  }
> +
> +namespace std
> +{
> +  class type_info { public: int foo; };
> +}
> +
> +void test_typeid (int i)
> +{
> +  __emit_expression_range (0, &typeid(i)); /* { dg-warning "range" } */
> +/* { dg-begin-multiline-output "" }
> +   __emit_expression_range (0, &typeid(i));
> +                               ^~~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +
> +  __emit_expression_range (0, &typeid(int)); /* { dg-warning "range" } */
> +/* { dg-begin-multiline-output "" }
> +   __emit_expression_range (0, &typeid(int));
> +                               ^~~~~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +
> +  __emit_expression_range (0, &typeid(i * 2)); /* { dg-warning "range" } */
> +/* { dg-begin-multiline-output "" }
> +   __emit_expression_range (0, &typeid(i * 2));
> +                               ^~~~~~~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +
> +  __emit_expression_range (0, typeid(int).foo); /* { dg-warning "range" } */
> +/* { dg-begin-multiline-output "" }
> +   __emit_expression_range (0, typeid(int).foo);
> +                               ~~~~~~~~~~~~^~~
> +   { dg-end-multiline-output "" } */
> +}
> --
> 1.8.5.3
>