Re: bison location reporting for potentially-empty list productions - Mailing list pgsql-hackers

I wrote:
> To produce a really useful cursor for this type of situation I think
> we would have to do something like this:

> #define YYLLOC_DEFAULT(Current, Rhs, N) \
>     do { \
>         int i;
>         (Current) = -1; \
>         for (i = 1; i <= (N); i++)
>         {
>             (Current) = (Rhs)[i]; \
>             if ((Current) >= 0)
>                 break;
>         }
>     } while (0)

> This is pretty ugly and seems likely to create a visible hit on the
> parser's speed (which is already not that good).  I'm not sure it's
> worth it for something that seems to be a corner case --- anyway
> we've not hit it before in six years since the location tracking
> support was added.

After another look at the Bison manual I realized that there's a way to
fix this without making YYLLOC_DEFAULT slower, because that macro only
sets the *default* location for the current nonterminal; the rule action
is allowed to override its location value.  So I propose the attached
patch against HEAD.  This is a little bit ugly because we have to
remember to put some extra code in productions that have this issue ...
but track record so far suggests that there will be very few of them.

Anyone have an objection, or a better idea?

            regards, tom lane

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7feadeac1693e7ff46f44ef3ad9ea2fa86bf46a8..e4ff76e66e0990d91790ccc54615c26dd64a143e 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 67,82 ****
  #include "utils/xml.h"


! /* Location tracking support --- simpler than bison's default */
  #define YYLLOC_DEFAULT(Current, Rhs, N) \
      do { \
!         if (N) \
              (Current) = (Rhs)[1]; \
          else \
!             (Current) = (Rhs)[0]; \
      } while (0)

  /*
   * Bison doesn't allocate anything that needs to live across parser calls,
   * so we can easily have it use palloc instead of malloc.  This prevents
   * memory leaks if we error out during parsing.  Note this only works with
--- 67,100 ----
  #include "utils/xml.h"


! /*
!  * Location tracking support --- simpler than bison's default, since we only
!  * want to track the start position not the end position of each nonterminal.
!  */
  #define YYLLOC_DEFAULT(Current, Rhs, N) \
      do { \
!         if ((N) > 0) \
              (Current) = (Rhs)[1]; \
          else \
!             (Current) = (-1); \
      } while (0)

  /*
+  * The above macro assigns -1 (unknown) as the parse location of any
+  * nonterminal that was reduced from an empty rule.  This is problematic
+  * for nonterminals defined like
+  *        OptFooList: / * EMPTY * / { ... } | OptFooList Foo { ... } ;
+  * because we'll set -1 as the location during the first reduction and then
+  * copy it during each subsequent reduction, leaving us with -1 for the
+  * location even when the list is not empty.  To fix that, do this in the
+  * action for the nonempty rule(s):
+  *        if (@$ < 0) @$ = @2;
+  * (Although we have many nonterminals that follow this pattern, we only
+  * bother with fixing @$ like this when the nonterminal's parse location
+  * is actually referenced in some rule.)
+  */
+
+ /*
   * Bison doesn't allocate anything that needs to live across parser calls,
   * so we can easily have it use palloc instead of malloc.  This prevents
   * memory leaks if we error out during parsing.  Note this only works with
*************** OptSchemaName:
*** 1223,1230 ****
          ;

  OptSchemaEltList:
!             OptSchemaEltList schema_stmt            { $$ = lappend($1, $2); }
!             | /* EMPTY */                            { $$ = NIL; }
          ;

  /*
--- 1241,1254 ----
          ;

  OptSchemaEltList:
!             OptSchemaEltList schema_stmt
!                 {
!                     if (@$ < 0)            /* see comments for YYLLOC_DEFAULT */
!                         @$ = @2;
!                     $$ = lappend($1, $2);
!                 }
!             | /* EMPTY */
!                 { $$ = NIL; }
          ;

  /*
diff --git a/src/test/regress/expected/namespace.out b/src/test/regress/expected/namespace.out
index 5fcd46daf42705147f53599d657e52d8fe7b5b1f..9187c8126a3ffa32e365400816b262daf3b6f2d5 100644
*** a/src/test/regress/expected/namespace.out
--- b/src/test/regress/expected/namespace.out
*************** CREATE SCHEMA IF NOT EXISTS test_schema_
*** 47,54 ****
                b int UNIQUE
         );
  ERROR:  CREATE SCHEMA IF NOT EXISTS cannot include schema elements
! LINE 1: CREATE SCHEMA IF NOT EXISTS test_schema_1
!                                     ^
  DROP SCHEMA test_schema_1 CASCADE;
  NOTICE:  drop cascades to 2 other objects
  DETAIL:  drop cascades to table test_schema_1.abc
--- 47,54 ----
                b int UNIQUE
         );
  ERROR:  CREATE SCHEMA IF NOT EXISTS cannot include schema elements
! LINE 2:        CREATE TABLE abc (
!                ^
  DROP SCHEMA test_schema_1 CASCADE;
  NOTICE:  drop cascades to 2 other objects
  DETAIL:  drop cascades to table test_schema_1.abc