Bug in completing-read

Bug in completing-read

[Luc Teirlinck]

If one evaluates:

(read-from-minibuffer "Input: " '("123456789" . 3))

point appears on the "3" in the minibuffer.

But if one evaluates:

(completing-read "Input: " '("123456789") nil nil '("123456789" . 3))

point appears on the "4" in the minibuffer.

This is inconsistent and it also contradicts the documentation string
of `completing-read'.  In other words, the behavior of
`completing-read' is an obvious bug.  It is trivial to fix, in fact I
"inadvertently" fixed it in my own private Emacs.  

In another thread ("read-from-minibuffer and friends"), I proposed a
patch that will move the handling of string-integer conses of the type
used above from Fread_from_minibuffer into read_minibuf.  (Not yet
installed because of the Savannah situation.)  After proposing that
patch I made some small further changes.  (I got rid of variables that
were no longer used, but which I inadvertently forgot to remove.)  But
then I noticed nearly identical code in Fcompleting_read.  I removed
this now redundant code.  But while diffing I noticed that a "-1" was
missing in a formula in the removed code.  At first sight, wonderful.
Code gone, bug gone, end of story.  Now the problem.

People using such arguments to `completing-read' apparently found it
easier to just get around the bug by putting the "-1" in their own
code, rather than reporting it or fixing it in minibuf.c.

So now we are left with the following choice:

1.  Document this inconsistency in the documentation string of
    `completing-read', as well as in the Elisp manual, and say that it
    exists for historical compatibility.  (I could do that, if that is
    what we decide to do.)

2.  Fix the bug and also fix existing code that relies on the bug.  (I
    could trivially fix the bug, if that is what we decide to do.  As
    mentioned above, I have already fixed it in my personal Emacs.
    Finding _all_ code relying on the bug seems very non-trivial,
    however, because `completing-read' is used so often.)

Example:

M-x load-library RET ffap

M-: (ffap-read-file-or-url "Input: " "myfile") RET

Result:

Input: ~/emacscvsdir/emacs/lisp/myfile

with point on the `m' of myfile, as probably intended.  Fixing the bug
in `completing-read' would put it on the last `/'.

So do we fix this bug or make the inconsistency an "official, but
deprecated, feature" by documenting it?

Sincerely,

Luc.

Re: Fixed bug in completing-read

[Luc Teirlinck]

I have fixed (in CVS) the bug I in `completing-read' which I reported
in my previous message.  Some packages may _rely_ on the bug.  This
concerns the initial position of point in an inserted default string
after entering the minibuffer.  In the (rare) case where the INITIAL
argument to `completing-read' is a cons of a string and an integer (as
opposed to just a string),`completing-read' used to put point one
positiom more to the right than `read-from-minibuffer', contradicting
its own documentation string.

This is now fixed.  There is no change in the (usual) case where
INITIAL is just a string: point appears at the end of the inserted
string.  In nearly all cases where initial is a cons, the purpose was
to make point appear at the _beginning_ of the inserted string.  To
get around the bug, many packages specified a position of 0 instead of
1.  This is *no problem*.  _Any_ integer 1 _or_ less puts point at the
beginning of the string.  The *only* time there is a problem is when
one tries to put point somewhere in the _middle_ of the inserted
string.  In that case, point will now appear one position more to the
_left_ than intended.  This can trivially be fixed by adding 1 to the
formula computing the position.  I fixed the one single example I knew
of.  However, the number of calls to `completing-read' is so huge and
many calls are so complex, that it is impossible to be certain that
there are no other cases.  So, if after entry to the minibuffer, on a
function that provides completion, point appears one position too far
to the left in the pre-inserted string, you know what the problem is
and it is trivial to fix.

Sincerely,

Luc.

Re: Fixed bug in completing-read

[Andreas Schwab]

Luc Teirlinck <teirllm@auburn.edu> writes:

> I have fixed (in CVS) the bug I in `completing-read' which I reported
> in my previous message.  Some packages may _rely_ on the bug.

This includes read-file-name, when called by find-alternate-file.

> In the (rare) case where the INITIAL
> argument to `completing-read' is a cons of a string and an integer (as
> opposed to just a string),`completing-read' used to put point one
> positiom more to the right than `read-from-minibuffer', contradicting
> its own documentation string.

No, it does not.  Compare the doc string of completing-read:

  If it is (STRING . POSITION), the initial input
  is STRING, but point is placed POSITION characters into the string.

with that of read-from-minibuffer:

  If INITIAL-CONTENTS is (STRING . POSITION), the initial input
  is STRING, but point is placed at position POSITION in the minibuffer.

"POSITION characters into string" == "at position POSITION in the
minibuffer" - 1.  String positions are zero-origin, buffer positions
are one-origin.

IMHO the behaviour of read-from-minibuffer is actually what should be
changed, because the Elisp manual describes it like this:

     Alternatively, INITIAL-CONTENTS can be a cons cell of the form
     `(STRING . POSITION)'.  This means to insert STRING in the
     minibuffer but put point POSITION characters from the beginning,
     rather than at the end.

which matches the (old) behaviour of completing-read, not
read-from-minibuffer.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fixed bug in completing-read

[Luc Teirlinck]

Andreas Schwab wrote:

   "POSITION characters into string" == "at position POSITION in the
   minibuffer" - 1.  String positions are zero-origin, buffer positions
   are one-origin.

To me, the English phrase "POSITION characters into string" is
equivalent with the technical Lisp phrase "at (zero-indexed) position
POSITION - 1 in string".  I could be wrong and, clearly, you
understand it differently.  The one thing this means for certain is
that things should be reformulated more unambiguously, in all involved
places, which I will do.

   IMHO the behaviour of read-from-minibuffer is actually what should be
   changed, because the Elisp manual describes it like this:

	Alternatively, INITIAL-CONTENTS can be a cons cell of the form
	`(STRING . POSITION)'.  This means to insert STRING in the
	minibuffer but put point POSITION characters from the beginning,
	rather than at the end.

Again, to me, the English phrase "POSITION characters from the
beginning" agrees with the behavior of read-from-minibuffer and
contradicts the old behavior of completing-read.  Again, I could be
wrong.  Same remark as above.

Anyway, terminology aside, we have three choices:

1. Adapt `completing-read' to `read-from-minibuffer'.  This is done in
   current CVS.

2. Adapt `read-from-minibuffer' to `completing-read', as you propose.

3. Keep the pre-yesterday behavior and clearly document the difference
   in behavior.

When I first brought this up, I offered to implement either (1) or
(3).  Richard decided on (1).  I could easily implement (2) too, if it
would be decided that this would be better.  In terms of "writing the
code", it just means adding or erasing a + 1 or -1, completely
trivial, and we do not need to undo the moving of the involved code
into read_minibuf, which was necessary, because certain functions
claimed they could handle conses for INITIAL, whereas they could not
(they can now).

Note that (2) would break existing code, just like (1) does, and it
would be equally difficult to find all that code.  (3) does not have
that problem, but the inconsistency is ugly and confusing.

If we decide to stay with (1), I would adapt `read-file-name'.

Maybe it might be good to move the duplicate (but currently, in as far
as I know, consistent) treatment of the HISTORY argument from
`read-from-minibuffer' and `completing-read' into `read_minibuf', as
was done for INITIAl, just to prevent future inconsistencies from
developing. I could easily do so, if desired.

Sincerely,

Luc.

Re: Fixed bug in completing-read

[Richard Stallman]

       "POSITION characters into string" == "at position POSITION in the
       minibuffer" - 1.  String positions are zero-origin, buffer positions
       are one-origin.

I agree.  "2 character into the string" means "after the first 2
characters".  When you put that text into a buffer, it would
correspond to buffer position 3, which is after the first 2
characters.

    To me, the English phrase "POSITION characters into string" is
    equivalent with the technical Lisp phrase "at (zero-indexed) position
    POSITION - 1 in string".

"2 characters into the string" certainly doesn't mean zero-indexed
position 1 in the string.

      The one thing this means for certain is
    that things should be reformulated more unambiguously, in all involved
    places, which I will do.

I agree that we should make it unambiguous.


Since this is a rather obscure feature, not used in too many places,
we should make the decision based on what is clean, not based on
compatibility with this or that.  Since these positions go with
strings, they should be zero-origin.

Re: Fixed bug in completing-read

[Luc Teirlinck]

Richard Stallman wrote:

   Since this is a rather obscure feature, not used in too many places,
   we should make the decision based on what is clean, not based on
   compatibility with this or that.  Since these positions go with
   strings, they should be zero-origin.

I have reverted my changes in `Fcompleting_read', as well as the
related change in ffap.el.  The entire treatment of conses as values
for INITIAL is back to where it was before my changes, except that a
few more functions can handle such values.

I do not know whether your remark above only applies to
`completing-read' (in which case it is completely taken care of) or
also to `read-from-minibuffer' and friends.  In the latter case, I
would have only one single objection, but a big one.  That is the
amount of work involved in adapting existing code.  The feature is
used more often than you seem to believe.  I already had my
reservations about changing things the other way around, but this way
things are a _lot_ worse.  (I took a look at it.)

The very existence of INITIAL is unclean (it competes with DEFAULT and
provides an inconsistent user interface) and it only exists for
historical compatibility.  Hence, I propose to just document the
inconsistency and say that the entire argument, inconsistencies and
all, only exists for historical compatibility anyway.  Just one more
reason to use DEFAULT instead.

A semi-obsolete argument is not worth tons of work.  Existing code
seems to use the argument based on the inconsistency, so no problem
there.  New code should not use it, so no problem there either.

Sincerely,

Luc.