-Wuseless-action

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 <int> "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]
 

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 <int> "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.
 

examples/c/lexcalc/parse.y

@@ -90,7 +90,7 @@ exp:
       $$ = $1 / $3;
   }
 | "(" exp ")"   { $$ = $2; }
-| NUM           { $$ = $1; }
+| NUM
 ;
 %%
 // Epilogue (C code).

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

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

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

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

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: "<tag>"  */
-         { (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]));

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
 

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 <symbol_list*> generic_symlist generic_symlist_item;
@@ -567,7 +565,7 @@ int.opt:
 %type <symbol*> alias;
 alias:
   %empty         { $$ = NULL; }
-| string_as_id   { $$ = $1; }
+| string_as_id
 | TSTRING
     {
       $$ = symbol_get ($1, @1);

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,

src/scan-code.l

@@ -26,6 +26,7 @@
 #include <quote.h>
 
 #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);