diff --git a/NEWS b/NEWS index 2882c72a1..1a00a1bd3 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,34 @@ GNU Bison NEWS * Noteworthy changes in release ?.? (????-??-??) [?] +** New features + +*** New warning flag: -Wuseless-action + + A new warning category is introduced: 'useless-action', which reports + useless explicit actions. For instance on the following grammar: + + %type "number" expr term + %% + expr: expr "+" term { $$ = $1 + $3; } + | term { $$ = $1; } + term: "(" expr ")" { $$ = $2; } + | "number" { $$ = $1; } + + bison diagnoses: + + $ bison -Wuseless-action foo.y + foo.y:4.21-32: warning: useless explicit action [-Wuseless-action] + 4 | | term { $$ = $1; } + | ^~~~~~~~~~~~ + foo.y:6.21-32: warning: useless explicit action [-Wuseless-action] + 6 | | "number" { $$ = $1; } + | ^~~~~~~~~~~~ + foo.y: warning: fix-its can be applied. Rerun with option '--update'. [-Wother] + + Running 'bison -Wuseless-action --update foo.y' would remove these + actions. + * Noteworthy changes in release 3.6.90 (2020-07-04) [beta] diff --git a/doc/bison.texi b/doc/bison.texi index 84a609cf1..e991088f9 100644 --- a/doc/bison.texi +++ b/doc/bison.texi @@ -11360,6 +11360,41 @@ One would get the exact same parser with the following directives instead: @end group @end example +@item useless-action +Useless explicit actions (@samp{@{ $$ = $1; @}}) are reported. For instance +on the following grammar: + +@example +@group +$ @kbd{cat foo.y} +%type "number" expr term +%% +expr: expr "+" term @{ $$ = $1 + $3; @} + | term @{ $$ = $1; @} +term: "(" expr ")" @{ $$ = $2; @} + | "number" @{ $$ = $1; @} +@end group +@end example + +@noindent +@command{bison} diagnoses: + +@example +$ @kbd{bison -Wuseless-action foo.y} +foo.y:4.21-32: warning: useless explicit action [-Wuseless-action] + | term @{ $$ = $1; @} + ^~~~~~~~~~~~ +foo.y:6.21-32: warning: useless explicit action [-Wuseless-action] + | "number" @{ $$ = $1; @} + ^~~~~~~~~~~~ +foo.y: warning: fix-its can be applied. Rerun with option '--update'. [-Wother] +@end example + +Running @kbd{bison -Wuseless-action --update foo.y} would remove these actions. + +Actions with named references (e.g., @samp{@{ $expr = $term; @}}) are not +reported. To disable the warning locally, write @samp{@{ $$ = ($1); @}}. + @item yacc Incompatibilities with POSIX Yacc. diff --git a/examples/c/lexcalc/parse.y b/examples/c/lexcalc/parse.y index 6db00c9bf..2c5bd0063 100644 --- a/examples/c/lexcalc/parse.y +++ b/examples/c/lexcalc/parse.y @@ -90,7 +90,7 @@ exp: $$ = $1 / $3; } | "(" exp ")" { $$ = $2; } -| NUM { $$ = $1; } +| NUM ; %% // Epilogue (C code). diff --git a/examples/c/reccalc/parse.y b/examples/c/reccalc/parse.y index c11f9b665..5bfeb7682 100644 --- a/examples/c/reccalc/parse.y +++ b/examples/c/reccalc/parse.y @@ -119,7 +119,7 @@ eol: ; exp: - NUM { $$ = $1; } + NUM | exp "+" exp { $$ = $1 + $3; } | exp "-" exp { $$ = $1 - $3; } | exp "*" exp { $$ = $1 * $3; } diff --git a/examples/d/calc.y b/examples/d/calc.y index a2ae85df3..6446345e4 100644 --- a/examples/d/calc.y +++ b/examples/d/calc.y @@ -37,7 +37,7 @@ line: ; exp: - NUM { $$ = $1; } + NUM | exp "+" exp { $$ = $1 + $3; } | exp "-" exp { $$ = $1 - $3; } | exp "*" exp { $$ = $1 * $3; } diff --git a/src/complain.c b/src/complain.c index 889f0ae6f..3e8797998 100644 --- a/src/complain.c +++ b/src/complain.c @@ -137,6 +137,7 @@ static const argmatch_warning_doc argmatch_warning_docs[] = { "empty-rule", N_("empty rules without %empty") }, { "midrule-values", N_("unset or unused midrule values") }, { "precedence", N_("useless precedence and associativity") }, + { "useless-action", N_("useless explicit actions") }, { "yacc", N_("incompatibilities with POSIX Yacc") }, { "other", N_("all other warnings (enabled by default)") }, { "all", N_("all the warnings except 'counterexamples', 'dangling-alias' and 'yacc'") }, @@ -160,6 +161,7 @@ static const argmatch_warning_arg argmatch_warning_args[] = { "none", Wnone }, { "other", Wother }, { "precedence", Wprecedence }, + { "useless-action", Wuseless_action }, { "yacc", Wyacc }, { NULL, Wnone } }; diff --git a/src/complain.h b/src/complain.h index 303d0a54c..55b32a0bf 100644 --- a/src/complain.h +++ b/src/complain.h @@ -55,6 +55,7 @@ typedef enum warning_midrule_values, warning_other, warning_precedence, + warning_useless_action, warning_yacc, /**< POSIXME. */ warnings_size /**< The number of warnings. Must be last. */ @@ -116,6 +117,7 @@ typedef enum Wmidrule_values = 1 << warning_midrule_values, Wother = 1 << warning_other, Wprecedence = 1 << warning_precedence, + Wuseless_action = 1 << warning_useless_action, Wyacc = 1 << warning_yacc, complaint = 1 << 11, /**< All complaints. */ diff --git a/src/parse-gram.c b/src/parse-gram.c index 1eaaf7949..0f54b98cd 100644 --- a/src/parse-gram.c +++ b/src/parse-gram.c @@ -1,4 +1,4 @@ -/* A Bison parser, made by GNU Bison 3.6.90. */ +/* A Bison parser, made by GNU Bison 3.6.90.7-a0e3b-dirty. */ /* Bison implementation for Yacc-like parsers in C @@ -49,7 +49,7 @@ #define YYBISON 1 /* Bison version. */ -#define YYBISON_VERSION "3.6.90" +#define YYBISON_VERSION "3.6.90.7-a0e3b-dirty" /* Skeleton name. */ #define YYSKELETON_NAME "yacc.c" @@ -642,16 +642,16 @@ static const yytype_int16 yyrline[] = 0, 312, 312, 321, 322, 326, 327, 333, 337, 342, 343, 344, 345, 346, 347, 348, 353, 358, 359, 360, 361, 362, 363, 363, 364, 365, 366, 367, 368, 369, - 370, 371, 375, 376, 385, 386, 390, 401, 405, 409, - 417, 427, 428, 438, 439, 445, 458, 458, 463, 463, - 468, 472, 482, 483, 484, 485, 489, 490, 495, 496, - 500, 501, 505, 506, 507, 520, 529, 533, 537, 545, - 546, 550, 563, 564, 569, 570, 571, 589, 593, 597, - 605, 607, 612, 619, 629, 633, 637, 645, 650, 662, - 663, 669, 670, 671, 678, 678, 686, 687, 688, 693, - 696, 698, 700, 702, 704, 706, 708, 710, 712, 717, - 718, 727, 751, 752, 753, 754, 766, 768, 792, 797, - 798, 803, 811, 812 + 370, 371, 375, 376, 385, 386, 390, 399, 403, 407, + 415, 425, 426, 436, 437, 443, 456, 456, 461, 461, + 466, 470, 480, 481, 482, 483, 487, 488, 493, 494, + 498, 499, 503, 504, 505, 518, 527, 531, 535, 543, + 544, 548, 561, 562, 567, 568, 569, 587, 591, 595, + 603, 605, 610, 617, 627, 631, 635, 643, 648, 660, + 661, 667, 668, 669, 676, 676, 684, 685, 686, 691, + 694, 696, 698, 700, 702, 704, 706, 708, 710, 715, + 716, 725, 749, 750, 751, 752, 764, 766, 790, 795, + 796, 801, 809, 810 }; #endif @@ -2187,11 +2187,9 @@ YYLTYPE yylloc = yyloc_default; code_props code; code_props_symbol_action_init (&code, (yyvsp[-1].BRACED_CODE), (yylsp[-1])); code_props_translate_code (&code); - { - for (symbol_list *list = (yyvsp[0].generic_symlist); list; list = list->next) - symbol_list_code_props_set (list, (yyvsp[-2].code_props_type), &code); - symbol_list_free ((yyvsp[0].generic_symlist)); - } + for (symbol_list *list = (yyvsp[0].generic_symlist); list; list = list->next) + symbol_list_code_props_set (list, (yyvsp[-2].code_props_type), &code); + symbol_list_free ((yyvsp[0].generic_symlist)); } break; @@ -2307,10 +2305,6 @@ YYLTYPE yylloc = yyloc_default; { (yyval.yykind_74) = NULL; } break; - case 57: /* tag.opt: "" */ - { (yyval.yykind_74) = (yyvsp[0].TAG); } - break; - case 59: /* generic_symlist: generic_symlist generic_symlist_item */ { (yyval.generic_symlist) = symbol_list_append ((yyvsp[-1].generic_symlist), (yyvsp[0].generic_symlist_item)); } break; @@ -2376,10 +2370,6 @@ YYLTYPE yylloc = yyloc_default; { (yyval.alias) = NULL; } break; - case 75: /* alias: string_as_id */ - { (yyval.alias) = (yyvsp[0].string_as_id); } - break; - case 76: /* alias: "translatable string" */ { (yyval.alias) = symbol_get ((yyvsp[0].TSTRING), (yylsp[0])); diff --git a/src/parse-gram.h b/src/parse-gram.h index 739b91b6f..716fac9a0 100644 --- a/src/parse-gram.h +++ b/src/parse-gram.h @@ -1,4 +1,4 @@ -/* A Bison parser, made by GNU Bison 3.6.90. */ +/* A Bison parser, made by GNU Bison 3.6.90.7-a0e3b-dirty. */ /* Bison interface for Yacc-like parsers in C diff --git a/src/parse-gram.y b/src/parse-gram.y index fc046f60e..17a2776ca 100644 --- a/src/parse-gram.y +++ b/src/parse-gram.y @@ -387,16 +387,14 @@ grammar_declaration: { grammar_start_symbol_set ($2, @2); } -| code_props_type "{...}" generic_symlist +| code_props_type "{...}" generic_symlist[syms] { code_props code; code_props_symbol_action_init (&code, $2, @2); code_props_translate_code (&code); - { - for (symbol_list *list = $3; list; list = list->next) - symbol_list_code_props_set (list, $1, &code); - symbol_list_free ($3); - } + for (symbol_list *list = $syms; list; list = list->next) + symbol_list_code_props_set (list, $1, &code); + symbol_list_free ($syms); } | "%default-prec" { @@ -487,7 +485,7 @@ precedence_declarator: tag.opt: %empty { $$ = NULL; } -| TAG { $$ = $1; } +| TAG ; %type generic_symlist generic_symlist_item; @@ -567,7 +565,7 @@ int.opt: %type alias; alias: %empty { $$ = NULL; } -| string_as_id { $$ = $1; } +| string_as_id | TSTRING { $$ = symbol_get ($1, @1); diff --git a/src/reader.c b/src/reader.c index 2d1423316..484a89bda 100644 --- a/src/reader.c +++ b/src/reader.c @@ -300,7 +300,10 @@ grammar_rule_check_and_complete (symbol_list *r) || STREQ (skeleton, "lalr1.cc"))); if (is_cxx) { - code_props_rule_action_init (&r->action_props, "{ $$ = $1; }", + /* Parens around $1 to avoid triggering the warning + about useless explicit actions. */ + code_props_rule_action_init (&r->action_props, + "{ $$ = ($1); }", r->rhs_loc, r, /* name */ NULL, /* type */ NULL, diff --git a/src/scan-code.l b/src/scan-code.l index 86f877495..bfcc0d77c 100644 --- a/src/scan-code.l +++ b/src/scan-code.l @@ -26,6 +26,7 @@ #include #include "src/complain.h" +#include "src/fixits.h" #include "src/getargs.h" #include "src/muscle-tab.h" #include "src/reader.h" @@ -290,9 +291,9 @@ variant_add (uniqstr id, location id_loc, int symbol_index, char const *cp, char const *cp_end, bool explicit_bracketing) { char const *prefix_end = find_prefix_end (id, cp, cp_end); - if (prefix_end && - (prefix_end == cp_end || - (!explicit_bracketing && is_dot_or_dash (*prefix_end)))) + if (prefix_end + && (prefix_end == cp_end + || (!explicit_bracketing && is_dot_or_dash (*prefix_end)))) { variant *r = variant_table_grow (); r->symbol_index = symbol_index; @@ -739,6 +740,70 @@ handle_action_at (symbol_list *rule, char *text, const location *at_loc) } +/* Comparing two strings, ignoring spaces. */ +static int +strspacecmp (const char *cp1, const char* cp2) +{ + aver (cp1 && cp2); + while (*cp1 && *cp2) + { + while (c_isspace (*cp1)) + ++cp1; + while (c_isspace (*cp2)) + ++cp2; + if (*cp1 == *cp2) + { + ++cp1; + ++cp2; + } + else + break; + } + return *cp1 - *cp2; +} + +/* Whether this rule could have been left implicit. + + A more natural place for this check is + grammar_rule_check_and_complete, but it is called after the action + was translated. Maybe we should compare translated code + instead? */ + +static bool +useless_default_action (code_props *self) +{ + /* Somewhat duplicates a part of grammar_rule_check_and_complete. */ + symbol_list const *r = self->rule; + char const *lhs_type = r->content.sym->content->type_name; + if (!lhs_type) + return false; + + /* The default action is installed for any rule with a non-empty + RHS. But it feels less natural not to write the action in this + case. Besides, the real point of -Wuseless-action is to help + getting rid of useless chain rules by exhibiting more + opportunities. And this is only for rules with a single RHS + symbol (so two symbols with the LHS). */ + if (symbol_list_length (r) != 2) + return false; + + /* If the first rhs is a dummy, then we cannot simply remove the + default action that is afterwards. Something fishy is going on, + but maybe a different warning for this case? + + foo: bar { $$ = 42; } { $$ = $1; } */ + symbol const *rhs = r->next->content.sym; + if (symbol_is_dummy (rhs)) + return false; + + char const *rhs_type = rhs->content->type_name; + if (!UNIQSTR_EQ (lhs_type, rhs_type ? rhs_type : "")) + return false; + + return strspacecmp (self->code, "{ $$ = $1; }") == 0; +} + + /*-------------------------. | Initialize the scanner. | `-------------------------*/ @@ -757,6 +822,15 @@ translate_action (code_props *self, int sc_context) initialized = true; } + /* Warn about explicit default actions. */ + if (sc_context == SC_RULE_ACTION + && warning_is_enabled (Wuseless_action) + && useless_default_action (self)) + { + complain (&self->location, Wuseless_action, _("useless explicit action")); + fixits_register (&self->location, ""); + } + loc->start = loc->end = self->location.start; yy_switch_to_buffer (yy_scan_string (self->code)); char *res = code_lex (self, sc_context); diff --git a/tests/actions.at b/tests/actions.at index d90851de5..603712301 100644 --- a/tests/actions.at +++ b/tests/actions.at @@ -115,6 +115,46 @@ AT_PARSER_CHECK([input], 0, AT_CLEANUP +## ------------------------- ## +## Useless default actions. ## +## ------------------------- ## + +AT_SETUP([Useless default actions]) + +AT_BISON_OPTION_PUSHDEFS +AT_DATA_GRAMMAR([[input.y]], +[[%union { + int ival; + int ival2; +} +%type expr term fact +%type "number" +%% +expr: expr '+' term { $$ = $1; } // No warning: too many symbols. +expr: term { $$ = $1; } +term: term '*' fact { $$ = $1; } | fact { $$ = $1 ; } +// No warning: types differ. +fact: "number" { $$ = $1; } +// No warning: there is a midrule action. +fact: { $$ = 42; } { $$ = $1; } +; +%% +]]) +AT_BISON_OPTION_POPDEFS + +AT_BISON_CHECK([-Wuseless-action -fcaret input.y], [], [], +[[input.y:17.12-23: warning: useless explicit action [-Wuseless-action] + 17 | expr: term { $$ = $1; } + | ^~~~~~~~~~~~ +input.y:18.41-57: warning: useless explicit action [-Wuseless-action] + 18 | term: term '*' fact { $$ = $1; } | fact { $$ = $1 ; } + | ^~~~~~~~~~~~~~~~~ +input.y: warning: fix-its can be applied. Rerun with option '--update'. [-Wother] +]]) +AT_CLEANUP + + + ## ----------------------- ## ## Implicitly empty rule. ## ## ----------------------- ## diff --git a/tests/c++.at b/tests/c++.at index 949bdeb68..c66481624 100644 --- a/tests/c++.at +++ b/tests/c++.at @@ -418,7 +418,7 @@ list: ; item: - TEXT { $$ = $][1; } + TEXT | NUMBER { int v = $][1; if (v == 3) YYERROR; else $$ = to_string (v); } ; %% @@ -1225,19 +1225,19 @@ $1 start: list {]AT_VARIANT_IF([], [ delete $][1]; )[}; list: - item { $$ = $][1; } + item // Right recursion to load the stack. | item list { $$ = $][1; ]AT_VARIANT_IF([], [delete $][2]; )[} ; item: - 'a' { $$ = $][1; } + 'a' | 'e' { YYUSE ($$); YYUSE ($][1); error ("syntax error"); } // Not just 'E', otherwise we reduce when 'E' is the lookahead, and // then the stack is emptied, defeating the point of the test. | 'E' 'a' { YYUSE ($][1); $$ = $][2; } | 'R' { ]AT_VARIANT_IF([], [$$ = YY_NULLPTR; delete $][1]; )[YYERROR; } -| 'p' { $$ = $][1; } +| 'p' | 's' { $$ = $][1; throw std::runtime_error ("reduction"); } | 'T' { ]AT_VARIANT_IF([], [$$ = YY_NULLPTR; delete $][1]; )[YYABORT; } ]m4_if([$2], [with], @@ -1253,9 +1253,10 @@ yylex (yy::parser::semantic_type *lvalp) // 'E': syntax error, with yyerror that throws. // 'i': initial action throws. // 'l': yylex throws. - // 'R': call YYERROR in the action + // 'p': printer throws. + // 'R': call YYERROR in the action. // 's': reduction throws. - // 'T': call YYABORT in the action + // 'T': call YYABORT in the action. switch (char res = *input++) { case 'l': diff --git a/tests/glr-regression.at b/tests/glr-regression.at index fede248de..6a0a484f1 100644 --- a/tests/glr-regression.at +++ b/tests/glr-regression.at @@ -147,12 +147,10 @@ command: var: 'V' - { $$ = $1; } ; var_list: var - { $$ = $1; } | var ',' var_list { char *s = YY_CAST (char *, realloc ($1, strlen ($1) + 1 + strlen ($3) + 1)); @@ -1660,9 +1658,9 @@ AT_DATA_GRAMMAR([glr-regr18.y], } %% -sym1: sym2 %merge { $$ = $1; } ; -sym2: sym3 %merge { $$ = $1; } ; -sym3: %merge { $$ = 0; } ; +sym1: sym2 %merge { $$ = $1; }; +sym2: sym3 %merge { $$ = $1; }; +sym3: %empty %merge { $$ = 0; }; %type sym1; %type sym2; @@ -1675,11 +1673,19 @@ sym3: %merge { $$ = 0; } ; ]]) AT_BISON_OPTION_POPDEFS -AT_BISON_CHECK([[-o glr-regr18.c -rall glr-regr18.y]], 1, [], -[[glr-regr18.y:28.18-24: error: result type clash on merge function 'merge': != -glr-regr18.y:27.18-24: note: previous declaration -glr-regr18.y:29.13-19: error: result type clash on merge function 'merge': != -glr-regr18.y:28.18-24: note: previous declaration +AT_BISON_CHECK([[-fcaret -o glr-regr18.c -rall glr-regr18.y]], 1, [], +[[glr-regr18.y:28.20-26: error: result type clash on merge function 'merge': != + 28 | sym2: sym3 %merge { $$ = $1; }; + | ^~~~~~~ +glr-regr18.y:27.20-26: note: previous declaration + 27 | sym1: sym2 %merge { $$ = $1; }; + | ^~~~~~~ +glr-regr18.y:29.20-26: error: result type clash on merge function 'merge': != + 29 | sym3: %empty %merge { $$ = 0; }; + | ^~~~~~~ +glr-regr18.y:28.20-26: note: previous declaration + 28 | sym2: sym3 %merge { $$ = $1; }; + | ^~~~~~~ ]]) AT_CLEANUP diff --git a/tests/java.at b/tests/java.at index 574724461..9d264ea8d 100644 --- a/tests/java.at +++ b/tests/java.at @@ -48,7 +48,7 @@ AT_CLEANUP # AT_JAVA_POSITION_DEFINE_OLD -# ----------------------- +# --------------------------- m4_define([AT_JAVA_POSITION_DEFINE_OLD], [[class Position { public int line; diff --git a/tests/reduce.at b/tests/reduce.at index e064cebcf..30e324e69 100644 --- a/tests/reduce.at +++ b/tests/reduce.at @@ -246,7 +246,7 @@ start ; used1 - : used2 { $$ = $1; } + : used2 { $$ = ($1); } // Keep this useless action to exercize translation. ; unused diff --git a/tests/torture.at b/tests/torture.at index 3dbbed5ec..ae13f47a4 100644 --- a/tests/torture.at +++ b/tests/torture.at @@ -418,7 +418,7 @@ AT_DATA_GRAMMAR([input.y], %define parse.error verbose %token WAIT_FOR_EOF %% -exp: WAIT_FOR_EOF exp | %empty; +exp: %empty | WAIT_FOR_EOF exp; %% ]AT_YYERROR_DEFINE[ #include