| Message ID | 61471b27-ea85-8fc2-a7e3-178547e518d6@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 84423 ("[6/7/8/9 Regression] [concepts] ICE with invalid using declaration") | expand |
On Fri, Sep 28, 2018 at 12:42 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > the primary issue here is a rather straightforward error-recovery, where, > after a sensible (but see below) error message we ICE in > get_underlying_template - called by convert_template_argument - because we > don't handle correctly error_mark_node as TREE_TYPE, as set in > grokdeclarator. While working on it I noticed that the error message emitted > by grokdeclarator seems suboptimal, because, per 10.1.3/2, we correctly > handle the alias declaration like a typedef but, I believe, we don't want to > just say *typedef* in the error message: luckily the infrastructure to be > more accurate is fully available, because we need it to set > TYPE_DECL_ALIAS_P anyway. OK, though as long as you're messing with this error you might want to improve its location as well. Jason
Hi, On 9/28/18 7:18 PM, Jason Merrill wrote: > On Fri, Sep 28, 2018 at 12:42 PM, Paolo Carlini > <paolo.carlini@oracle.com> wrote: >> Hi, >> >> the primary issue here is a rather straightforward error-recovery, where, >> after a sensible (but see below) error message we ICE in >> get_underlying_template - called by convert_template_argument - because we >> don't handle correctly error_mark_node as TREE_TYPE, as set in >> grokdeclarator. While working on it I noticed that the error message emitted >> by grokdeclarator seems suboptimal, because, per 10.1.3/2, we correctly >> handle the alias declaration like a typedef but, I believe, we don't want to >> just say *typedef* in the error message: luckily the infrastructure to be >> more accurate is fully available, because we need it to set >> TYPE_DECL_ALIAS_P anyway. > OK, though as long as you're messing with this error you might want to > improve its location as well. Thanks. About the location, you are certainly right, but doesn't seem trivial. Something we can do *now* is using declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], but that gives us the location of the keyword 'typedef' and 'using', respectively, whereas I think that we would like to have the location of 'auto' itself. I could look into that as a follow-up piece work (by the way, I'm always reluctant to embark in non trivial work having to do with locations because I suppose David is already on it?!?) Paolo. /////////////////// Index: constraint.cc =================================================================== --- constraint.cc (revision 264687) +++ constraint.cc (working copy) @@ -1164,6 +1164,9 @@ build_constraints (tree tmpl_reqs, tree decl_reqs) if (!tmpl_reqs && !decl_reqs) return NULL_TREE; + //if (tmpl_reqs == error_mark_node || decl_reqs == error_mark_node) + //return NULL_TREE; + tree_constraint_info* ci = build_constraint_info (); ci->template_reqs = tmpl_reqs; ci->declarator_reqs = decl_reqs; Index: decl.c =================================================================== --- decl.c (revision 264687) +++ decl.c (working copy) @@ -11879,6 +11879,7 @@ grokdeclarator (const cp_declarator *declarator, /* If this is declaring a typedef name, return a TYPE_DECL. */ if (typedef_p && decl_context != TYPENAME) { + bool alias_p = decl_spec_seq_has_spec_p (declspecs, ds_alias); tree decl; /* This declaration: @@ -11901,7 +11902,12 @@ grokdeclarator (const cp_declarator *declarator, if (type_uses_auto (type)) { - error ("typedef declared %<auto%>"); + if (alias_p) + error_at (declspecs->locations[ds_alias], + "%<auto%> not allowed in alias declaration"); + else + error_at (declspecs->locations[ds_typedef], + "typedef declared %<auto%>"); type = error_mark_node; } @@ -11961,7 +11967,7 @@ grokdeclarator (const cp_declarator *declarator, inlinep, friendp, raises != NULL_TREE, declspecs->locations); - if (decl_spec_seq_has_spec_p (declspecs, ds_alias)) + if (alias_p) /* Acknowledge that this was written: `using analias = atype;'. */ TYPE_DECL_ALIAS_P (decl) = 1; Index: pt.c =================================================================== --- pt.c (revision 264687) +++ pt.c (working copy) @@ -7776,7 +7776,7 @@ convert_template_argument (tree parm, tree val; int is_type, requires_type, is_tmpl_type, requires_tmpl_type; - if (parm == error_mark_node) + if (parm == error_mark_node || error_operand_p (arg)) return error_mark_node; /* Trivially convert placeholders. */
Hi again, On 9/28/18 9:15 PM, Paolo Carlini wrote: > Thanks. About the location, you are certainly right, but doesn't seem > trivial. Something we can do *now* is using > declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], > but that gives us the location of the keyword 'typedef' and 'using', > respectively, whereas I think that we would like to have the location > of 'auto' itself. I could look into that as a follow-up piece work In fact, completing the work turned out to be easy: ensure that cp_parser_alias_declaration saves the location of the defining-type-id too and then consistently use locations[ds_type_spec] in the error messages. Tested x86_64-linux. Still Ok? ;) Thanks, Paolo. /////////////////// /cp 2018-09-29 Paolo Carlini <paolo.carlini@oracle.com> PR c++/84423 * pt.c (convert_template_argument): Immediately return error_mark_node if the second argument is erroneous. * parser.c (cp_parser_alias_declaration): Save the location of the type-id too. * decl.c (grokdeclarator): Improve error message for 'auto' in alias declaration. /testsuite 2018-09-29 Paolo Carlini <paolo.carlini@oracle.com> PR c++/84423 * g++.dg/concepts/pr84423.C: New. Index: cp/decl.c =================================================================== --- cp/decl.c (revision 264687) +++ cp/decl.c (working copy) @@ -11879,6 +11879,7 @@ grokdeclarator (const cp_declarator *declarator, /* If this is declaring a typedef name, return a TYPE_DECL. */ if (typedef_p && decl_context != TYPENAME) { + bool alias_p = decl_spec_seq_has_spec_p (declspecs, ds_alias); tree decl; /* This declaration: @@ -11901,7 +11902,12 @@ grokdeclarator (const cp_declarator *declarator, if (type_uses_auto (type)) { - error ("typedef declared %<auto%>"); + if (alias_p) + error_at (declspecs->locations[ds_type_spec], + "%<auto%> not allowed in alias declaration"); + else + error_at (declspecs->locations[ds_type_spec], + "typedef declared %<auto%>"); type = error_mark_node; } @@ -11961,7 +11967,7 @@ grokdeclarator (const cp_declarator *declarator, inlinep, friendp, raises != NULL_TREE, declspecs->locations); - if (decl_spec_seq_has_spec_p (declspecs, ds_alias)) + if (alias_p) /* Acknowledge that this was written: `using analias = atype;'. */ TYPE_DECL_ALIAS_P (decl) = 1; Index: cp/parser.c =================================================================== --- cp/parser.c (revision 264687) +++ cp/parser.c (working copy) @@ -19073,6 +19073,7 @@ cp_parser_alias_declaration (cp_parser* parser) G_("types may not be defined in alias template declarations"); } + cp_token *type_token = cp_lexer_peek_token (parser->lexer); type = cp_parser_type_id (parser); /* Restore the error message if need be. */ @@ -19107,6 +19108,9 @@ cp_parser_alias_declaration (cp_parser* parser) set_and_check_decl_spec_loc (&decl_specs, ds_alias, using_token); + set_and_check_decl_spec_loc (&decl_specs, + ds_type_spec, + type_token); if (parser->num_template_parameter_lists && !cp_parser_check_template_parameters (parser, Index: cp/pt.c =================================================================== --- cp/pt.c (revision 264687) +++ cp/pt.c (working copy) @@ -7776,7 +7776,7 @@ convert_template_argument (tree parm, tree val; int is_type, requires_type, is_tmpl_type, requires_tmpl_type; - if (parm == error_mark_node) + if (parm == error_mark_node || error_operand_p (arg)) return error_mark_node; /* Trivially convert placeholders. */ Index: testsuite/g++.dg/concepts/pr84423.C =================================================================== --- testsuite/g++.dg/concepts/pr84423.C (nonexistent) +++ testsuite/g++.dg/concepts/pr84423.C (working copy) @@ -0,0 +1,10 @@ +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fconcepts" } + +template<typename> using A = auto; // { dg-error "30:.auto. not allowed in alias declaration" } + +template<template<typename> class> struct B {}; + +B<A> b; + +typedef auto C; // { dg-error "9:typedef declared .auto." }
Hi, gently pinging the below... On 29/09/18 21:27, Paolo Carlini wrote: > Hi again, > > On 9/28/18 9:15 PM, Paolo Carlini wrote: >> Thanks. About the location, you are certainly right, but doesn't seem >> trivial. Something we can do *now* is using >> declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], >> but that gives us the location of the keyword 'typedef' and 'using', >> respectively, whereas I think that we would like to have the location >> of 'auto' itself. I could look into that as a follow-up piece work > > In fact, completing the work turned out to be easy: ensure that > cp_parser_alias_declaration saves the location of the defining-type-id > too and then consistently use locations[ds_type_spec] in the error > messages. Tested x86_64-linux. Still Ok? ;) https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01773.html Thanks! Paolo.
On Sat, Sep 29, 2018 at 3:27 PM Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi again, > > On 9/28/18 9:15 PM, Paolo Carlini wrote: > > Thanks. About the location, you are certainly right, but doesn't seem > > trivial. Something we can do *now* is using > > declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], > > but that gives us the location of the keyword 'typedef' and 'using', > > respectively, whereas I think that we would like to have the location > > of 'auto' itself. I could look into that as a follow-up piece work > > In fact, completing the work turned out to be easy: ensure that > cp_parser_alias_declaration saves the location of the defining-type-id > too and then consistently use locations[ds_type_spec] in the error > messages. Tested x86_64-linux. Still Ok? ;) Hmm, I think you need to look past any cv-qualifiers at the beginning of the type-id. Jason
Hi, On 09/10/18 17:17, Jason Merrill wrote: > On Sat, Sep 29, 2018 at 3:27 PM Paolo Carlini <paolo.carlini@oracle.com> wrote: >> Hi again, >> >> On 9/28/18 9:15 PM, Paolo Carlini wrote: >>> Thanks. About the location, you are certainly right, but doesn't seem >>> trivial. Something we can do *now* is using >>> declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], >>> but that gives us the location of the keyword 'typedef' and 'using', >>> respectively, whereas I think that we would like to have the location >>> of 'auto' itself. I could look into that as a follow-up piece work >> In fact, completing the work turned out to be easy: ensure that >> cp_parser_alias_declaration saves the location of the defining-type-id >> too and then consistently use locations[ds_type_spec] in the error >> messages. Tested x86_64-linux. Still Ok? ;) > Hmm, I think you need to look past any cv-qualifiers at the beginning > of the type-id. Ah, good point. The information is readily available in cp_parser_type_id thus we can simply return it to cp_parser_alias_declaration. I guess an alternate approach would be adding a pointer to the whole cp_decl_specifier_seq parameter to cp_parser_type_id (and cp_parser_type_id_1, etc), but that seems over killing to me. Thanks! Paolo. ///////////////////// Index: cp/decl.c =================================================================== --- cp/decl.c (revision 264985) +++ cp/decl.c (working copy) @@ -11879,6 +11879,7 @@ grokdeclarator (const cp_declarator *declarator, /* If this is declaring a typedef name, return a TYPE_DECL. */ if (typedef_p && decl_context != TYPENAME) { + bool alias_p = decl_spec_seq_has_spec_p (declspecs, ds_alias); tree decl; /* This declaration: @@ -11901,7 +11902,12 @@ grokdeclarator (const cp_declarator *declarator, if (type_uses_auto (type)) { - error ("typedef declared %<auto%>"); + if (alias_p) + error_at (declspecs->locations[ds_type_spec], + "%<auto%> not allowed in alias declaration"); + else + error_at (declspecs->locations[ds_type_spec], + "typedef declared %<auto%>"); type = error_mark_node; } @@ -11961,7 +11967,7 @@ grokdeclarator (const cp_declarator *declarator, inlinep, friendp, raises != NULL_TREE, declspecs->locations); - if (decl_spec_seq_has_spec_p (declspecs, ds_alias)) + if (alias_p) /* Acknowledge that this was written: `using analias = atype;'. */ TYPE_DECL_ALIAS_P (decl) = 1; Index: cp/parser.c =================================================================== --- cp/parser.c (revision 264985) +++ cp/parser.c (working copy) @@ -2216,12 +2216,12 @@ static tree cp_parser_late_return_type_opt static tree cp_parser_declarator_id (cp_parser *, bool); static tree cp_parser_type_id - (cp_parser *); + (cp_parser *, location_t * = NULL); static tree cp_parser_template_type_arg (cp_parser *); static tree cp_parser_trailing_type_id (cp_parser *); static tree cp_parser_type_id_1 - (cp_parser *, bool, bool); + (cp_parser *, bool, bool, location_t *); static void cp_parser_type_specifier_seq (cp_parser *, bool, bool, cp_decl_specifier_seq *); static tree cp_parser_parameter_declaration_clause @@ -19034,7 +19034,7 @@ static tree cp_parser_alias_declaration (cp_parser* parser) { tree id, type, decl, pushed_scope = NULL_TREE, attributes; - location_t id_location; + location_t id_location, type_location; cp_declarator *declarator; cp_decl_specifier_seq decl_specs; bool member_p; @@ -19086,7 +19086,7 @@ cp_parser_alias_declaration (cp_parser* parser) G_("types may not be defined in alias template declarations"); } - type = cp_parser_type_id (parser); + type = cp_parser_type_id (parser, &type_location); /* Restore the error message if need be. */ if (parser->num_template_parameter_lists) @@ -19120,6 +19120,7 @@ cp_parser_alias_declaration (cp_parser* parser) set_and_check_decl_spec_loc (&decl_specs, ds_alias, using_token); + decl_specs.locations[ds_type_spec] = type_location; if (parser->num_template_parameter_lists && !cp_parser_check_template_parameters (parser, @@ -21110,7 +21111,7 @@ cp_parser_declarator_id (cp_parser* parser, bool o static tree cp_parser_type_id_1 (cp_parser* parser, bool is_template_arg, - bool is_trailing_return) + bool is_trailing_return, location_t * type_location) { cp_decl_specifier_seq type_specifier_seq; cp_declarator *abstract_declarator; @@ -21119,6 +21120,9 @@ cp_parser_type_id_1 (cp_parser* parser, bool is_te cp_parser_type_specifier_seq (parser, /*is_declaration=*/false, is_trailing_return, &type_specifier_seq); + if (type_location) + *type_location = type_specifier_seq.locations[ds_type_spec]; + if (is_template_arg && type_specifier_seq.type && TREE_CODE (type_specifier_seq.type) == TEMPLATE_TYPE_PARM && CLASS_PLACEHOLDER_TEMPLATE (type_specifier_seq.type)) @@ -21184,9 +21188,9 @@ cp_parser_type_id_1 (cp_parser* parser, bool is_te } static tree -cp_parser_type_id (cp_parser *parser) +cp_parser_type_id (cp_parser *parser, location_t * type_location) { - return cp_parser_type_id_1 (parser, false, false); + return cp_parser_type_id_1 (parser, false, false, type_location); } static tree @@ -21196,7 +21200,7 @@ cp_parser_template_type_arg (cp_parser *parser) const char *saved_message = parser->type_definition_forbidden_message; parser->type_definition_forbidden_message = G_("types may not be defined in template arguments"); - r = cp_parser_type_id_1 (parser, true, false); + r = cp_parser_type_id_1 (parser, true, false, NULL); parser->type_definition_forbidden_message = saved_message; if (cxx_dialect >= cxx14 && !flag_concepts && type_uses_auto (r)) { @@ -21209,7 +21213,7 @@ cp_parser_template_type_arg (cp_parser *parser) static tree cp_parser_trailing_type_id (cp_parser *parser) { - return cp_parser_type_id_1 (parser, false, true); + return cp_parser_type_id_1 (parser, false, true, NULL); } /* Parse a type-specifier-seq. Index: cp/pt.c =================================================================== --- cp/pt.c (revision 264985) +++ cp/pt.c (working copy) @@ -7776,7 +7776,7 @@ convert_template_argument (tree parm, tree val; int is_type, requires_type, is_tmpl_type, requires_tmpl_type; - if (parm == error_mark_node) + if (parm == error_mark_node || error_operand_p (arg)) return error_mark_node; /* Trivially convert placeholders. */ Index: testsuite/g++.dg/concepts/pr84423-1.C =================================================================== --- testsuite/g++.dg/concepts/pr84423-1.C (nonexistent) +++ testsuite/g++.dg/concepts/pr84423-1.C (working copy) @@ -0,0 +1,8 @@ +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fconcepts" } + +template<typename> using A = auto; // { dg-error "30:.auto. not allowed in alias declaration" } + +template<template<typename> class> struct B {}; + +B<A> b; Index: testsuite/g++.dg/concepts/pr84423-2.C =================================================================== --- testsuite/g++.dg/concepts/pr84423-2.C (nonexistent) +++ testsuite/g++.dg/concepts/pr84423-2.C (working copy) @@ -0,0 +1,18 @@ +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fconcepts" } + +using A = auto; // { dg-error "11:.auto. not allowed in alias declaration" } + +using A1 = const auto; // { dg-error "18:.auto. not allowed in alias declaration" } + +using A2 = volatile auto; // { dg-error "21:.auto. not allowed in alias declaration" } + +using A3 = const volatile auto; // { dg-error "27:.auto. not allowed in alias declaration" } + +typedef auto B; // { dg-error "9:typedef declared .auto." } + +typedef const auto B1; // { dg-error "15:typedef declared .auto." } + +typedef volatile auto B2; // { dg-error "18:typedef declared .auto." } + +typedef const volatile auto B3; // { dg-error "24:typedef declared .auto." } Index: testsuite/g++.dg/cpp0x/auto39.C =================================================================== --- testsuite/g++.dg/cpp0x/auto39.C (revision 264985) +++ testsuite/g++.dg/cpp0x/auto39.C (working copy) @@ -1,6 +1,6 @@ // PR c++/58560 // { dg-do compile { target c++11 } } -typedef auto T; // { dg-error "typedef declared 'auto'" } +typedef auto T; // { dg-error "9:typedef declared 'auto'" } void foo() { T(); } Index: testsuite/g++.dg/cpp0x/auto9.C =================================================================== --- testsuite/g++.dg/cpp0x/auto9.C (revision 264985) +++ testsuite/g++.dg/cpp0x/auto9.C (working copy) @@ -120,4 +120,4 @@ void qq (auto); // { dg-error "auto" "" { void qr (auto*); // { dg-error "auto" "" { target { ! concepts } } } // PR c++/46145 -typedef auto autot; // { dg-error "auto" } +typedef auto autot; // { dg-error "9:typedef declared .auto." } Index: testsuite/g++.dg/cpp1y/pr60384.C =================================================================== --- testsuite/g++.dg/cpp1y/pr60384.C (revision 264985) +++ testsuite/g++.dg/cpp1y/pr60384.C (working copy) @@ -5,5 +5,5 @@ template<typename> int foo(); struct A { - typedef auto foo<>(); // { dg-error "typedef declared 'auto'" } + typedef auto foo<>(); // { dg-error "11:typedef declared 'auto'" } };
OK. On Tue, Oct 9, 2018 at 1:49 PM Paolo Carlini <paolo.carlini@oracle.com> wrote: > > Hi, > > On 09/10/18 17:17, Jason Merrill wrote: > > On Sat, Sep 29, 2018 at 3:27 PM Paolo Carlini <paolo.carlini@oracle.com> wrote: > >> Hi again, > >> > >> On 9/28/18 9:15 PM, Paolo Carlini wrote: > >>> Thanks. About the location, you are certainly right, but doesn't seem > >>> trivial. Something we can do *now* is using > >>> declspecs->locations[ds_typedef] and declspecs->locations[ds_alias], > >>> but that gives us the location of the keyword 'typedef' and 'using', > >>> respectively, whereas I think that we would like to have the location > >>> of 'auto' itself. I could look into that as a follow-up piece work > >> In fact, completing the work turned out to be easy: ensure that > >> cp_parser_alias_declaration saves the location of the defining-type-id > >> too and then consistently use locations[ds_type_spec] in the error > >> messages. Tested x86_64-linux. Still Ok? ;) > > Hmm, I think you need to look past any cv-qualifiers at the beginning > > of the type-id. > > Ah, good point. The information is readily available in > cp_parser_type_id thus we can simply return it to > cp_parser_alias_declaration. I guess an alternate approach would be > adding a pointer to the whole cp_decl_specifier_seq parameter to > cp_parser_type_id (and cp_parser_type_id_1, etc), but that seems over > killing to me. > > Thanks! Paolo. > > ///////////////////// >
Index: cp/decl.c =================================================================== --- cp/decl.c (revision 264687) +++ cp/decl.c (working copy) @@ -11879,6 +11879,7 @@ grokdeclarator (const cp_declarator *declarator, /* If this is declaring a typedef name, return a TYPE_DECL. */ if (typedef_p && decl_context != TYPENAME) { + bool alias_p = decl_spec_seq_has_spec_p (declspecs, ds_alias); tree decl; /* This declaration: @@ -11901,7 +11902,10 @@ grokdeclarator (const cp_declarator *declarator, if (type_uses_auto (type)) { - error ("typedef declared %<auto%>"); + if (alias_p) + error ("%<auto%> not allowed in alias declaration"); + else + error ("typedef declared %<auto%>"); type = error_mark_node; } @@ -11961,7 +11965,7 @@ grokdeclarator (const cp_declarator *declarator, inlinep, friendp, raises != NULL_TREE, declspecs->locations); - if (decl_spec_seq_has_spec_p (declspecs, ds_alias)) + if (alias_p) /* Acknowledge that this was written: `using analias = atype;'. */ TYPE_DECL_ALIAS_P (decl) = 1; Index: cp/pt.c =================================================================== --- cp/pt.c (revision 264687) +++ cp/pt.c (working copy) @@ -7776,7 +7776,7 @@ convert_template_argument (tree parm, tree val; int is_type, requires_type, is_tmpl_type, requires_tmpl_type; - if (parm == error_mark_node) + if (parm == error_mark_node || error_operand_p (arg)) return error_mark_node; /* Trivially convert placeholders. */ Index: testsuite/g++.dg/concepts/pr84423.C =================================================================== --- testsuite/g++.dg/concepts/pr84423.C (nonexistent) +++ testsuite/g++.dg/concepts/pr84423.C (working copy) @@ -0,0 +1,8 @@ +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fconcepts" } + +template<typename> using A = auto; // { dg-error "alias declaration" } + +template<template<typename> class> struct B {}; + +B<A> b;