Bugs in newly added completion capabilities.

Bugs in newly added completion capabilities.

[Luc Teirlinck]

The following change:

2005-02-22  Kim F. Storm  <storm@cua.dk>

	    * minibuf.c (Ftry_completion, Fall_completions): Allow
              both string and symbol keys in alists and hash tables.

has some remaining bugs and inconsistencies.  First of all, it does
not work perfectly for try-completion and all-completions as the ielm
run below shows.  This problem actually prevents minibuffer completion
from working in the patch I sent for Custom themes (so the problem
occurs in natural, practical situations):

ELISP> (try-completion "b" '(aa bb))
*** Eval error ***  Invalid function: (aa bb)
ELISP> (try-completion "b" '("aa" bb))
"bb"
ELISP> 

The problem is that try-completion and friends try to handle both
lists of symbols and anonymous lambda expressions.  They check whether
a list is an anonymous lambda expression by checking whether its car
is a symbol other than nil.  Why not just simply check whether the car
is lambda?  The patch below implements this for try-completion,
all-completions and test-completion (which has the same problem).
After that, a user would still have to make sure that his list of
symbols does not start with lambda, which we could point out in the
docs.   That may not be pretty, but should not yield too much
inconvenience in practice.

This leaves the inconsistency that try-completion and all-completions
now can handle symbols in alists, lists and hash-tables, but not
test-completion.  Even after applying my patch, one gets:

ELISP> (test-completion "bb" '(aa bb))
nil
ELISP> 

I do not know what the best fix for that is.  Would we be willing to
go as far as to make assoc-string treat symbols as strings in exactly
the same way, which would automatically take care of test-completion?

Here is my patch, which I can install if desired:

===File ~/minibuf.c-diff====================================
*** minibuf.c	25 Mar 2005 14:53:52 -0600	1.280
--- minibuf.c	27 Jun 2005 15:51:51 -0500	
***************
*** 1217,1224 ****
    int type = HASH_TABLE_P (alist) ? 3
      : VECTORP (alist) ? 2
      : NILP (alist) || (CONSP (alist)
! 		       && (!SYMBOLP (XCAR (alist))
! 			   || NILP (XCAR (alist))));
    int index = 0, obsize = 0;
    int matchcount = 0;
    int bindcount = -1;
--- 1217,1223 ----
    int type = HASH_TABLE_P (alist) ? 3
      : VECTORP (alist) ? 2
      : NILP (alist) || (CONSP (alist)
! 		       && !EQ (XCAR (alist), Qlambda));
    int index = 0, obsize = 0;
    int matchcount = 0;
    int bindcount = -1;
***************
*** 1480,1487 ****
    int type = HASH_TABLE_P (alist) ? 3
      : VECTORP (alist) ? 2
      : NILP (alist) || (CONSP (alist)
! 		       && (!SYMBOLP (XCAR (alist))
! 			   || NILP (XCAR (alist))));
    int index = 0, obsize = 0;
    int bindcount = -1;
    Lisp_Object bucket, tem, zero;
--- 1479,1485 ----
    int type = HASH_TABLE_P (alist) ? 3
      : VECTORP (alist) ? 2
      : NILP (alist) || (CONSP (alist)
! 		       && !EQ (XCAR (alist), Qlambda));
    int index = 0, obsize = 0;
    int bindcount = -1;
    Lisp_Object bucket, tem, zero;
***************
*** 1754,1761 ****
  
    CHECK_STRING (string);
  
!   if ((CONSP (alist) && (!SYMBOLP (XCAR (alist)) || NILP (XCAR (alist))))
!       || NILP (alist))
      {
        tem = Fassoc_string (string, alist, completion_ignore_case ? Qt : Qnil);
        if NILP (tem)
--- 1752,1758 ----
  
    CHECK_STRING (string);
  
!   if ((CONSP (alist) && !EQ (XCAR (alist), Qlambda)) || NILP (alist))
      {
        tem = Fassoc_string (string, alist, completion_ignore_case ? Qt : Qnil);
        if NILP (tem)
============================================================

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    The problem is that try-completion and friends try to handle both
    lists of symbols and anonymous lambda expressions.

Looking at the code, I don't think they do.  It does not try to
handle a list of symbols--and, because of the conflict you mentioned,
it should not try.

I think we should document that a simple list of symbols is NOT
allowed, but that you can add "" as the first element to make that
list into a legitimate completion table.

Can you fix your custom thread patch to do that?

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Richard Stallman wrote:

       The problem is that try-completion and friends try to handle both
       lists of symbols and anonymous lambda expressions.

   Looking at the code, I don't think they do.  It does not try to
   handle a list of symbols--and, because of the conflict you mentioned,
   it should not try.

   I think we should document that a simple list of symbols is NOT
   allowed, but that you can add "" as the first element to make that
   list into a legitimate completion table.

   Can you fix your custom thread patch to do that?

My latest revised patches no longer make `custom-do-theme-reset'
interactive, so that the problem no longer exists.  (But it could
easily crop up in other places, using lists of symbols for completion
appears to be natural).

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Richard Stallman wrote:

       The problem is that try-completion and friends try to handle both
       lists of symbols and anonymous lambda expressions.

   Looking at the code, I don't think they do.  It does not try to
   handle a list of symbols--and, because of the conflict you mentioned,
   it should not try.

   I think we should document that a simple list of symbols is NOT
   allowed, but that you can add "" as the first element to make that
   list into a legitimate completion table.

But what about the other problem I mentioned.  Should
`test-completion' not one way or the other be made consistent with
`try-completion' and `all-completions'.  Kim adapted the latter two to
handle symbols in alists and hash tables, but not test-completion.
Thereby, the former symmetry between the three is gone.

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    My latest revised patches no longer make `custom-do-theme-reset'
    interactive, so that the problem no longer exists.  (But it could
    easily crop up in other places, using lists of symbols for completion
    appears to be natural).

Could you fix the doc strings, and the manual, to explain that a list
of symbols needs "" at the front in order to be valid as a completion
table?

    But what about the other problem I mentioned.  Should
    `test-completion' not one way or the other be made consistent with
    `try-completion' and `all-completions'.  Kim adapted the latter two to
    handle symbols in alists and hash tables, but not test-completion.
    Thereby, the former symmetry between the three is gone.

I agree, test-completion should be fixed.  Would you like
to fix that?

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Richard Stallman wrote:

   Could you fix the doc strings, and the manual, to explain that a list
   of symbols needs "" at the front in order to be valid as a completion
   table?

Yes, but let me first make sure I understand.  Is the reason that we
do not want _any_ symbol as car of the list that forbidding _any_
symbol as car of the list actually seems _more natural_ than just
forbidding lambda?  It seems that any symbol other than lambda can not
be mistaken for the car of an anonymous lambda expresion and hence
could not lead to ambiguity.

To make sure that there is no confusion:

ELISP> (try-completion "b" '(aa bb))
*** Eval error ***  Invalid function: (aa bb)

Note that `try-completion' does not believe that aa is a function
called with argument bb.  It believes that (aa bb) is an anonymous
lambda expression.  It could know that it is not, because the car is
not lambda.  (That is what the patch I sent did.  It checked whether
the car was lambda.)

   I agree, test-completion should be fixed.  Would you like
   to fix that?

There are several ways to do that.  The easiest one would probably be
to make assoc-string be able to handle symbols as well as strings.
But maybe that would be too radical?

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> ELISP> (try-completion "b" '(aa bb))
> *** Eval error ***  Invalid function: (aa bb)
>
> Note that `try-completion' does not believe that aa is a function
> called with argument bb.  It believes that (aa bb) is an anonymous
> lambda expression.  It could know that it is not, because the car is
> not lambda.  (That is what the patch I sent did.  It checked whether
> the car was lambda.)

I agree that checking for lambda is better -- it makes the ordinary
cases work without hacks, so only in the case where lambda itself is
in the list, special attention is needed.

>
>    I agree, test-completion should be fixed.  Would you like
>    to fix that?
>
> There are several ways to do that.  The easiest one would probably be
> to make assoc-string be able to handle symbols as well as strings.
> But maybe that would be too radical?

It could break existing code if somebody already abuses assoc-string to
find a string in a mixed list of strings and symbols.

But I'd say it would be ok to change it as you suggest.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Bugs in newly added completion capabilities.

[Miles Bader]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:
> Yes, but let me first make sure I understand.  Is the reason that we
> do not want _any_ symbol as car of the list that forbidding _any_
> symbol as car of the list actually seems _more natural_ than just
> forbidding lambda?  It seems that any symbol other than lambda can not
> be mistaken for the car of an anonymous lambda expresion and hence
> could not lead to ambiguity.

One possible reason is that if we allow almost all symbol lists, people
will tend to overlook the need for a `lambda' special case, write their
code to use straight-forward symbol lists -- and odd bugs will arise
when lambda does happen to occur at the beginning of such a list.

Always requiring an initial "" forces the problem to be dealt with, so
will make such code more robust.

-Miles
-- 
97% of everything is grunge

Re: Bugs in newly added completion capabilities.

[Kim F. Storm]

Miles Bader <miles@lsi.nec.co.jp> writes:

> Always requiring an initial "" forces the problem to be dealt with, so
> will make such code more robust.

True.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Miles Bader wrote:

   Always requiring an initial "" forces the problem to be dealt with, so
   will make such code more robust.

Actually, in my own use of it, I would probably have needed the:

(cons "" custom-loaded-themes)

anyway, because a theme could be named `lambda'.

So I guess that I will just document the "" stuff.

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Stefan Monnier]

>> Yes, but let me first make sure I understand.  Is the reason that we
>> do not want _any_ symbol as car of the list that forbidding _any_
>> symbol as car of the list actually seems _more natural_ than just
>> forbidding lambda?  It seems that any symbol other than lambda can not
>> be mistaken for the car of an anonymous lambda expresion and hence
>> could not lead to ambiguity.

> One possible reason is that if we allow almost all symbol lists, people
> will tend to overlook the need for a `lambda' special case, write their
> code to use straight-forward symbol lists -- and odd bugs will arise
> when lambda does happen to occur at the beginning of such a list.

> Always requiring an initial "" forces the problem to be dealt with, so
> will make such code more robust.

I don't like the idea of changing assoc-string to also match symbols.
Maybe it won't lead to any catastrophe, but it just sounds wrong.

I don't like the idea of adding "" at the beginning of a completion table.
Here I have a good reason, which is that it changes the behavior:

   (try-completion "" '("aaa" "aab" "aac"))  => "aa"
   (try-completion "" '("" "aaa" "aab" "aac"))  => ""

Honestly, what's so great about being able to use lists of symbols rather
than lists of strings?  In Emacs-21, we don't even allow lists of strings,
but only lists of pairs whose car is a string, and people haven't complained
about it.


        Stefan

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Stefan Monnier wrote:

   Honestly, what's so great about being able to use lists of symbols rather
   than lists of strings?  In Emacs-21, we don't even allow lists of strings,
   but only lists of pairs whose car is a string, and people haven't complained
   about it.

I am not arguing for allowing lists of symbols rather than lists of
strings, but mixed lists of strings and symbols _are_ currently allowed
and try-completion and all-completions now nearly everywhere treat
symbols as being completely equivalent to (symbol-name 'symbol)
since the following change made relatively recently (I thought we were
in a feature freeze, but anyway):

2005-02-22  Kim F. Storm  <storm@cua.dk>

	    * minibuf.c (Ftry_completion, Fall_completions): Allow
              both string and symbol keys in alists and hash tables.

Again I am not arguing for or against the desirability of this change,
I am trying to deal with its consequences.

There are two problems: the _nearly_ everywhere is very inconsistent
and the exceptions at first seem totally arbitrary, so it should be
clearly documented what the exceptions are and what the motivation for
the exceptions is.

Secondly, try-completion and all-completions are mainly used by
minibuffer completion functions that can also pass the same argument
to test-completion.  If test-completion treats this argument
differently from the other two (as it currently does), there is the
obvious potential for trouble.

So either one just reverts the 2005-02-22 change, or one needs to
take care of the inconsistency problems I pointed out.

   I don't like the idea of adding "" at the beginning of a completion table.
   Here I have a good reason, which is that it changes the behavior:

      (try-completion "" '("aaa" "aab" "aac"))  => "aa"
      (try-completion "" '("" "aaa" "aab" "aac"))  => ""

Of course, the person consing "" to the front of the completion table
will have to write his code to expect the second answer.

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Luc Teirlinck]

Stefan Monnier wrote:

   I don't like the idea of adding "" at the beginning of a completion table.
   Here I have a good reason, which is that it changes the behavior:

      (try-completion "" '("aaa" "aab" "aac"))  => "aa"
      (try-completion "" '("" "aaa" "aab" "aac"))  => ""

ELISP> (try-completion "" '([] "aaa" "aab" "aac"))
"aa"

So, [] should be consed to the front of the list.

Sincerely,

Luc.

Re: Bugs in newly added completion capabilities.

[Stefan Monnier]

> I am not arguing for allowing lists of symbols rather than lists of
> strings, but mixed lists of strings and symbols _are_ currently allowed
> and try-completion and all-completions now nearly everywhere treat symbols
> as being completely equivalent to (symbol-name 'symbol) since the
> following change made relatively recently (I thought we were in a feature
> freeze, but anyway):

I understand that.  I was just arguing against that recent change.

>    I don't like the idea of adding "" at the beginning of a completion table.
>    Here I have a good reason, which is that it changes the behavior:

>       (try-completion "" '("aaa" "aab" "aac"))  => "aa"
>       (try-completion "" '("" "aaa" "aab" "aac"))  => ""

> Of course, the person consing "" to the front of the completion table
> will have to write his code to expect the second answer.

What if the code *needs* the first answer?


        Stefan

Re: Bugs in newly added completion capabilities.

[Stefan Monnier]

>    I don't like the idea of adding "" at the beginning of a completion table.
>    Here I have a good reason, which is that it changes the behavior:

>       (try-completion "" '("aaa" "aab" "aac"))  => "aa"
>       (try-completion "" '("" "aaa" "aab" "aac"))  => ""

ELISP> (try-completion "" '([] "aaa" "aab" "aac"))
> "aa"

> So, [] should be consed to the front of the list.

Yuck,


        Stefan

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    Yes, but let me first make sure I understand.  Is the reason that we
    do not want _any_ symbol as car of the list that forbidding _any_
    symbol as car of the list actually seems _more natural_ than just
    forbidding lambda?

Right.  It is not clean to make an exception for "lambda".
It is clean to make an exception for lists of symbols.

       I agree, test-completion should be fixed.  Would you like
       to fix that?

    There are several ways to do that.  The easiest one would probably be
    to make assoc-string be able to handle symbols as well as strings.
    But maybe that would be too radical?

I think that would be ok.  Since its basic purpose is to operate on strings,
it could operate on symbols as if they were strings.

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    I don't like the idea of adding "" at the beginning of a completion table.
    Here I have a good reason, which is that it changes the behavior:

       (try-completion "" '("aaa" "aab" "aac"))  => "aa"
       (try-completion "" '("" "aaa" "aab" "aac"))  => ""

Oops.  I suggested adding "" because I thought it would not change
the results.  Since that is not true, adding "" is not a solution.

The only real solution would be to add a first element that's neither
a symbol nor a string, nor a cons cell whose car is a symbol or a
string.  Such as 0 or [].  That is sort of ugly.  It would be cleaner
to say that lists of symbols can't be used at all.  As you've said,
that would be no great loss.

Let's recall how this came up: as a side effect of the change to allow
symbols as the car of cons cells in an alist.  We could allow symbols
when they come from the car of an element, and not allow them when
they don't come from there.

Any objections?

Re: Bugs in newly added completion capabilities.

[Miles Bader]

2005/7/1, Richard M. Stallman <rms@gnu.org>:
> Let's recall how this came up: as a side effect of the change to allow
> symbols as the car of cons cells in an alist.  We could allow symbols
> when they come from the car of an element, and not allow them when
> they don't come from there.

I think it's only ambiguous if you used symbols in a _mixed_ list --
one with both plain symbols and cons-cells with a symbol as their car.
 If the elements of the list are consistently one or the other, the
pattern (lambda (arg ...) ...) can never occur.

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Bugs in newly added completion capabilities.

[David Kastrup]

Miles Bader <snogglethorpe@gmail.com> writes:

> 2005/7/1, Richard M. Stallman <rms@gnu.org>:
>> Let's recall how this came up: as a side effect of the change to allow
>> symbols as the car of cons cells in an alist.  We could allow symbols
>> when they come from the car of an element, and not allow them when
>> they don't come from there.
>
> I think it's only ambiguous if you used symbols in a _mixed_ list --
> one with both plain symbols and cons-cells with a symbol as their car.
>  If the elements of the list are consistently one or the other, the
> pattern (lambda (arg ...) ...) can never occur.

(lambda nil t) is a valid function.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Bugs in newly added completion capabilities.

[Miles Bader]

David Kastrup <dak@gnu.org> writes:
>>  If the elements of the list are consistently one or the other, the
>> pattern (lambda (arg ...) ...) can never occur.
>
> (lambda nil t) is a valid function.

Not in this case -- the completion function is passed 3 arguments.

-Miles
-- 
"Nah, there's no bigger atheist than me.  Well, I take that back.
I'm a cancer screening away from going agnostic and a biopsy away
from full-fledged Christian."  [Adam Carolla]

Re: Bugs in newly added completion capabilities.

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> since the following change made relatively recently (I thought we were
> in a feature freeze, but anyway):
>
> 2005-02-22  Kim F. Storm  <storm@cua.dk>
>
> 	    * minibuf.c (Ftry_completion, Fall_completions): Allow
>               both string and symbol keys in alists and hash tables.
>
> Again I am not arguing for or against the desirability of this change,
> I am trying to deal with its consequences.

I changed them to support the following change:

2005-02-22  Kim F. Storm  <storm@cua.dk>

	* progmodes/hideif.el (hide-ifdef-use-define-alist):
	Use completing-read.  Suggested by Juan-Leon Lahoz Garcia.


> There are two problems: the _nearly_ everywhere is very inconsistent
> and the exceptions at first seem totally arbitrary, so it should be
> clearly documented what the exceptions are and what the motivation for
> the exceptions is.

I don't think there were any explicit motivation (rather oversight on
my part) -- better fix the code to DTRT.  Please do.

>    I don't like the idea of adding "" at the beginning of a completion table.
>    Here I have a good reason, which is that it changes the behavior:
>
>       (try-completion "" '("aaa" "aab" "aac"))  => "aa"
>       (try-completion "" '("" "aaa" "aab" "aac"))  => ""
>
> Of course, the person consing "" to the front of the completion table
> will have to write his code to expect the second answer.

Perhaps a check to see if first elt is lambda and second elt is a list
would be a reasonable way to recognize a lambda form.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Bugs in newly added completion capabilities.

[Kim F. Storm]

"Richard M. Stallman" <rms@gnu.org> writes:

> The only real solution would be to add a first element that's neither
> a symbol nor a string, nor a cons cell whose car is a symbol or a
> string.  Such as 0 or [].  That is sort of ugly.  It would be cleaner
> to say that lists of symbols can't be used at all.  As you've said,
> that would be no great loss.
>
> Let's recall how this came up: as a side effect of the change to allow
> symbols as the car of cons cells in an alist.  We could allow symbols
> when they come from the car of an element, and not allow them when
> they don't come from there.
>
> Any objections?

IMO, it looks like another arbitrary exception -- we allow a list of
strings, but not a list of symbols.

But I wont object...

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Bugs in newly added completion capabilities.

[Kim F. Storm]

Miles Bader <snogglethorpe@gmail.com> writes:

> 2005/7/1, Richard M. Stallman <rms@gnu.org>:
>> Let's recall how this came up: as a side effect of the change to allow
>> symbols as the car of cons cells in an alist.  We could allow symbols
>> when they come from the car of an element, and not allow them when
>> they don't come from there.
>
> I think it's only ambiguous if you used symbols in a _mixed_ list --
> one with both plain symbols and cons-cells with a symbol as their car.
>  If the elements of the list are consistently one or the other, the
> pattern (lambda (arg ...) ...) can never occur.

And the lambda always has 3 args in this context, so (lambda nil ...)
will never occur.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Bugs in newly added completion capabilities.

[Stefan Monnier]

>> since the following change made relatively recently (I thought we were
>> in a feature freeze, but anyway):
>> 
>> 2005-02-22  Kim F. Storm  <storm@cua.dk>
>> 
>> * minibuf.c (Ftry_completion, Fall_completions): Allow
>> both string and symbol keys in alists and hash tables.
>> 
>> Again I am not arguing for or against the desirability of this change,
>> I am trying to deal with its consequences.

> I changed them to support the following change:

> 2005-02-22  Kim F. Storm  <storm@cua.dk>

> 	* progmodes/hideif.el (hide-ifdef-use-define-alist):
> 	Use completing-read.  Suggested by Juan-Leon Lahoz Garcia.

Then I simply suggest the patch below.  Then we can revert your change and
forget about this idea of allowing symbols in completion lists.

I mean, honestly, why make the primitives try-completion, all-completions,
and test-completions yet more hairy for this one particular case, which is
not even an often used piece of code and where the workaround won't even
lead to any noticeable performance hit, seeing how hide-ifdef-define-alist
is unlikely to grow large.


        Stefan


--- hideif.el	04 avr 2005 09:53:32 -0400	1.50
+++ hideif.el	01 jui 2005 10:53:14 -0400	
@@ -960,7 +960,9 @@
   "Set `hide-ifdef-env' to the define list specified by NAME."
   (interactive
    (list (completing-read "Use define list: "
-			  hide-ifdef-define-alist nil t)))
+			  (mapcar (lambda (x) (symbol-name (car x)))
+                                  hide-ifdef-define-alist)
+                          nil t)))
   (if (stringp name) (setq name (intern name)))
   (let ((define-list (assoc name hide-ifdef-define-alist)))
     (if define-list

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    > I think it's only ambiguous if you used symbols in a _mixed_ list --
    > one with both plain symbols and cons-cells with a symbol as their car.
    >  If the elements of the list are consistently one or the other, the
    > pattern (lambda (arg ...) ...) can never occur.

    And the lambda always has 3 args in this context, so (lambda nil ...)
    will never occur.

Ok, would people like to implement this?

Re: Bugs in newly added completion capabilities.

[Richard M. Stallman]

    Then I simply suggest the patch below.  Then we can revert your change and
    forget about this idea of allowing symbols in completion lists.

    I mean, honestly, why make the primitives try-completion, all-completions,
    and test-completions yet more hairy for this one particular case, which is
    not even an often used piece of code and where the workaround won't even
    lead to any noticeable performance hit, seeing how hide-ifdef-define-alist
    is unlikely to grow large.

It seems ok to me.  This feature of completion tables is not important
for users, and I would not mind if we abandon it.