Thread

1. bison location reporting for potentially-empty list productions

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-04T00:29:24Z

   In the just-committed patch for CREATE SCHEMA IF NOT EXISTS, there is
   an error thrown by the grammar when IF NOT EXISTS is specified together
   with any schema-element clauses.  I thought it would make more sense for
   the error cursor to point at the schema-element clauses, rather than at
   the IF NOT EXISTS as submitted.  So the code looks like, eg,
   
               | CREATE SCHEMA IF_P NOT EXISTS ColId OptSchemaEltList
                       ...
                       if ($7 != NIL)
                           ereport(ERROR,
                                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                                    errmsg("CREATE SCHEMA IF NOT EXISTS cannot include schema elements"),
                                    parser_errposition(@7)));
   
   However, it turns out this actually results in a cursor pointing at the
   previous word:
   
   CREATE SCHEMA IF NOT EXISTS test_schema_1 -- fail, disallowed
          CREATE TABLE abc (
                 a serial,
                 b int UNIQUE
          );
   ERROR:  CREATE SCHEMA IF NOT EXISTS cannot include schema elements
   LINE 1: CREATE SCHEMA IF NOT EXISTS test_schema_1 
                                       ^
   
   After some study I think what is happening is that there's a deficiency
   in the location-tracking macro in gram.y:
   
   /* 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)
   
   OptSchemaEltList looks like this:
   
   OptSchemaEltList:
               OptSchemaEltList schema_stmt            { $$ = lappend($1, $2); }
               | /* EMPTY */                           { $$ = NIL; }
           ;
   
   When the macro is evaluated for the initial empty production for
   OptSchemaEltList, the "else" case causes it to index off the beginning
   of the array and pick up the location for the preceding token.  Then,
   each succeeding reduction of the first part of the production copies
   that bogus value from the first RHS member item.  So this same problem
   applies not only to OptSchemaEltList but to any case where we've formed
   a zero-or-more-element-list production in this style.  So far as I can
   tell, there are no other cases in the current grammar where we make use
   of the position of a list nonterminal for error-reporting purposes,
   which is why we'd not noticed this before.  But it seems likely to come
   up again.
   
   It seems clear to me now that the macro ought at least to be changed to
   
   #define YYLLOC_DEFAULT(Current, Rhs, N) \
       do { \
           if (N) \
               (Current) = (Rhs)[1]; \
           else \
               (Current) = -1; \
       } while (0)
   
   so that the parse location attributed to an empty production is -1
   (ie, unknown) rather than the current quite bogus behavior of marking
   it as the preceding token's location.  (For one thing, what if there
   *isn't* a preceding token?  That's not possible I think in the current
   grammar, but if it were possible this code would be indexing off the
   beginning of the locations array.)
   
   But that change wouldn't really fix the issue for CREATE SCHEMA ---
   the -1 would be propagated up to all instances of OptSchemaEltList
   even after we'd reduced the first production a few times, so that
   we'd end up printing no error cursor for this case.
   
   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.
   
   At this point I'm inclined to change the macro to the second case
   (with the -1) and accept a less good error cursor position for the
   CREATE SCHEMA error.
   
   Has anybody got a better idea?
   
   			regards, tom lane
   
   
   

2. Re: bison location reporting for potentially-empty list productions

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-04T15:43:12Z

   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