Bug 87386 - [8/9 Regression] Error message for static_assert show wrong range
Summary: [8/9 Regression] Error message for static_assert show wrong range
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: 9.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2018-09-21 19:46 UTC by trashyankes
Modified: 2018-11-27 14:38 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 7.3.0
Known to fail:
Last reconfirmed: 2018-09-21 00:00:00


Attachments
gcc9-pr87386.patch (2.68 KB, patch)
2018-11-20 18:27 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description trashyankes 2018-09-21 19:46:18 UTC
Compiler output:

```
24 | static_assert(std::is_same<int, long>::value,"eee");
   |               ^~~
```

Range mark only `std` instead of whole expression. At least is should mark `value`.
Comment 1 trashyankes 2018-09-21 19:51:03 UTC
btw how reduce "Importance" of this bug?
Right now it have same level as bug that could break my code.
Comment 2 Jonathan Wakely 2018-09-21 21:19:06 UTC
(In reply to trashyankes from comment #1)
> btw how reduce "Importance" of this bug?

You don't. That field doesn't matter.
Comment 3 Jonathan Wakely 2018-09-21 21:21:48 UTC
Proper testcase that actually compiles:

#include <type_traits>
static_assert(std::is_same<int, long>::value,"eee");

87386.cc:2:15: error: static assertion failed: eee
2 | static_assert(std::is_same<int, long>::value,"eee");
  |           



Reduced testcase without header dependencies:

namespace foo {
  template<typename> struct test { static constexpr bool value = false; };
}
static_assert(foo::test<int>::value, "eee");

87386.cc:4:15: error: static assertion failed: eee
4 | static_assert(foo::test<int>::value, "eee");
  |               ^~~


This is a regression, as GCC 7 highlighted the keyword 'static_assert' instead, which makes more sense than just the first token of the condition:

87386.cc:4:1: error: static assertion failed: eee
 static_assert(foo::test<int>::value, "eee");
 ^~~~~~~~~~~~~
Comment 4 Jonathan Wakely 2018-09-21 21:31:21 UTC
Started with r255255

    C++: improve location of static_assert errors
    
    gcc/cp/ChangeLog:
            * parser.c (cp_parser_unary_expression): Generate a location for
            "noexcept".
            (cp_parser_trait_expr): Generate and return a location_t,
            converting the return type from tree to cp_expr.
            (cp_parser_static_assert): Pass location of the condition to
            finish_static_assert, rather than that of the "static_assert"
            token, where available.
Comment 5 Jonathan Wakely 2018-09-21 21:34:56 UTC
It would be better to highlight the whole condition:

87386.cc:4:15: error: static assertion failed: eee
4 | static_assert(foo::test<int>::value, "eee");
  |               ^~~~~~~~~~~~~~~~~~~~~


Which is what already happens for a more complex expression:

namespace foo {
  template<typename> struct test { static constexpr bool value = false; };
}
static_assert(foo::test<int>::value && true, "");


87386.cc:4:37: error: static assertion failed
4 | static_assert(foo::test<int>::value && true, "");
  |               ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
Comment 6 Jakub Jelinek 2018-11-20 16:23:28 UTC
With:
--- gcc/cp/parser.c.jj	2018-11-20 08:41:28.686923718 +0100
+++ gcc/cp/parser.c	2018-11-20 17:06:27.942626189 +0100
@@ -5602,7 +5602,7 @@ cp_parser_primary_expression (cp_parser
 					  /*is_namespace=*/false,
 					  /*check_dependency=*/true,
 					  &ambiguous_decls,
-					  id_expr_token->location);
+					  id_expression.get_location ());
 	    /* If the lookup was ambiguous, an error will already have
 	       been issued.  */
 	    if (ambiguous_decls)
@@ -5673,7 +5673,7 @@ cp_parser_primary_expression (cp_parser
 	    if (parser->local_variables_forbidden_p
 		&& local_variable_p (decl))
 	      {
-		error_at (id_expr_token->location,
+		error_at (id_expression.get_location (),
 			  "local variable %qD may not appear in this context",
 			  decl.get_value ());
 		return error_mark_node;
@@ -5692,7 +5692,7 @@ cp_parser_primary_expression (cp_parser
 		 id_expression.get_location ()));
 	if (error_msg)
 	  cp_parser_error (parser, error_msg);
-	decl.set_location (id_expr_token->location);
+	decl.set_location (id_expression.get_location ());
 	return decl;
       }
 
@@ -5758,6 +5758,7 @@ cp_parser_id_expression (cp_parser *pars
 {
   bool global_scope_p;
   bool nested_name_specifier_p;
+  location_t start_loc = cp_lexer_peek_token (parser->lexer)->location;
 
   /* Assume the `template' keyword was not used.  */
   if (template_p)
@@ -5809,6 +5810,9 @@ cp_parser_id_expression (cp_parser *pars
       parser->object_scope = saved_object_scope;
       parser->qualifying_scope = saved_qualifying_scope;
 
+      location_t loc = make_location (unqualified_id.get_start (), start_loc,
+				      unqualified_id.get_finish ());
+      unqualified_id.set_location (loc);
       return unqualified_id;
     }
   /* Otherwise, if we are in global scope, then we are looking at one

the output is:
pr87386.C:4:31: error: static assertion failed: foo
    4 | static_assert(foo::test<int>::value, "foo");
      |               ~~~~~~~~~~~~~~~~^~~~~
pr87386.C:5:37: error: static assertion failed: bar
    5 | static_assert(foo::test<int>::value && true, "bar");
      |               ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~

Is that what we want?  I.e. put the care at the start of the identifier in the id-expression and use range of the whole id-expression?

We'd need to adjust probably
g++.dg/cpp0x/auto52.C
g++.dg/cpp0x/pr51420.C
g++.dg/cpp0x/udlit-declare-neg.C
g++.dg/lookup/suggestions2.C
g++.dg/parse/error17.C
g++.dg/spellcheck-pr77829.C
g++.dg/spellcheck-pr78656.C
g++.dg/spellcheck-pr79298.C
g++.dg/spellcheck-single-vs-multiple.C
testcases, so before I do that I'd like to get agreement if this is the right thing to do.  David?
Comment 7 Jakub Jelinek 2018-11-20 18:27:28 UTC
Created attachment 45046 [details]
gcc9-pr87386.patch

More complete patch.  Though, in some cases it might be better to just use range of just the last identifier of an id-expression, and the suggestions look weird too.
  foo::bar
  ~~~~~^~~
  baz
instead of
       baz
David?
Comment 8 Jakub Jelinek 2018-11-21 22:41:41 UTC
Author: jakub
Date: Wed Nov 21 22:41:07 2018
New Revision: 266359

URL: https://gcc.gnu.org/viewcvs?rev=266359&root=gcc&view=rev
Log:
	PR c++/87386
	* parser.c (cp_parser_primary_expression): Use
	id_expression.get_location () instead of id_expr_token->location.
	Adjust the range from id_expr_token->location to
	id_expressio.get_finish ().
	(cp_parser_operator_function_id): Pass location of the operator
	token down to cp_parser_operator.
	(cp_parser_operator): Add start_loc argument, always construct a
	location with caret at start_loc and range from start_loc to the
	finish of the last token.
gcc/testsuite/
	* g++.dg/diagnostic/pr87386.C: New test.
	* g++.dg/parse/error17.C: Adjust expected diagnostics.
libstdc++-v3/
	* testsuite/20_util/scoped_allocator/69293_neg.cc: Adjust expected
	line.
	* testsuite/20_util/uses_allocator/cons_neg.cc: Likewise.
	* testsuite/20_util/uses_allocator/69293_neg.cc: Likewise.
	* testsuite/experimental/propagate_const/requirements2.cc: Likewise.
	* testsuite/experimental/propagate_const/requirements3.cc: Likewise.
	* testsuite/experimental/propagate_const/requirements4.cc: Likewise.
	* testsuite/experimental/propagate_const/requirements5.cc: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/diagnostic/pr87386.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/parser.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/parse/error17.C
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc
    trunk/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc
    trunk/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc
    trunk/libstdc++-v3/testsuite/experimental/propagate_const/requirements2.cc
    trunk/libstdc++-v3/testsuite/experimental/propagate_const/requirements3.cc
    trunk/libstdc++-v3/testsuite/experimental/propagate_const/requirements4.cc
    trunk/libstdc++-v3/testsuite/experimental/propagate_const/requirements5.cc
Comment 9 Jakub Jelinek 2018-11-22 09:27:07 UTC
Author: jakub
Date: Thu Nov 22 09:26:29 2018
New Revision: 266369

URL: https://gcc.gnu.org/viewcvs?rev=266369&root=gcc&view=rev
Log:
	PR c++/87386
	* parser.c (cp_parser_operator): Use str.get_value () instead of just
	str in USERDEF_LITERAL_VALUE and USERDEF_LITERAL_SUFFIX_ID arguments.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/parser.c
Comment 10 Jakub Jelinek 2018-11-27 14:38:31 UTC
Fixed.
Comment 11 Jakub Jelinek 2018-11-27 14:38:47 UTC
Not going to backport for 8.x.