Re: Wrong type in part-combiner.scm

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> Pierre-Luc Gauthier <p.luc.gauthier@gmail.com> writes:
>
>> Hi there,
>>
>> When I compile :
>>
>> test = {}
>> \addQuote "test" \test
>>
>> I get :
>>
>> GNU LilyPond 2.21.7
>> Processing `test.ly'
>> Parsing...
>> Interpreting music.../usr/share/lilypond/2.21.7/scm/part-combiner.scm:956:30:
>> In procedure car in expression (car current-tail):
>> /usr/share/lilypond/2.21.7/scm/part-combiner.scm:956:30: Wrong type
>> (expecting pair): #f
>>
>> Can anyone corroborate ?
>
> That's interesting.  With my current version in /usr/local/bin I get
>
> -*- mode: compilation; default-directory: "/tmp/" -*-
> Compilation started at Tue Sep 22 14:35:27
>
> lilypond /tmp/ba.ly
> GNU LilyPond 2.21.0
> Processing `/tmp/ba.ly'
> Parsing...
> Interpreting music...
> warning: no music found in score
> /tmp/ba.ly:2:18: warning: quoted music `test' is empty
> \addQuote "test" 
>                  \test
> /tmp/ba.ly:1: warning: no \version statement found, please add
>
> \version "2.21.0"
>
> for future compatibility
> Success: compilation successfully completed
>
> Compilation finished at Tue Sep 22 14:35:28
>
> With current master, I instead get
>
> -*- mode: compilation; default-directory: "/tmp/" -*-
> Compilation started at Tue Sep 22 14:35:51
>
> /usr/local/tmp/lilypond/out/bin/lilypond ba.ly
> GNU LilyPond 2.21.7
> Processing `ba.ly'
> Parsing...
> Interpreting 
> music.../usr/local/tmp/lilypond/out/share/lilypond/current/scm/part-combiner.scm:956:30:
>  In procedure car in expression (car current-tail):
> /usr/local/tmp/lilypond/out/share/lilypond/current/scm/part-combiner.scm:956:30:
>  Wrong type (expecting pair): #f
>
> Compilation exited abnormally with code 1 at Tue Sep 22 14:35:52
>
> The code bombing out is clearly wrong:
>
>     (if (null? quote-contents)
>         (let find-non-empty ((current-tail (member raw-voice context-list)))
>           ;; if voice has contents, use them, otherwise check next ctx
>           (cond ((null? current-tail) #f)
>                 ((and (pair? (car current-tail))
>                       (pair? (cdar current-tail)))
>                  (set! quote-contents (cdar current-tail)))
>                 (else (find-non-empty (cdr current-tail))))))
>
> because (null? current-tail) can never be true (it's either #f or a
> pair).  This should be (not current-tail) instead.
>
> However, this particular buggy code has not changed for a long long
> time, so a more recent change must have affected context-list which
> previous to that change apparently always contained raw-voice somewhere,
> or at least in the case you now posted.

The whole code in the find-non-empty loop, apart from this error, is
garbage.  Here is a bit more code and analysis:

(define-public (add-quotable name mus)
  (let* ((tab (eval 'musicQuotes (current-module)))
         (voicename (get-next-unique-voice-name))
         ;; recording-group-emulate returns an assoc list (reversed!), so
         ;; hand it a proper unique context name and extract that key:
         (ctx-spec (context-spec-music mus 'Voice voicename))
         (listener (ly:parser-lookup 'partCombineListener))
         (context-list (reverse (recording-group-emulate ctx-spec listener)))
         (raw-voice (assoc voicename context-list))
         (quote-contents (if (pair? raw-voice) (cdr raw-voice) '())))

    ;; If the context-specced quoted music does not contain anything, try to
    ;; use the first child, i.e. the next in context-list after voicename
    ;; That's the case e.g. for \addQuote "x" \relative c \new Voice {...}
    (if (null? quote-contents)
        (let find-non-empty ((current-tail (member raw-voice context-list)))
          ;; if voice has contents, use them, otherwise check next ctx
          (cond ((null? current-tail) #f)
                ((and (pair? (car current-tail))
                      (pair? (cdar current-tail)))
                 (set! quote-contents (cdar current-tail)))
                (else (find-non-empty (cdr current-tail))))))

    (if (not (null? quote-contents))
        (hash-set! tab name (list->vector (reverse! quote-contents '())))
        (ly:music-warning mus (ly:format (_ "quoted music `~a' is empty") 
name)))))

So raw-voice is an assoc into context-list.  This assoc is then searched
for in the find-non-empty loop.  For each found instance, a test is
made.  If that test is not successful, the search is repeated for more
instances.  However, since all found instances are _guaranteed_ to be
the same (since assoc is only called once, and then member searches for
the same key/value pair rather than just the same key), the test is
guaranteed to deliver the same result for each found instance.

This is just crap.  Presumably the idea was to search for the same key
but potentially different values?

-- 
David Kastrup