Re: symbol declarations after rules

[Joel E. Denny]

On Thu, 22 Jun 2006, Joel E. Denny wrote:

> I was originally thinking the error messages would be easier to implement 
> even if only as a short-term fix.  However, I'm now thinking it won't be 
> too difficult to move the rule checks to after parsing the entire grammar 
> file.  I should probably implement this before implementing the 
> %destructor/%printer extensions.

I committed the following to address this, and I have another coming.

Joel

Index: ChangeLog
===================================================================
RCS file: /sources/bison/bison/ChangeLog,v
retrieving revision 1.1516
diff -p -u -r1.1516 ChangeLog
--- ChangeLog   26 Jun 2006 03:35:27 -0000      1.1516
+++ ChangeLog   26 Jun 2006 04:08:28 -0000
@@ -1,5 +1,49 @@
 2006-06-26  Joel E. Denny  <address@hidden>
 
+       Get action warnings (grammar_rule_check) right even when symbol
+       declarations appear after the rules.  Don't mistake the type of $$ in
+       a midrule to be that of its parent rule's $$.
+       * src/reader.c (grammar_current_rule_end): Don't invoke
+       grammar_rule_check yet since not all symbol declarations may have been
+       parsed yet.
+       (grammar_midrule_action): Likewise.
+       Don't record whether the midrule's $$ has been used yet since actions
+       haven't been translated yet.
+       Record the midrule's parent rule and its RHS index within the parent
+       rule.
+       (grammar_current_rule_action_append): Don't translate the action yet
+       since not all symbol declarations may have been parsed yet and, thus,
+       warnings about types for $$, $n, @$, and @n can't be reported yet.
+       (packgram): Translate the action and invoke grammar_rule_check now that
+       all symbol declarations have been parsed.
+       * src/scan-code.l (handle_action_dollar): Now that this is invoked
+       after parsing the entire grammar file, the symbol list here in the case
+       of a midrule is actually the midrule's empty RHS, so reference its
+       parent rule's RHS where necessary.
+       On the other hand, now that you can already know it's a midrule, you
+       aren't forced to think $$ has the same type as its parent rule's $$.
+       (handle_action_at): In the case of a midrule, reference the parent rule
+       where necessary.
+       * src/symlist.c (symbol_list_new): Initialize new midrule-related
+       members.
+       (symbol_list_length): Now that this is invoked after all rules have
+       been parsed, a NULL symbol (rather than a NULL symbol list node)
+       terminates a rule.  symbol_list_print already does this correctly.
+       * src/symlist.h (symbol_list.midrule_parent_rule,
+       symbol_list.midrule_parent_rhs_index): New members so that midrules can
+       remember their relationships with their parents.
+       * tests/input.at (Type Clashes): Extend to catch the midrule $$ error
+       fixed by the above patch.
+       (_AT_UNUSED_VALUES_DECLARATIONS, AT_CHECK_UNUSED_VALUES): New m4 macros
+       implementing...
+       (Unused values): ... this old test case and...
+       (Unused values before symbol declarations): ... this new test case.
+       This one is the same as `Unused values' except that all symbol
+       declarations appear after the rules in order to catch the rest of the
+       errors fixed by the above patch.
+
+2006-06-26  Joel E. Denny  <address@hidden>
+
        More cleanup.
        * src/reader.c (current_rule): Declare it static since it's no longer
        used outside this file.
Index: src/reader.c
===================================================================
RCS file: /sources/bison/bison/src/reader.c,v
retrieving revision 1.259
diff -p -u -r1.259 reader.c
--- src/reader.c        26 Jun 2006 03:28:55 -0000      1.259
+++ src/reader.c        26 Jun 2006 04:08:29 -0000
@@ -293,7 +293,6 @@ grammar_current_rule_end (location loc)
   /* Put an empty link in the list to mark the end of this rule  */
   grammar_symbol_append (NULL, grammar_end->location);
   current_rule->location = loc;
-  grammar_rule_check (current_rule);
 }
 
 
@@ -326,10 +325,9 @@ grammar_midrule_action (void)
   midrule->action = current_rule->action;
   midrule->action_location = dummy_location;
   current_rule->action = NULL;
-  /* If $$ was used in the action, the LHS of the enclosing rule was
-     incorrectly flagged as used.  */
-  midrule->used = current_rule->used;
-  current_rule->used = false;
+  /* The action has not been translated yet, so $$ use hasn't been
+     detected yet.  */
+  midrule->used = false;
 
   if (previous_rule_end)
     previous_rule_end->next = midrule;
@@ -338,7 +336,6 @@ grammar_midrule_action (void)
 
   /* End the dummy's rule.  */
   midrule->next = symbol_list_new (NULL, dummy_location);
-  grammar_rule_check (midrule);
   midrule->next->next = current_rule;
 
   previous_rule_end = midrule->next;
@@ -347,6 +344,8 @@ grammar_midrule_action (void)
      the current rule.  Bind it to its dedicated rule.  */
   grammar_current_rule_symbol_append (dummy, dummy_location);
   grammar_end->midrule = midrule;
+  midrule->midrule_parent_rule = current_rule;
+  midrule->midrule_parent_rhs_index = symbol_list_length (current_rule->next);
 }
 
 /* Set the precedence symbol of the current rule to PRECSYM. */
@@ -405,9 +404,10 @@ grammar_current_rule_action_append (cons
 {
   if (current_rule->action)
     grammar_midrule_action ();
+  /* After all symbol declarations have been parsed, packgram invokes
+     translate_rule_action.  */
   current_rule->action = action;
   current_rule->action_location = loc;
-  current_rule->action = translate_rule_action (current_rule);
 }
 
 
@@ -444,9 +444,16 @@ packgram (void)
       rules[ruleno].precsym = NULL;
       rules[ruleno].location = p->location;
       rules[ruleno].useful = true;
-      rules[ruleno].action = p->action;
+      rules[ruleno].action = p->action ? translate_rule_action (p) : NULL;
       rules[ruleno].action_location = p->action_location;
 
+      /* If this rule contains midrules, rest assured that
+        grammar_midrule_action inserted the midrules into grammar before this
+        rule.  Thus, the midrule actions have already been scanned in order to
+        set `used' flags for this rule's rhs, so grammar_rule_check will work
+        properly.  */
+      grammar_rule_check (p);
+
       for (p = p->next; p && p->sym; p = p->next)
        {
          ++rule_length;
Index: src/scan-code.l
===================================================================
RCS file: /sources/bison/bison/src/scan-code.l,v
retrieving revision 1.6
diff -p -u -r1.6 scan-code.l
--- src/scan-code.l     26 Jun 2006 03:28:57 -0000      1.6
+++ src/scan-code.l     26 Jun 2006 04:08:29 -0000
@@ -240,7 +240,19 @@ handle_action_dollar (symbol_list *rule,
 {
   const char *type_name = NULL;
   char *cp = text + 1;
-  int rule_length = symbol_list_length (rule->next);
+  symbol_list *effective_rule;
+  int effective_rule_length;
+
+  if (rule->midrule_parent_rule)
+    {
+      effective_rule = rule->midrule_parent_rule;
+      effective_rule_length = rule->midrule_parent_rhs_index - 1;
+    }
+  else
+    {
+      effective_rule = rule;
+      effective_rule_length = symbol_list_length (rule->next);
+    }
 
   /* Get the type name if explicit. */
   if (*cp == '<')
@@ -257,8 +269,17 @@ handle_action_dollar (symbol_list *rule,
       if (!type_name)
        type_name = symbol_list_n_type_name_get (rule, dollar_loc, 0);
       if (!type_name && typed)
-       complain_at (dollar_loc, _("$$ of `%s' has no declared type"),
-                    rule->sym->tag);
+       {
+        if (rule->midrule_parent_rule)
+          complain_at (dollar_loc,
+                       _("$$ for the midrule at $%d of `%s' has no declared"
+                         " type"),
+                       rule->midrule_parent_rhs_index,
+                       effective_rule->sym->tag);
+        else
+          complain_at (dollar_loc, _("$$ of `%s' has no declared type"),
+                       rule->sym->tag);
+       }
       if (!type_name)
        type_name = "";
       obstack_fgrow1 (&obstack_for_string,
@@ -270,23 +291,23 @@ handle_action_dollar (symbol_list *rule,
       long int num;
       set_errno (0);
       num = strtol (cp, 0, 10);
-      if (INT_MIN <= num && num <= rule_length && ! get_errno ())
+      if (INT_MIN <= num && num <= effective_rule_length && ! get_errno ())
        {
          int n = num;
          if (1-n > max_left_semantic_context)
            max_left_semantic_context = 1-n;
          if (!type_name && n > 0)
            type_name =
-             symbol_list_n_type_name_get (rule, dollar_loc, n);
+             symbol_list_n_type_name_get (effective_rule, dollar_loc, n);
          if (!type_name && typed)
            complain_at (dollar_loc, _("$%d of `%s' has no declared type"),
-                        n, rule->sym->tag);
+                        n, effective_rule->sym->tag);
          if (!type_name)
            type_name = "";
          obstack_fgrow3 (&obstack_for_string,
                          "]b4_rhs_value(%d, %d, [%s])[",
-                         rule_length, n, type_name);
-         symbol_list_n_used_set (rule, n, true);
+                         effective_rule_length, n, type_name);
+         symbol_list_n_used_set (effective_rule, n, true);
        }
       else
        complain_at (dollar_loc, _("integer out of range: %s"), quote (text));
@@ -303,8 +324,13 @@ static void
 handle_action_at (symbol_list *rule, char *text, location at_loc)
 {
   char *cp = text + 1;
-  int rule_length = symbol_list_length (rule->next);
   locations_flag = true;
+  int effective_rule_length;
+
+  if (rule->midrule_parent_rule)
+    effective_rule_length = rule->midrule_parent_rhs_index - 1;
+  else
+    effective_rule_length = symbol_list_length (rule->next);
 
   if (*cp == '$')
     obstack_sgrow (&obstack_for_string, "]b4_lhs_location[");
@@ -314,11 +340,11 @@ handle_action_at (symbol_list *rule, cha
       set_errno (0);
       num = strtol (cp, 0, 10);
 
-      if (INT_MIN <= num && num <= rule_length && ! get_errno ())
+      if (INT_MIN <= num && num <= effective_rule_length && ! get_errno ())
        {
          int n = num;
          obstack_fgrow2 (&obstack_for_string, "]b4_rhs_location(%d, %d)[",
-                         rule_length, n);
+                         effective_rule_length, n);
        }
       else
        complain_at (at_loc, _("integer out of range: %s"), quote (text));
Index: src/symlist.c
===================================================================
RCS file: /sources/bison/bison/src/symlist.c,v
retrieving revision 1.17
diff -p -u -r1.17 symlist.c
--- src/symlist.c       5 Jan 2006 13:38:58 -0000       1.17
+++ src/symlist.c       26 Jun 2006 04:08:29 -0000
@@ -39,6 +39,8 @@ symbol_list_new (symbol *sym, location l
   res->location = loc;
 
   res->midrule = NULL;
+  res->midrule_parent_rule = NULL;
+  res->midrule_parent_rhs_index = 0;
 
   res->action = NULL;
   res->used = false;
@@ -102,7 +104,7 @@ unsigned int
 symbol_list_length (const symbol_list *l)
 {
   int res = 0;
-  for (/* Nothing. */; l; l = l->next)
+  for (/* Nothing. */; l && l->sym; l = l->next)
     ++res;
   return res;
 }
Index: src/symlist.h
===================================================================
RCS file: /sources/bison/bison/src/symlist.h,v
retrieving revision 1.14
diff -p -u -r1.14 symlist.h
--- src/symlist.h       5 Jan 2006 13:38:58 -0000       1.14
+++ src/symlist.h       26 Jun 2006 04:08:29 -0000
@@ -32,10 +32,17 @@ typedef struct symbol_list
   symbol *sym;
   location location;
 
-  /* If this symbol is the generated lhs for a mid-rule, a pointer to
-     that mid-rule.  */
+  /* If this symbol is the generated lhs for a midrule but this is the rule in
+     whose rhs it appears, MIDRULE = a pointer to that midrule.  */
   struct symbol_list *midrule;
 
+  /* If this symbol is the generated lhs for a midrule and this is that
+     midrule, MIDRULE_PARENT_RULE = a pointer to the rule in whose rhs it
+     appears, and MIDRULE_PARENT_RHS_INDEX = its rhs index (1-origin) in the
+     parent rule.  */
+  struct symbol_list *midrule_parent_rule;
+  int midrule_parent_rhs_index;
+
   /* The action is attached to the LHS of a rule. */
   const char *action;
   location action_location;
Index: tests/input.at
===================================================================
RCS file: /sources/bison/bison/tests/input.at,v
retrieving revision 1.46
diff -p -u -r1.46 input.at
--- tests/input.at      22 Jun 2006 19:46:05 -0000      1.46
+++ tests/input.at      26 Jun 2006 04:08:29 -0000
@@ -48,89 +48,123 @@ AT_CLEANUP
 AT_SETUP([Type Clashes])
 
 AT_DATA([input.y],
-[[%token foo
+[[%union { int bar; }
+%token foo
 %type <bar> exp
 %%
-exp: foo {} foo
+exp: foo { $$; } foo { $2; } foo
    | foo
    | /* Empty. */
    ;
 ]])
 
-AT_CHECK([bison input.y], [], [],
-[[input.y:4.6-15: warning: type clash on default action: <bar> != <>
-input.y:5.6-8: warning: type clash on default action: <bar> != <>
-input.y:6.5: warning: empty rule for typed nonterminal, and no action
+AT_CHECK([bison input.y], [1], [],
+[[input.y:5.12-13: $$ for the midrule at $2 of `exp' has no declared type
+input.y:5.24-25: $2 of `exp' has no declared type
+input.y:5.6-32: warning: type clash on default action: <bar> != <>
+input.y:6.6-8: warning: type clash on default action: <bar> != <>
+input.y:7.5: warning: empty rule for typed nonterminal, and no action
 ]])
 
 AT_CLEANUP
 
 
-## --------------- ##
-## Unused values.  ##
-## --------------- ##
+# _AT_UNUSED_VALUES_DECLARATIONS()
+# --------------------------------------------
+# Generate the token, type, and destructor
+# declarations for the unused values tests.
+
+m4_define([_AT_UNUSED_VALUES_DECLARATIONS],
+[[[%token <integer> INT;
+%type <integer> a b c d e f g h i j k l;
+%destructor { destroy ($$); } INT a b c d e f g h i j k l;]]])
+
+
+# AT_CHECK_UNUSED_VALUES(DECLARATIONS_AFTER)
+# --------------------------------------------
+# Generate a grammar to test unused values,
+# compile it, run it.  If DECLARATIONS_AFTER
+# is set, then the token, type, and destructor
+# declarations are generated after the rules
+# rather than before.
+
+m4_define([AT_CHECK_UNUSED_VALUES],
+[AT_DATA([input.y],
+m4_ifval($1, [
 
-AT_SETUP([Unused values])
 
-AT_DATA([input.y],
-[[%token <integer> INT
-%type <integer> a b c d e f g h i j k l
-%destructor { destroy ($$); } INT a b c d e f g h i j k l
-%%
+], [_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 { } INT { };
-d: INT | INT { } INT { $1 } INT { };
-e: INT | INT { } INT {  } INT { $1 };
-f: INT | INT { } INT {  } INT { $$ = $1 + $3 + $5; };
-g: INT | INT { $$ } INT { $$ } INT { };
-h: INT | INT { $$ } INT { $$ = $2 } INT { };
-i: INT | INT INT { } { $$ = $1 + $2; };
-j: INT | INT INT { $<integer>$ = 1; } { $$ = $1 + $2; };
-k: INT | INT INT { $$; } { $$ = $3; } { };
-l: INT | INT { $$ = $1; } INT { $$ = $2 + $3; } INT { $$ = $4 + $5; };
+c: INT | INT { $]1[ } INT { } INT { };
+d: INT | INT { } INT { $]1[ } INT { };
+e: INT | INT { } INT {  } INT { $]1[ };
+f: INT | INT { } INT {  } INT { $]$[ = $]1[ + $]3[ + $]5[; };
+g: INT | INT { $]$[ } INT { $]$[ } INT { };
+h: INT | INT { $]$[ } INT { $]$[ = $]2[ } INT { };
+i: INT | INT INT { } { $]$[ = $]1[ + $]2[; };
+j: INT | INT INT { $<integer>$ = 1; } { $]$[ = $]1[ + $]2[; };
+k: INT | INT INT { $]$[; } { $]$[ = $]3[; } { };
+l: INT | INT { $]$[ = $]1[; } INT { $]$[ = $]2[ + $]3[; } INT { $]$[ = $]4[ + 
$]5[; };]]m4_ifval($1, [
+_AT_UNUSED_VALUES_DECLARATIONS])
+)
+
+AT_CHECK([bison input.y], [0], [],
+[[input.y:11.10-32: warning: unset value: $]$[
+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
+input.y:13.10-35: warning: unset value: $]$[
+input.y:13.10-35: warning: unused value: $]3[
+input.y:13.10-35: warning: unused value: $]5[
+input.y:14.10-35: warning: unset value: $]$[
+input.y:14.10-35: warning: unused value: $]3[
+input.y:14.10-35: 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:17.10-38: warning: unset value: $]$[
+input.y:17.10-38: warning: unused value: $]1[
+input.y:17.10-38: warning: unused value: $]2[
+input.y:17.10-38: warning: unused value: $]3[
+input.y:17.10-38: warning: unused value: $]4[
+input.y:17.10-38: warning: unused value: $]5[
+input.y:18.10-43: warning: unset value: $]$[
+input.y:18.10-43: warning: unused value: $]1[
+input.y:18.10-43: warning: unused value: $]3[
+input.y:18.10-43: warning: unused value: $]4[
+input.y:18.10-43: warning: unused value: $]5[
+input.y:20.10-55: warning: unused value: $]3[
+input.y:21.10-41: warning: unset value: $]$[
+input.y:21.10-41: warning: unused value: $]1[
+input.y:21.10-41: warning: unused value: $]2[
+input.y:21.10-41: warning: unused value: $]4[
+]])])
 
-]])
 
-AT_CHECK([bison input.y], [], [],
-[[input.y:11.10-32: warning: unset value: $$
-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
-input.y:13.10-35: warning: unset value: $$
-input.y:13.10-35: warning: unused value: $3
-input.y:13.10-35: warning: unused value: $5
-input.y:14.10-35: warning: unset value: $$
-input.y:14.10-35: warning: unused value: $3
-input.y:14.10-35: 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:17.10-38: warning: unset value: $$
-input.y:17.10-38: warning: unused value: $1
-input.y:17.10-38: warning: unused value: $2
-input.y:17.10-38: warning: unused value: $3
-input.y:17.10-38: warning: unused value: $4
-input.y:17.10-38: warning: unused value: $5
-input.y:18.10-43: warning: unset value: $$
-input.y:18.10-43: warning: unused value: $1
-input.y:18.10-43: warning: unused value: $3
-input.y:18.10-43: warning: unused value: $4
-input.y:18.10-43: warning: unused value: $5
-input.y:20.10-55: warning: unused value: $3
-input.y:21.10-41: warning: unset value: $$
-input.y:21.10-41: warning: unused value: $1
-input.y:21.10-41: warning: unused value: $2
-input.y:21.10-41: warning: unused value: $4
-]])
+## --------------- ##
+## Unused values.  ##
+## --------------- ##
+
+AT_SETUP([Unused values])
+AT_CHECK_UNUSED_VALUES
+AT_CLEANUP
+
+
+## ------------------------------------------ ##
+## Unused values before symbol declarations.  ##
+## ------------------------------------------ ##
 
+AT_SETUP([Unused values before symbol declarations])
+AT_CHECK_UNUSED_VALUES([1])
 AT_CLEANUP