bug#14029: 24.2.50; [PATCH] imenu problems with special elements

[Andreas Politz]

Andreas Politz <politza@fh-trier.de> writes:

> "Drew Adams" <drew.adams@oracle.com> writes:
>
>> Thanks for this report and fix.  Neither the original code nor your patch is
>> super clear to me, so I have some (non-rhetorical) questions below.  But if
>> someone else understands this better, feel free to ignore.
>>
>>> | Special elements look like this:
>>> |           (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
>>> |      Selecting a special element performs:
>>> |           (funcall FUNCTION
>>> |                    INDEX-NAME INDEX-POSITION ARGUMENTS...)
>>> 
>>> 1. At least one function does not recognize these elements, resulting
>>> in an error.
>>
>> Can you be more specific?  Which function?  What error?  Recipe to reproduce?
>> (Maybe you are referring to the last change in your patch, for `imenu'?)
>
> Yes in `imenu--in-alist'.  It used to be >1, but one is already fixed in
> bzr.
>
>>
>>> 2. The advertised calling convention is different from the 
>>> actual one in `imenu'.
>>
>> How so?  What difference do you see, where?
>>
>
> The last change in `imenu' about cdddr.  
>
>>> 3. The `imenu--subalist-p' predicate is unsafe and incomplete.
>>
>> How so?
>>
>
>>>   (defun imenu--subalist-p (item)
>>> !   (and (consp (cdr item)) (listp (cadr item))
>>
>>    (defun imenu--subalist-p (item)
>>> !   (and (consp item)
>>> !        (consp (cdr item))
>>> !        (listp (cadr item))
>>
>> (consp (cdr item)) is equivalent to
>> (and (consp item) (consp (cdr item))), assuming ITEM has the proper form (so
>> that cdr does not raise an error).  (consp (cdr-safe item)) should do the 
>> same
>> thing.
>
> That's right.
>
>>
>>> !        (not (eq (car (cadr item)) 'lambda))))
>>
>>> !        (not (functionp (cadr item)))))
>>
>> Is it possible for (cadr item) to be a list and also be `functionp' and yet 
>> not
>> have its car be `lambda'?  Dunno.  I was under the impression that it was
>> impossible, but I could be wrong.  If it is possible, is it better to test
>> `functionp' here?  Dunno.
>
> If the documentation states FUNCTION, then it should be a function.
>
> Let's recap.  The three types are:
>
> (INDEX-NAME . INDEX-POSITION)
> (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
> (MENU-TITLE SUB-ALIST)
>
> First, I think the documentation is incorrect, and the last one should read
>
> (MENU-TITLE . SUB-ALIST)
>
> since SUB-ALIST is supposed to be the cdr of the list (see
> e.g. imenu--split-submenus)
>
> So (listp (cdr item)) would exclude the first type and we are
> left with
>
> (INDEX-POSITION FUNCTION ARGUMENTS...) and
> SUB-ALIST = (ITEM ITEM ...)
>
> and (cadr item) is either a function, an item or nil.  I think
> that INDEX-NAME and MENU-TITLE (the car of a possible item) are
> supposed to be strings, so this should work.
>
>
>>
>>> !        (if (setq res (imenu--in-alist str tail))
>>
>>> !        (if (and (imenu--subalist-p elt)
>>> !                 (setq res (imenu--in-alist str tail)))
>>
>> Why is (imenu--subalist-p elt) needed here?  What error case does it 
>> prevent?  
>>
>> Can't the code assume a properly constructed menu here, so that if TAIL is a
>> list then it is a bottom-level element, and so it is proper to just set 
>> ALIST to
>> nil?
>>
>
> No, it may contain subalists.
>
>>> !      (rest (if is-special-item (cddr index-item))))
>>
>>> !      (rest (if is-special-item (cdddr index-item))))
>>
>> This change looks good, but `cdddr' is in cl.el, so perhaps it is better to 
>> use
>> (nthcdr 3 index-item).
>>
>> I'm only a little bit surprised that this one hasn't already been reported 
>> and
>> fixed - there have been other bugs (e.g. #12717) related to special items.  I
>> use special items myself, but so far I have not used non-nil ARGS, so I have 
>> not
>> encountered this one (your last change).
>
> That, and you probably always used the mouse menu.  
>
> A