Special-case syntax error of duration before octave marks

lily/parser.yy

 /* -*- mode: c++; c-file-style: "linux"; indent-tabs-mode: t -*- */
 /*
   This file is part of LilyPond, the GNU music typesetter.
 
   Copyright (C) 1997--2020 Han-Wen Nienhuys <hanwen@xs4all.nl>
                  Jan Nieuwenhuizen <janneke@gnu.org>
 
   LilyPond is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
@@ skipping 61 matching lines @@
 
 or
 
     \repeat { \repeat } \alternative
 */
 
 %nonassoc COMPOSITE
 %left ADDLYRICS
 
 %right ':' UNSIGNED REAL E_UNSIGNED EVENT_IDENTIFIER EVENT_FUNCTION '^' '_'
-       HYPHEN EXTENDER DURATION_IDENTIFIER '!'
+        HYPHEN EXTENDER DURATION_IDENTIFIER '!' '\'' ','
 
  /* The above are needed for collecting tremoli and other items (that
     could otherwise be interpreted as belonging to the next function
     argument) greedily, and together with the next rule will serve to
     join numbers and units greedily instead of allowing them into
     separate function arguments
  */
 
 %nonassoc NUMBER_IDENTIFIER
 
@@ skipping 3189 matching lines @@
 
 octave_check:
         /**/ { $$ = SCM_EOL; }
         | '=' quotes { $$ = $2; }
         ;
 
 quotes:
         /* empty */
         {
                 $$ = SCM_INUM0;
-        }
-        | sub_quotes
-        | sup_quotes
+        } %prec ':'
+        | sub_quotes %prec ':'
+        | sup_quotes %prec ':'
         ;
 
-stray_quotes:
-        sub_quotes
-        | sup_quotes
+// no quotes, no error: pass *undefined* in that case.
+erroneous_quotes:
+        quotes {
+                if (scm_is_eq (SCM_INUM0, $1))
+                        $$ = SCM_UNDEFINED;
+        }
         ;
 
 sup_quotes:
         '\'' {
                 $$ = scm_from_int (1);
         }
         | sup_quotes '\'' {
                 $$ = scm_oneplus ($1);
         }
         ;
@@ skipping 310 matching lines @@
                 $$ = scm_cons ($2, $1);
         }
         ;
 
 optional_rest:
         /**/   { $$ = SCM_BOOL_F; }
         | REST { $$ = SCM_BOOL_T; }
         ;
 
 pitch_or_music:
-        pitch exclamations questions octave_check maybe_notemode_duration optional_rest post_events {
+// The erroneous_quotes element is for input such as a1'' which is a
+// typical note entry error that we don't want the parser to get
+// confused about.  The resulting grammar, however, is inconsistent
+// enough that accepting it is not doing anybody a favor.
+        pitch exclamations questions octave_check maybe_notemode_duration erroneous_quotes optional_rest post_events {
                 if (!parser->lexer_->is_note_state ())
                         parser->parser_error (@1, _ ("have to be in Note mode for notes"));
+                if (!SCM_UNBNDP ($6))
+                {
+                        // It's possible to get here without a
+                        // duration, like when there is no
+                        // octave_check but a question mark.  But we
+                        // point out the most frequent error of an
+                        // interspersed duration specifically
+                        if (!SCM_UNBNDP ($5))
+                                parser->parser_error (@6, _ ("octave marks must precede duration"));
+                        else
+                                parser->parser_error (@6, _ ("badly placed octave marks"));
+                        // Try sorting the quotes to where they likely belong
+                        if (scm_is_number ($4)) {
+                                $4 = scm_sum ($4, $6);
+                        } else {
+                                $1 = unsmob<Pitch> ($1)->transposed
+                                        (Pitch (scm_to_int ($6), 0)).smobbed_copy ();
+                        }
+                }
+
                 if (!SCM_UNBNDP ($2)
                     || !SCM_UNBNDP ($3)
                     || scm_is_number ($4)
                     || !SCM_UNBNDP ($5)
-                    || scm_is_true ($6)
-                    || scm_is_pair ($7))
+                    || scm_is_true ($7)
+                    || scm_is_pair ($8))
                 {
                         Music *n = 0;
-                        if (scm_is_true ($6))
+                        if (scm_is_true ($7))
                                 n = MY_MAKE_MUSIC ("RestEvent", @$);
                         else
                                 n = MY_MAKE_MUSIC ("NoteEvent", @$);
 
                         n->set_property ("pitch", $1);
                         if (SCM_UNBNDP ($5))
                                 n->set_property ("duration",
                                                  parser->default_duration_.smobbed_copy ());
                         else
                                 n->set_property ("duration", $5);
 
                         if (scm_is_number ($4))
                         {
                                 int q = scm_to_int ($4);
                                 n->set_property ("absolute-octave", scm_from_int (q-1));
                         }
-
-                        if (to_boolean ($3))
-                                n->set_property ("cautionary", SCM_BOOL_T);
-                        if (to_boolean ($2) || to_boolean ($3))
-                                n->set_property ("force-accidental", SCM_BOOL_T);
-                        if (scm_is_pair ($7))
-                                n->set_property ("articulations",
-                                                 scm_reverse_x ($7, SCM_EOL));
-                        $$ = n->unprotect ();
-                }
-        } %prec ':'
-        // Next rule is a frequent note entry error, like c4''
-        //
-        // It is quite unlikely that an octave check precedes a
-        // duration, but we have to keep it in the rule in order not
-        // to force the parser into early decisions before actually
-        // seeing a stray quote.  So we try to best interpret that
-        // case as well, even though it's not a likely error case.
-        | pitch exclamations questions octave_check duration stray_quotes optional_rest post_events {

[hanwenn, 2020/02/12 06:45:08]

why can't this case be folded in to the preceding rule? Maybe this comment says
it already; if so an example would help.

[dak, 2020/02/12 10:38:16]

Astute observation.  We print out the grammar in the manual and mixing error
productions and normal productions seems weird and misleading.  However, there
is no way to hide error productions from the grammar, so my version is not
better in that regard (though stray_quotes seems rather obvious).

But you are right: this calls for making folding into the previous rule.
Will do.

-                if (!parser->lexer_->is_note_state ())
-                        parser->parser_error (@1, _ ("have to be in Note mode for notes"));
-                {
-                        Music *n = 0;
-                        if (scm_is_true ($7))
-                                n = MY_MAKE_MUSIC ("RestEvent", @$);
-                        else
-                                n = MY_MAKE_MUSIC ("NoteEvent", @$);
-
-                        if (scm_is_number ($4))
-                        {
-                                int q = scm_to_int ($4) + scm_to_int ($6);
-                                n->set_property ("absolute-octave", scm_from_int (q-1));
-                        } else
-                                $1 = unsmob<Pitch> ($1)->transposed
-                                        (Pitch (scm_to_int ($6), 0)).smobbed_copy ();
-
-                        n->set_property ("pitch", $1);
-                        n->set_property ("duration", $5);
 
                         if (to_boolean ($3))
                                 n->set_property ("cautionary", SCM_BOOL_T);
                         if (to_boolean ($2) || to_boolean ($3))
                                 n->set_property ("force-accidental", SCM_BOOL_T);
                         if (scm_is_pair ($8))
                                 n->set_property ("articulations",
                                                  scm_reverse_x ($8, SCM_EOL));
                         $$ = n->unprotect ();
                 }
-                parser->parser_error (@6, _ ("octave marks must precede duration"));

[hanwenn, 2020/02/12 06:45:08]

add a comment that we sholudn't drop this error, even though this works for
users (I think we don't want that because it makes parsing .ly harder for
others.)

[dak, 2020/02/12 10:38:16]

Accepting that "syntax" would give me a rash.  I had enough of a problem giving
the compiler knowledge about it.  Having it accept it would make me very
unhappy.

I'll try to put in an appropriate comment.

         } %prec ':'
         | new_chord post_events {
                 if (!parser->lexer_->is_chord_state ())
                         parser->parser_error (@1, _ ("have to be in Chord mode for chords"));
                 if (scm_is_pair ($2)) {
                         if (unsmob<Pitch> ($1))
                                 $1 = make_chord_elements (@1,
                                                           $1,
                                                           parser->default_duration_.smobbed_copy (),
                                                           SCM_EOL);
@@ skipping 1084 matching lines @@
         Lily_lexer *lex = parser->lexer_;
 
         lex->lexval_ = s;
         lex->lexloc_ = loc;
         int tok = lex->pop_extra_token ();
         if (tok >= 0)
                 return tok;
         lex->prepare_for_next_token ();
         return lex->yylex ();
 }