bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Vitalie Spinu]

Hi, 

Try (read-number "Number: ") and insert some non-numeric junk. The
expected behavior is for the read-number to recognize the faulty string
and ask again, like documented in (elisp) Interactive Codes:

   `n'
        A number, read with the minibuffer.  If the input is not a number,
        the user has to try again. 


This doesn't happen because read-number relies on string-to-number to
throw an error, which presumably was happening some time ago. Now
(string-to-number "junk") returns 0.

    Vitalie


In GNU Emacs 24.3.1 (i686-pc-linux-gnu, GTK+ Version 2.24.13)
 of 2013-03-12 on vitoshka-home
Windowing system distributor `The X.Org Foundation', version 11.0.11300000
System Description:	Ubuntu 12.10

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number toblame)

[Drew Adams]

Yes, the regression was introduced in Emacs 24.3.

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Andreas Schwab]

Vitalie Spinu <spinuvit@gmail.com> writes:

> Now (string-to-number "junk") returns 0.

It always did, as documented.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number toblame)

[Drew Adams]

> > Now (string-to-number "junk") returns 0.
> 
> It always did, as documented.

The regression comes from this change in `read-number' (not from a change in
`string-to-number'):

Old (good): (read str)
New (bad) : (string-to-number str)

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Vitalie Spinu]

 >> Andreas Schwab <schwab@linux-m68k.org>
 >> on Wed, 24 Apr 2013 16:21:28 +0200 wrote:

 > Vitalie Spinu <spinuvit@gmail.com> writes:
 >> Now (string-to-number "junk") returns 0.

 > It always did, as documented.

That presumably means that read-number never worked as originally
expected. The comment in source file says that read-number should be
used for "n" interactive spec, but "n" spec works as expected, so
read-number is not used there.

Wouldn't it be more natural for string-to-number to return nil rather
than 0 in a non-number case?

    Vitalie 

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number toblame)

[Drew Adams]

>  >> Now (string-to-number "junk") returns 0.
> 
>  > It always did, as documented.
> 
> That presumably means that read-number never worked as originally
> expected. The comment in source file says that read-number should be
> used for "n" interactive spec, but "n" spec works as expected, so
> read-number is not used there.
> 
> Wouldn't it be more natural for string-to-number to return nil rather
> than 0 in a non-number case?

See my last reply.  It's not about `string-to-number'.  It's about
`read-number'.  

And no, it has always worked as expected, up through Emacs 24.2.  It was broken
in 24.3, by changing (read str) to (string-to-number str).

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Glenn Morris]

Vitalie Spinu wrote:

> This doesn't happen because read-number relies on string-to-number to
> throw an error, which presumably was happening some time ago.

No, it used to use "read", prior to
http://lists.gnu.org/archive/html/emacs-diffs/2012-07/msg00477.html

  Replace `read' with `string-to-number' for consistency with
  `number-to-string'.

I don't know why that mattered, it seems to bear no relation to the rest
of the change. Going back to "read" again will fix this.
Done in emacs-24.

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Juri Linkov]

> Try (read-number "Number: ") and insert some non-numeric junk. The
> expected behavior is for the read-number to recognize the faulty string
> and ask again, like documented in (elisp) Interactive Codes:
>
>    `n'
>         A number, read with the minibuffer.  If the input is not a number,
>         the user has to try again.

`call-interactively' doesn't use `read-number'.  It duplicates code
from `read-number' with a similar loop to re-read non-numbers.
However, it uses `read' instead of `string-to-number'.
So I agree it's better to revert the regression and to use `read'
in both `read-number' and `call-interactively' for consistency.

BTW, while comparing `read-number' and `call-interactively'
I noticed a difference between them.  Try to evaluate:

(defun read-num (n)
  (interactive "nNumber: ")
  (message "Number: %s" n))

then `M-x read-num RET non-number RET' clears the prompt to the empty string.

This is because `callint_message' is a global variable whose value gets
cleared while reading a number from the minibuffer recursively.
It should be re-initialized before re-reading the next number.

Since this bug is not a regression, I propose to install this patch to trunk:

=== modified file 'src/callint.c'
--- src/callint.c	2013-02-27 07:42:43 +0000
+++ src/callint.c	2013-04-24 20:54:52 +0000
@@ -692,6 +692,11 @@ (at your option) any later version.
 		  {
 		    message1 ("Please enter a number.");
 		    sit_for (make_number (1), 0, 0);
+		    /* Re-initialize callint_message for next iteration.  */
+		    if (strchr (SSDATA (visargs[0]), '%'))
+		      callint_message = Fformat (i, visargs);
+		    else
+		      callint_message = visargs[0];
 		  }
 		first = 0;
 

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Stefan Monnier]

> `call-interactively' doesn't use `read-number'.  It duplicates code
> from `read-number' with a similar loop to re-read non-numbers.

Could you try and see if/how the C code could be changed to just call
the Elisp function?


        Stefan

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Juri Linkov]

>> `call-interactively' doesn't use `read-number'.  It duplicates code
>> from `read-number' with a similar loop to re-read non-numbers.
>
> Could you try and see if/how the C code could be changed to just call
> the Elisp function?

I tried and see no problems with this patch:

=== modified file 'src/callint.c'
--- src/callint.c	2013-02-27 07:42:43 +0000
+++ src/callint.c	2013-04-25 20:41:12 +0000
@@ -34,6 +34,7 @@ (at your option) any later version.
 static Lisp_Object Qenable_recursive_minibuffers;
 
 static Lisp_Object Qhandle_shift_selection;
+static Lisp_Object Qread_number;
 
 Lisp_Object Qmouse_leave_buffer_hook;
 
@@ -683,29 +684,7 @@ (at your option) any later version.
 	  if (!NILP (prefix_arg))
 	    goto have_prefix_arg;
 	case 'n':		/* Read number from minibuffer.  */
-	  {
-	    bool first = 1;
-	    do
-	      {
-		Lisp_Object str;
-		if (! first)
-		  {
-		    message1 ("Please enter a number.");
-		    sit_for (make_number (1), 0, 0);
-		  }
-		first = 0;
-
-		str = Fread_from_minibuffer (callint_message,
-					     Qnil, Qnil, Qnil, Qnil, Qnil,
-					     Qnil);
-		if (! STRINGP (str) || SCHARS (str) == 0)
-		  args[i] = Qnil;
-		else
-		  args[i] = Fread (str);
-	      }
-	    while (! NUMBERP (args[i]));
-	  }
-	  visargs[i] = args[i];
+	  args[i] = call1 (Qread_number, callint_message);
 	  break;
 
 	case 'P':		/* Prefix arg in raw form.  Does no I/O.  */
@@ -903,6 +882,7 @@ (at your option) any later version.
   DEFSYM (Qminus, "-");
   DEFSYM (Qplus, "+");
   DEFSYM (Qhandle_shift_selection, "handle-shift-selection");
+  DEFSYM (Qread_number, "read-number");
   DEFSYM (Qcall_interactively, "call-interactively");
   DEFSYM (Qcommand_debug_status, "command-debug-status");
   DEFSYM (Qenable_recursive_minibuffers, "enable-recursive-minibuffers");

=== modified file 'lisp/subr.el'
--- lisp/subr.el	2013-04-18 00:12:33 +0000
+++ lisp/subr.el	2013-04-25 20:41:35 +0000
@@ -2200,11 +2200,11 @@ (defun read-passwd (prompt &optional con
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
-;; This should be used by `call-interactively' for `n' specs.
 (defun read-number (prompt &optional default)
   "Read a numeric value in the minibuffer, prompting with PROMPT.
 DEFAULT specifies a default value to return if the user just types RET.
-The value of DEFAULT is inserted into PROMPT."
+The value of DEFAULT is inserted into PROMPT.
+This function is used by the `interactive' code letter `n'."
   (let ((n nil)
 	(default1 (if (consp default) (car default) default)))
     (when default1

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Stefan Monnier]

>>> `call-interactively' doesn't use `read-number'.  It duplicates code
>>> from `read-number' with a similar loop to re-read non-numbers.
>> Could you try and see if/how the C code could be changed to just call
>> the Elisp function?
> I tried and see no problems with this patch:

Thank you, looks good: please install,


        Stefan

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Juri Linkov]

>>> `call-interactively' doesn't use `read-number'.  It duplicates code
>>> from `read-number' with a similar loop to re-read non-numbers.
>>
>> Could you try and see if/how the C code could be changed to just call
>> the Elisp function?
>
> === modified file 'src/callint.c'
> --- src/callint.c	2013-02-27 07:42:43 +0000
> +++ src/callint.c	2013-04-25 20:41:12 +0000
> [...]
> -	  visargs[i] = args[i];
> +	  args[i] = call1 (Qread_number, callint_message);
>  	  break;

I should have mentioned that original code contained the line

          visargs[i] = args[i];

but I omitted it in the change since it has no effect
because this code at the end of `Fcall_interactively'

      for (i = 1; i < nargs; i++)
        {
          if (varies[i] > 0)
            visargs[i] = Fcons (intern (callint_argfuns[varies[i]]), Qnil);
          else
            visargs[i] = quotify_arg (args[i]);
        }

overwrites elements of `visargs' anyway.  I don't understand why
`Fcall_interactively' contains many lines of such useless code as

	  visargs[i] = last_minibuf_string;

If the intention was to collect strings in `visargs' and use them later
then old code for numbers (currently still useless) was wrong,
it should convert numbers to strings with something like

          visargs[i] = Fnumber_to_string(args[i]);

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Stefan Monnier]

> I should have mentioned that original code contained the line
>           visargs[i] = args[i];
> but I omitted it in the change since it has no effect
> because this code at the end of `Fcall_interactively'

>       for (i = 1; i < nargs; i++)
>         {
>           if (varies[i] > 0)
>             visargs[i] = Fcons (intern (callint_argfuns[varies[i]]), Qnil);
>           else
>             visargs[i] = quotify_arg (args[i]);
>         }

Here's what's going on:
visargs is used in two different ways, just in order to avoid allocating
a third array:
- inside the loop, visargs keeps a representation of the arguments,
  which is used when the prompt of an argument contains a % (in which
  case it's passed to `format').  This is a rarely used feature which
  can be seen in:

   % grep '(interactive ".*%' lisp/**/*.el
   lisp/abbrev.el:  (interactive "sDefine global abbrev: \nsExpansion for %s: ")
   lisp/abbrev.el:  (interactive "sDefine mode abbrev: \nsExpansion for %s: ")
   lisp/dired-x.el:  (interactive "FRelSymLink: \nFRelSymLink %s: \np")
   lisp/mail/mailabbrev.el:  (interactive "sDefine mail alias: \nsDefine %s as mail alias for: ")
   lisp/mail/mailalias.el:  (interactive "sDefine mail alias: \nsDefine %s as mail alias for: ")
   lisp/net/ange-ftp.el:  (interactive "fCopy file: \nFCopy %s to file: \np")
   lisp/net/ange-ftp.el:  (interactive "fRename file: \nFRename %s to file: \np")
   lisp/subr.el:  (interactive "KSet key globally: \nCSet key %s to command: ")
   lisp/subr.el:  (interactive "KSet key locally: \nCSet key %s locally to command: ")
   %

  This is the reason why it's called "visargs" because it contains
  a representation of the argument which should be appropriate for
  display (e.g. it keeps a key-description instead of a key).

  FWIW, I think this is a misfeature.  Better force people to use a Lisp
  form for the interactive spec when they need access to previous args
  while building subsequent args.

- after the loop, this is not needed any more, but the same array is
  reused to build the arguments to pass to `Flist' to make the entry to
  add in command-history.

> I don't understand why `Fcall_interactively' contains many lines of
> such useless code as
> 	  visargs[i] = last_minibuf_string;
> If the intention was to collect strings in `visargs' and use them later
> then old code for numbers (currently still useless) was wrong,
> it should convert numbers to strings with something like
>           visargs[i] = Fnumber_to_string(args[i]);

I don't understand all those "visargs[i] = last_minibuf_string;" either,
because visargs doesn't need to contain only strings, since format's %s
will handle non-strings.


        Stefan

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Juri Linkov]

> Here's what's going on:
> visargs is used in two different ways, just in order to avoid allocating
> a third array:
> - inside the loop, visargs keeps a representation of the arguments,
>   which is used when the prompt of an argument contains a % (in which
>   case it's passed to `format').

Thanks for the explanation.  So I added visargs[i] for numbers.

IIUC, converting to the string with `Fnumber_to_string'
is not needed, but I still added it for consistency with
other interactive code letters like `Fchar_to_string' for `c'.

Also I don't know what compiler bug is stimulated by passing
args[i] directly, so I just copied code from the code letter `c'.

Now it should work for this test case:

    (defun test (n s)
      (interactive "nNumber: \nsString for %s: "))

bug#14254: 24.3; read-number fails to recognize faulty numbers (string-to-number to blame)

[Stefan Monnier]

> Also I don't know what compiler bug is stimulated by passing
> args[i] directly, so I just copied code from the code letter `c'.

This workaround has been in the file since 1991 (original commit), so
I hope noone uses this compiler nowadays.


        Stefan