Syntax: lyric_mode_music might be a MUSIC_FUNCTION

Syntax: lyric_mode_music might be a MUSIC_FUNCTION

With this patch, all four scores in the lilypond
source below use a valid syntax and produce correct
output.

Without this patch only the first two scores use
a valid syntax (an extra pair of curly brackets
would fix the problem).

  \version "2.21.0"

  music = \relative { c''2 2 }
  lyrics_dir = \lyricmode { foo bar }
  lyrics_fun = #(define-music-function () ()
                 #{ \lyricmode { foo bar } #})

  \score {\music \addlyrics { foo bar } \layout {}}
  \score {\music \addlyrics \lyrics_dir \layout {}}
  \score {\music \addlyrics \lyrics_fun \layout {}}
  \score {\music \addlyrics \displayLilyMusic
                            \lyrics_fun \layout {}}

Signed-off-by: Knut Petersen <knut_petersen@t-online.de>

[knupero, 2018-04-25 06:33:19]

Please test & review the proposed syntax change.

[dak, 2018-04-25 08:36:05]

On 2018/04/25 06:33:19, knupero wrote:
> Please test & review the proposed syntax change.

I don't see the point in starting a formal review after this has already been
discussed on the mailing list.  In particular since this differs from your
originally proposed patch (where you did not switch to lyrics mode at all) and
you disengenuously continued the discussion based on a modified patch (the one
proposed here) which you then tacked onto your results without further mention. 
Now let's review this modified patch.  The respective passage in the parser
contains the comment

// We must not have lookahead tokens parsed in lyric mode.  In order
// to save confusion, we take almost the same set as permitted with
// \lyricmode and/or \lyrics.  However, music identifiers are also
// allowed, and they obviously do not require switching into lyrics
// mode for parsing.

Your code gets out of lyrics mode after scanning the music function _and_ a
prospective lookahead token.  This lookahead token will consequently be parsed
in the wrong state (lyrics mode instead of music mode).  For example, in

{
  e1 \addlyrics \displayLilyMusic hi-there
  c1
}

c1 will not be recognized as a note event since it is parsed in lyrics mode as
hi-there could be followed by a post-event.  Most music function arguments will
be terminated by examining a lookahead token.  A braced list (as used in your
examples) is one of the rare kind of arguments that isn't.

We don't state "please don't use that syntax in the following way or LilyPond
will be silently confused into producing nonsense" in other respects so it is
irrelevant that LilyPond can parse _some_ input correctly with that change and
will "only" get confused about other input.

[dak, 2018-04-25 09:05:16]

On 2018/04/25 08:36:05, dak wrote:

> We don't state "please don't use that syntax in the following way or LilyPond
> will be silently confused into producing nonsense" in other respects so it is
> irrelevant that LilyPond can parse _some_ input correctly with that change and
> will "only" get confused about other input.

To wit: to do this correctly, one would need to define a production for a
special kind of music function call that ends in a manner not requiring
lookahead, like a braced music list and/or \default.  Extending that set further
would yield diminishing returns since it would become increasingly harder to
document what may and may not work.

It would still mean that one would need to document why music functions in this
location only accept specific kinds of arguments.  So it would be quite more
work than you imagine to cover only reliably recognizable constructs in the
parser, and it would be a puzzler to document as well, for a comparatively small
actual gain.

That's the reason for the decision to draw the line here for me and document the
restriction in the comment.

Feel free to draw the line differently, but then you also need to do all the
work necessary for redrawing the line.  It's a whole lot more than you appear to
think, and your unwillingness to believe me in regard to statements about the
parser I make does not bode particularly well for the prospect of acquiring the
necessary expertise for implementing such a change.

It's certainly a good idea to question authority, but considering that
significantly more than half of the parser code is now attributed to me and
basically all of the music function handling, it stands to reason that there
might be a point to statements I make either in comments or on the mailing list
and you might want to double-check before dismissing them.

[knupero, 2018-04-25 12:22:18]

> I don't see the point in starting a formal review after this has already been
> discussed on the mailing list.

I was hoping, apparently in vain, that the formal process would lead to a more
reasonable discussion. Apparently, however, this process also invites
speculation about the amount of work someone is willing to invest, his
qualifications and his honesty.

> In particular since this differs from your
> originally proposed patch (where you did not switch to lyrics mode at all) and
> you disengenuously continued the discussion based on a modified patch (the one
> proposed here) which you then tacked onto your results without further
mention.

For the record: All the test results were obtained with a) an unpatched lilypond
and b) a lilypond with my originally proposed patch.
This is easy to verify, and everybody is invited to do so. David: You did not do
that, apparently.

@Everyone: I won't bother you here any longer.

[dak, 2018-04-25 14:29:16]

On 2018/04/25 12:22:18, knupero wrote:
> > I don't see the point in starting a formal review after this has already
been
> > discussed on the mailing list.
> 
> I was hoping, apparently in vain, that the formal process would lead to a more
> reasonable discussion.

You make it appear like "reasonable" means "with the desired outcome".

The formal process differs from the discussion not in who participates but
rather that it is connected with semi-automated procedures for making a given
patch progress into the code base, given that the discussion on the trackers
does not suggest the need for further changes.

Proceeding to a formal patch suggestion when the discussion on the developer
list does not suggest the patch's design being in acceptable state is just a
waste of everyone's time.  This is different when we are not talking about the
design of the patch but rather its execution: in that case the line-by-line
review and commenting tools might be helpful for getting the patch progress into
a form which is acceptable.

The "formal" process is not a substitute for discussion, merely a tool for
allowing a more detailed review of the code.

> Apparently, however, this process also invites
> speculation about the amount of work someone is willing to invest, his
> qualifications and his honesty.

You are using hyperbole.

Your original patch suggestion was in
https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00097.html .  This
produces "not a note name" errors for things which are, well, not a note name. 
The error recovery of current grammar rules runs through rules then actually
producing lyric elements, and it would do so regardless of whether the results
are then used as parts of lyrics or other music expressions.  Which is more a
side effect of the current grammar refactoring and an oversight in error
processing than actually useful behavior.

When I asked "how was the output correct?" without checking the output of the
current recovery path for strings encountered outside of lyrics mode, you sent a
reply https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00108.html . 
This contains a whole lot of verbiage as well as PDFs.

At the end of the verbiage, there indeed is a sentence

> Attached is a 2nd version of the patch. With that extended version _all_
examples above
> compile without warnings/errors and give the desired result.

which I overlooked.  My reply
https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00114.html however
comments on why there is a requirement of a braced music list when switching to
Lyrics mode.

Instead of replying to that objection, you formally propose the patch which will
not work reliably because of the reasons I pointed out.

While a careful rereading of the communication makes clear that I was wrong
stating:

> > In particular since this differs from your
> > originally proposed patch (where you did not switch to lyrics mode at all)
and
> > you disengenuously continued the discussion based on a modified patch (the
one
> > proposed here) which you then tacked onto your results without further
> > mention.
> 
> For the record: All the test results were obtained with a) an unpatched
lilypond
> and b) a lilypond with my originally proposed patch.

The record supports your statement here.

> This is easy to verify, and everybody is invited to do so. David: You did not
do
> that, apparently.

No, I didn't.  However, that the current error recovery path produces lyrics
events (regardless in which mode spurious strings appear) does not imply that
the flagged behavior would lead to sensible recognition of lyrics input which
differs from music input in a whole lot more respects than just accepting words
or not.  You can, for example, write sentences with punctuation in lyrics mode,
something which would not work in the original code triggering errors but still
producing lyrics as recovery.

So there was no point in testing the patch as I knew perfectly well it could not
function comprehensively.  That led me to overlooking the second patch which
would fail in less obvious manners (without triggering the error path) for input
requiring lookahead, as spelled out in the comment in the code and explained by
myself in the discussion.

I apologize for misinterpreting your actions with regard to the second patch I
overlooked.  However, your disinterest in accepting any explanation not in line
with your desires and just going ahead ignoring input in the expectation that
this may somehow lead to more favorable results is annoying, and I might have
overreacted as a result of that annoyance.

Whether it is inconvenient for your use case or not: mode-changing commands can
only accept a limited number of constructs, those known to be delimited well
enough that recognizing them does not require further lookahead, or the next
token will get scanned as lookahead in the wrong mode, an action which cannot be
redone.

It would be feasible to create a family of music function calls matching that
descriptions.  However, doing so would require a thorough understanding of the
current parser code and your manner of rejecting input is likely going to make
the acquisition of such knowledge comparatively ineffective for you as well as
reviewers.  That's not necessarily a showstopper: after all, I acquired
basically all of my current parser chops at a time where its previous
maintainers were essentially no longer available.

However, it makes this particular project a bad idea for starting.

[Carl, 2018-04-25 20:47:09]

On 2018/04/25 12:22:18, knupero wrote:

> 
> @Everyone: I won't bother you here any longer.

Knut,

It's not a bother.

But it's important to recognize that the parser is a difficult system to work
with, and we've had lots of problems over the years with lookahead not being
handled properly.  So the issue that David raises here about lookahead is a real
issue.  

Your proposal is to allow music functions enclosed in braced lists, but I don't
know that there is anything in the parser that explicitly says it needs a braced
list.

I can see the value of having lyrics provided by a music function (for instance,
if you want to sing solfege).

It's just important to do it properly so it works well in the parser.  And David
has spent a lot of time trying to make the parser work in an explainable and
predictable manner.  And this means that there are dozens of things that didn't
work well in 2.14 that now are trivial to do in 2.19.

So hang in there, and have the discussions, and I'm convinced there is a way to
make both you and David happy.  And LilyPond will be better because of it.

Thanks,

Carl

[dak, 2018-04-25 21:14:11]

Carl.D.Sorensen@gmail.com writes:

> On 2018/04/25 12:22:18, knupero wrote:
>
>
>> @Everyone: I won't bother you here any longer.
>
> Knut,
>
> It's not a bother.
>
> But it's important to recognize that the parser is a difficult system to
> work with, and we've had lots of problems over the years with lookahead
> not being handled properly.  So the issue that David raises here about
> lookahead is a real issue.
>
> Your proposal is to allow music functions enclosed in braced lists, but
> I don't know that there is anything in the parser that explicitly says
> it needs a braced list.

No, the proposal was to allow music functions _not_ enclosed in braced
lists as the music expression subject to \addlyrics .  His examples were
of music function calls where the last argument was brace-enclosed, but
the proposed syntax would have accepted music function calls without
such delimited arguments, switching back from lyrics mode at a point
where the next token already had been scanned in lyrics mode.

> I can see the value of having lyrics provided by a music function (for
> instance, if you want to sing solfege).

You can do that by using braces around the lyricmode expression
including the music function call already.  The proposal was to allow
this without braces.  Functionally, this is different since the
additional level of braces also creates an additional layer of
sequential music.  However, nesting several layers of sequential music
does not typically lead to different results since the wrapping
sequential music expression does not survive into the stream events
caused by iteration.

> It's just important to do it properly so it works well in the parser.
> And David has spent a lot of time trying to make the parser work in an
> explainable and predictable manner.  And this means that there are
> dozens of things that didn't work well in 2.14 that now are trivial to
> do in 2.19.
>
> So hang in there, and have the discussions, and I'm convinced there is
> a way to make both you and David happy.

I've already spelled out the approach that would be required for solving
that problem in a sound technical manner.  However, a "discussion"
requires a basic willingness to listen.  With Knut not considering
comments in the code and explanations in the mailing list and at the
same time acting indignant when feeling his competence is being
questioned, there does not seem an amicable way to bring him up to speed
on the parser internals without having to fight him for accepting any
bit of information.

This is very time-intensive, exhausting, draining, and unpleasant.  I
don't really want to spend my time in that manner, particularly if I
don't see this getting anywhere.  Now most certainly any colloborator on
the syntax would want initial coaching and I am willing to do my part in
that.  But not when this means a constant fight to get the other side
accept any bit of information or experience.

I very much doubt that there is a "way to make both [Knut] and David
happy" but I'd be willing to settle for a communication style allowing
for technically sound contributions to evolve without having an ugly
fight over every item.

> https://codereview.appspot.com/343820043/

-- 
David Kastrup