Re: [PATCH] Warning about missing semicolon at end of action

[Di-an JAN]

Here's a new version of the patch that only warns and adds ';'
with default language, skeleton and no --yacc.
I added an extra warning:
        "future versions of Bison will not add the `;'"
Also I changed the heuristic to not require a semicolon after
a cpp directive (and whitespaces) instead of adding a semicolon
after a cpp directive (as if it wasn't there).

For example,
exp '+' exp:
{
#ifdef USE_GMP
mpz_add($$, $1, $3);
#else
$$ = $1 + $3
#endif
}
The old version warns and adds a semicolon after the #endif
but the new version does not warn.

Di-an Jan


diff --git a/ChangeLog b/ChangeLog
index 7b3358c..e829558 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2008-11-22  Di-an Jan  <address@hidden>
+
+       Implement the FIXME that ends an user action with a semicolon
+       if it seems necessary.
+       * src/scan-code.l (flex rules section): Flag cpp directive from
+       any `#' to the first unescaped end-of-line.  Semicolon is not
+       needed after `;', `{', '}', or cpp directives and is needed after
+        any other token (whitespaces and comments have no effect).
+       * tests/actions.at (Fix user actions without a trailing semicolon):
+       New tests with and without --yacc.
+       * tests/input.at (AT_CHECK_UNUSED_VALUES): Add semicolons to
+       to make user actions complete statements.
+       Adjust column numbers in error messages.
+       * tests/regression.at (Fix user actions without a trailing semicolon):
+       Remove.  Covered by new test.
+
 2008-11-21  Di-an Jan  <address@hidden>

        Implement no-XXX arguments for --warnings, --report, --trace.
diff --git a/src/scan-code.l b/src/scan-code.l
index e883e71..7a655fb 100644
--- a/src/scan-code.l
+++ b/src/scan-code.l
@@ -81,6 +81,19 @@ splice        (\\[ \f\t\v]*\n)*
   /* Nesting level of the current code in braces.  */
   int braces_level = 0;

+  /* Whether a semicolon is probably needed.
+     The heuristic is that a semicolon is not needed after `{', `}', `;',
+     or a C preprocessor directive, and that whitespaces and comments
+     do not affect this flag.
+     Note that `{' does not need a semicolon because of `{}'.
+     A semicolon may be needed before a cpp direcive, but don't bother.  */
+  bool need_semicolon = false;
+
+  /* Whether in a C preprocessor directive.  Don't use a start condition
+     for this because, at the end of strings and comments, we still need
+     to know whether we're in a directive.  */
+  bool in_cpp = false;
+
   /* This scanner is special: it is invoked only once, henceforth
      is expected to return only once.  This initialization is
      therefore done once per action to translate. */
@@ -135,10 +148,12 @@ splice     (\\[ \f\t\v]*\n)*
   "'" {
     STRING_GROW;
     BEGIN SC_CHARACTER;
+    need_semicolon = true;
   }
   "\"" {
     STRING_GROW;
     BEGIN SC_STRING;
+    need_semicolon = true;
   }
   "/"{splice}"*" {
     STRING_GROW;
@@ -154,46 +169,63 @@ splice     (\\[ \f\t\v]*\n)*
 {
   "$"("<"{tag}">")?(-?[0-9]+|"$")  {
     handle_action_dollar (self->rule, yytext, *loc);
+    need_semicolon = true;
   }
   "@"(-?[0-9]+|"$") {
     handle_action_at (self->rule, yytext, *loc);
+    need_semicolon = true;
   }

   "$"  {
     warn_at (*loc, _("stray `$'"));
     obstack_sgrow (&obstack_for_string, "$][");
+    need_semicolon = true;
   }
   "@"  {
     warn_at (*loc, _("stray `@'"));
     obstack_sgrow (&obstack_for_string, "@@");
+    need_semicolon = true;
+  }
+  "["  {
+    obstack_sgrow (&obstack_for_string, "@{");
+    need_semicolon = true;
+  }
+  "]"  {
+    obstack_sgrow (&obstack_for_string, "@}");
+    need_semicolon = true;
   }

-  "{"  STRING_GROW; ++braces_level;
+  ";"  STRING_GROW;                 need_semicolon = false;
+  "{"  STRING_GROW; ++braces_level; need_semicolon = false;
   "}"  {
     bool outer_brace = --braces_level == 0;

     /* As an undocumented Bison extension, append `;' before the last
        brace in braced code, so that the user code can omit trailing
        `;'.  But do not append `;' if emulating Yacc, since Yacc does
-       not append one.  Also, some output languages (like Java) do not
-       accept an extra semicolon, so don't append if the user specified
-       a skeleton or language.
-
-       FIXME: Bison should warn if a semicolon seems to be necessary
-       here, and should omit the semicolon if it seems unnecessary
-       (e.g., after ';', '{', or '}', each followed by comments or
-       white space).  Such a warning shouldn't depend on --yacc; it
-       should depend on a new --pedantic option, which would cause
-       Bison to warn if it detects an extension to POSIX.  --pedantic
-       should also diagnose other Bison extensions like %yacc.
-       Perhaps there should also be a GCC-style --pedantic-errors
-       option, so that such warnings are diagnosed as errors.  */
+       not append one.  */
     if (outer_brace && !yacc_flag && language_prio == default_prio
-        && skeleton_prio == default_prio)
-      obstack_1grow (&obstack_for_string, ';');
+        && skeleton_prio == default_prio && need_semicolon && ! in_cpp)
+      {
+       warn_at (*loc, _("a `;' might be needed at the end of action code"));
+       warn_at (*loc, _("future versions of Bison will not add the `;'"));
+       obstack_1grow (&obstack_for_string, ';');
+      }

     STRING_GROW;
+    need_semicolon = false;
   }
+
+  /* Preprocessing directives should only be recognized at the beginning
+     of lines, allowing whitespace including comments, but in C/C++,
+     `#' can only be the start of preprocessor directives or within
+     `#define' directives anyway, so don't bother with begin of line.  */
+  "#"       STRING_GROW; in_cpp = true;
+
+  {splice}  STRING_GROW;
+  [\n\r]    STRING_GROW; if (in_cpp) in_cpp = need_semicolon = false; 
+  [ \t\f]   STRING_GROW;
+  .         STRING_GROW; need_semicolon = true;
 }

 <SC_SYMBOL_ACTION>
diff --git a/tests/actions.at b/tests/actions.at
index afbc94d..32bfc34 100644
--- a/tests/actions.at
+++ b/tests/actions.at
@@ -1300,3 +1300,120 @@ AT_CLEANUP])
 AT_CHECK_ACTION_LOCATIONS([[%initial-action]])
 AT_CHECK_ACTION_LOCATIONS([[%destructor]])
 AT_CHECK_ACTION_LOCATIONS([[%printer]])
+
+
+## ----------------------------------------------- ##
+## Fix user actions without a trailing semicolon.  ##
+## ----------------------------------------------- ##
+
+AT_SETUP([[Fix user actions without a trailing semicolon]])
+
+# This feature is undocumented, but we accidentally broke it in 2.3a,
+# and there was a complaint at:
+# <http://lists.gnu.org/archive/html/bug-bison/2008-11/msg00001.html>.
+
+AT_DATA([input.y],
+[[%%
+start: test2 test1 test0 testc;
+
+test2
+: 'a' { semi;                  /* TEST:N:2 */ }
+| 'b' { if (0) {no_semi}       /* TEST:N:2 */ }
+| 'c' { if (0) {semi;}         /* TEST:N:2 */ }
+| 'd' { semi;   no_semi                /* TEST:Y:2 */ }
+| 'e' { semi(); no_semi()      /* TEST:Y:2 */ }
+| 'f' { semi[]; no_semi[]      /* TEST:Y:2 */ }
+| 'g' { semi++; no_semi++      /* TEST:Y:2 */ }
+| 'h' { {no_semi} no_semi      /* TEST:Y:2 */ }
+| 'i' { {semi;}   no_semi      /* TEST:Y:2 */ }
+;
+test1
+  : 'a' { semi;                        // TEST:N:1 ;
+} | 'b' { if (0) {no_semi}     // TEST:N:1 ;
+} | 'c' { if (0) {semi;}       // TEST:N:1 ;
+} | 'd' { semi;   no_semi      // TEST:Y:1 ;
+} | 'e' { semi(); no_semi()    // TEST:Y:1 ;
+} | 'f' { semi[]; no_semi[]    // TEST:Y:1 ;
+} | 'g' { semi++; no_semi++    // TEST:Y:1 ;
+} | 'h' { {no_semi} no_semi    // TEST:Y:1 ;
+} | 'i' { {semi;}   no_semi    // TEST:Y:1 ;
+} ;
+test0
+  : 'a' { semi;                        // TEST:N:1 {}
+} | 'b' { if (0) {no_semi}     // TEST:N:1 {}
+} | 'c' { if (0) {semi;}       // TEST:N:1 {}
+} | 'd' { semi;   no_semi      // TEST:Y:1 {}
+} | 'e' { semi(); no_semi()    // TEST:Y:1 {}
+} | 'f' { semi[]; no_semi[]    // TEST:Y:1 {}
+} | 'g' { semi++; no_semi++    // TEST:Y:1 {}
+} | 'h' { {no_semi} no_semi    // TEST:Y:1 {}
+} | 'i' { {semi;}   no_semi    // TEST:Y:1 {}
+} ;
+
+testc
+: 'a' {
+#define TEST_MACRO_N \
+[]"broken\" $ @ $$ @$ [];\
+string;"}
+| 'b' {
+no_semi
+#define TEST_MACRO_N \
+[]"broken\" $ @ $$ @$ [];\
+string;"}
+]])
+
+AT_BISON_CHECK([[-o input.c input.y]], [0], [],
+[[input.y:8.48: warning: a `;' might be needed at the end of action code
+input.y:8.48: warning: future versions of Bison will not add the `;'
+input.y:9.48: warning: a `;' might be needed at the end of action code
+input.y:9.48: warning: future versions of Bison will not add the `;'
+input.y:10.48: warning: a `;' might be needed at the end of action code
+input.y:10.48: warning: future versions of Bison will not add the `;'
+input.y:11.48: warning: a `;' might be needed at the end of action code
+input.y:11.48: warning: future versions of Bison will not add the `;'
+input.y:12.48: warning: a `;' might be needed at the end of action code
+input.y:12.48: warning: future versions of Bison will not add the `;'
+input.y:13.48: warning: a `;' might be needed at the end of action code
+input.y:13.48: warning: future versions of Bison will not add the `;'
+input.y:20.1: warning: a `;' might be needed at the end of action code
+input.y:20.1: warning: future versions of Bison will not add the `;'
+input.y:21.1: warning: a `;' might be needed at the end of action code
+input.y:21.1: warning: future versions of Bison will not add the `;'
+input.y:22.1: warning: a `;' might be needed at the end of action code
+input.y:22.1: warning: future versions of Bison will not add the `;'
+input.y:23.1: warning: a `;' might be needed at the end of action code
+input.y:23.1: warning: future versions of Bison will not add the `;'
+input.y:24.1: warning: a `;' might be needed at the end of action code
+input.y:24.1: warning: future versions of Bison will not add the `;'
+input.y:25.1: warning: a `;' might be needed at the end of action code
+input.y:25.1: warning: future versions of Bison will not add the `;'
+input.y:31.1: warning: a `;' might be needed at the end of action code
+input.y:31.1: warning: future versions of Bison will not add the `;'
+input.y:32.1: warning: a `;' might be needed at the end of action code
+input.y:32.1: warning: future versions of Bison will not add the `;'
+input.y:33.1: warning: a `;' might be needed at the end of action code
+input.y:33.1: warning: future versions of Bison will not add the `;'
+input.y:34.1: warning: a `;' might be needed at the end of action code
+input.y:34.1: warning: future versions of Bison will not add the `;'
+input.y:35.1: warning: a `;' might be needed at the end of action code
+input.y:35.1: warning: future versions of Bison will not add the `;'
+input.y:36.1: warning: a `;' might be needed at the end of action code
+input.y:36.1: warning: future versions of Bison will not add the `;'
+]])
+AT_CHECK([[grep -c '/\* TEST:N:2 \*/ }$' input.c]], [0], [[3
+]])
+AT_CHECK([[grep -c '/\* TEST:Y:2 \*/ ;}$' input.c]], [0], [[6
+]])
+AT_CHECK([[sed -n '/TEST:N:1/{N
+s/\n/<NL>/gp}' input.c | grep -c '// TEST:N:1 [;{}]*<NL>}$']], [0], [[6
+]])
+AT_CHECK([[sed -n '/TEST:Y:1/{N
+s/\n/<NL>/gp}' input.c | grep -c '// TEST:Y:1 [;{}]*<NL>;}$']], [0], [[12
+]])
+AT_CHECK([[sed -n '/^#define TEST_MACRO_N/{N
+N
+s/\n/<NL>/gp}' input.c | grep -F -xc '#define TEST_MACRO_N \<NL>[]"broken\" $ @ $$ @$ 
[];\<NL>string;"}']],
+       [0], [[2
+]])
+
+AT_CLEANUP
diff --git a/tests/input.at b/tests/input.at
index 9ce3768..daff2ec 100644
--- a/tests/input.at
+++ b/tests/input.at
@@ -92,16 +92,16 @@ m4_ifval($1, [
 ], [_AT_UNUSED_VALUES_DECLARATIONS
 ])[[%%
 start:
-  'a' a { $]2[ } | 'b' b { $]2[ } | 'c' c { $]2[ } | 'd' d { $]2[ } | 'e' e { 
$]2[ }
-| 'f' f { $]2[ } | 'g' g { $]2[ } | 'h' h { $]2[ } | 'i' i { $]2[ } | 'j' j { 
$]2[ }
-| 'k' k { $]2[ } | 'l' l { $]2[ }
+  'a' a { $]2[; } | 'b' b { $]2[; } | 'c' c { $]2[; } | 'd' d { $]2[; }
+| 'e' e { $]2[; } | 'f' f { $]2[; } | 'g' g { $]2[; } | 'h' h { $]2[; }
+| 'i' i { $]2[; } | 'j' j { $]2[; } | 'k' k { $]2[; } | 'l' l { $]2[; }
 ;

 a: INT | INT { } INT { } INT { };
 b: INT | /* empty */;
-c: INT | INT { $]1[ } INT { $<integer>2 } INT { $<integer>4 };
-d: INT | INT { } INT { $]1[ } INT { $<integer>2 };
-e: INT | INT { } INT {  } INT { $]1[ };
+c: INT | INT { $]1[; } INT { $<integer>2; } INT { $<integer>4; };
+d: INT | INT { } INT { $]1[; } INT { $<integer>2; };
+e: INT | INT { } INT {  } INT { $]1[; };
 f: INT | INT { } INT {  } INT { $]$[ = $]1[ + $]3[ + $]5[; };
 g: INT | INT { $<integer>$; } INT { $<integer>$; } INT { };
 h: INT | INT { $<integer>$; } INT { $<integer>$ = $<integer>2; } INT { };
@@ -118,18 +118,18 @@ input.y:11.10-32: warning: unused value: $]1[
 input.y:11.10-32: warning: unused value: $]3[
 input.y:11.10-32: warning: unused value: $]5[
 input.y:12.9: warning: empty rule for typed nonterminal, and no action
-]]m4_ifval($2, [[[input.y:13.14-19: warning: unset value: $$
-input.y:13.25-39: warning: unset value: $$
-]]])[[input.y:13.10-59: warning: unset value: $]$[
-input.y:13.10-59: warning: unused value: $]3[
-input.y:13.10-59: warning: unused value: $]5[
+]]m4_ifval($2, [[[input.y:13.14-20: warning: unset value: $$
+input.y:13.26-41: warning: unset value: $$
+]]])[[input.y:13.10-62: warning: unset value: $]$[
+input.y:13.10-62: warning: unused value: $]3[
+input.y:13.10-62: warning: unused value: $]5[
 ]]m4_ifval($2, [[[input.y:14.14-16: warning: unset value: $$
-]]])[[input.y:14.10-47: warning: unset value: $]$[
-input.y:14.10-47: warning: unused value: $]3[
-input.y:14.10-47: warning: unused value: $]5[
-input.y:15.10-36: warning: unset value: $]$[
-input.y:15.10-36: warning: unused value: $]3[
-input.y:15.10-36: warning: unused value: $]5[
+]]])[[input.y:14.10-49: warning: unset value: $]$[
+input.y:14.10-49: warning: unused value: $]3[
+input.y:14.10-49: warning: unused value: $]5[
+input.y:15.10-37: warning: unset value: $]$[
+input.y:15.10-37: warning: unused value: $]3[
+input.y:15.10-37: warning: unused value: $]5[
 input.y:17.10-58: warning: unset value: $]$[
 input.y:17.10-58: warning: unused value: $]1[
 ]]m4_ifval($2, [[[input.y:17.10-58: warning: unused value: $]2[
diff --git a/tests/regression.at b/tests/regression.at
index 1845524..383f6fe 100644
--- a/tests/regression.at
+++ b/tests/regression.at
@@ -1247,27 +1247,3 @@ AT_COMPILE([[input]])
 AT_PARSER_CHECK([[./input]])

 AT_CLEANUP
-
-
-
-## ----------------------------------------------- ##
-## Fix user actions without a trailing semicolon.  ##
-## ----------------------------------------------- ##
-
-AT_SETUP([[Fix user actions without a trailing semicolon]])
-
-# This feature is undocumented, but we accidentally broke it in 2.3a, and there
-# was a complaint at:
-# <http://lists.gnu.org/archive/html/bug-bison/2008-11/msg00001.html>.
-
-AT_DATA([input.y],
-[[%%
-start: {asdffdsa} ;
-]])
-
-AT_BISON_CHECK([[-o input.c input.y]])
-AT_CHECK([[sed -n '/asdffdsa/s/^ *//p' input.c]], [[0]],
-[[{asdffdsa;}
-]])
-
-AT_CLEANUP