diagnostics: report useless actions

In order for the useless chain productions to work properly, there
should not be actions that make explicit the default behavior.

* src/scan-code.l (strspacecmp, useless_default_action): New.
(translate_action): Report useless actions.
* src/reader.c (grammar_rule_check_and_complete): Don't trigger this
warning when generating explicit actions for C++.
* tests/actions.at (Useless default actions): New.

Author: akimd

src/reader.c

@@ -307,7 +307,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>
@@ -289,9 +290,9 @@ variant_add (uniqstr id, location id_loc, unsigned 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;
@@ -743,6 +744,70 @@ handle_action_at (symbol_list *rule, char *text, 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.  |
 `-------------------------*/
@@ -761,6 +826,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);

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 <ival> expr term fact
+%type <ival2> "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: <ival>{ $$ = 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.  ##
 ## ----------------------- ##