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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14254 in the body.
You can then email your comments to 14254 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vitalie Spinu <spinuvit <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 14:46:50 +0200

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

Message #8 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Vitalie Spinu'" <spinuvit <at> gmail.com>, <14254 <at> debbugs.gnu.org>
Subject: RE: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number
	toblame)
Date: Wed, 24 Apr 2013 06:09:18 -0700

Yes, the regression was introduced in Emacs 24.3.

Message #11 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Vitalie Spinu <spinuvit <at> gmail.com>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 16:21:28 +0200

Vitalie Spinu <spinuvit <at> gmail.com> writes:

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

It always did, as documented.

Andreas.

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

Message #14 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Andreas Schwab'" <schwab <at> linux-m68k.org>,
	"'Vitalie Spinu'" <spinuvit <at> gmail.com>
Cc: 14254 <at> debbugs.gnu.org
Subject: RE: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number
	toblame)
Date: Wed, 24 Apr 2013 07:44:50 -0700

> > 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)

Message #17 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Vitalie Spinu <spinuvit <at> gmail.com>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 16:48:04 +0200

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

 > Vitalie Spinu <spinuvit <at> 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 

Message #20 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Vitalie Spinu'" <spinuvit <at> gmail.com>,
	"'Andreas Schwab'" <schwab <at> linux-m68k.org>
Cc: 14254 <at> debbugs.gnu.org
Subject: RE: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number
	toblame)
Date: Wed, 24 Apr 2013 08:13:41 -0700

>  >> 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).

Message #23 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Vitalie Spinu <spinuvit <at> gmail.com>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 13:29:18 -0400

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.

Message #28 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Vitalie Spinu <spinuvit <at> gmail.com>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 23:57:18 +0300

> 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;
 

Message #31 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 14254 <at> debbugs.gnu.org, Vitalie Spinu <spinuvit <at> gmail.com>
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Wed, 24 Apr 2013 23:33:54 -0400

> `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

Message #34 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14254 <at> debbugs.gnu.org, Vitalie Spinu <spinuvit <at> gmail.com>
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Thu, 25 Apr 2013 23:44:45 +0300

>> `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

Message #37 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 14254 <at> debbugs.gnu.org, Vitalie Spinu <spinuvit <at> gmail.com>
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Thu, 25 Apr 2013 21:46:12 -0400

>>> `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

Message #40 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Tue, 07 May 2013 11:42:41 +0300

>>> `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]);

Message #43 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Tue, 07 May 2013 09:38:02 -0400

> 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

Message #46 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3;
	read-number fails to recognize faulty numbers (string-to-number to
	blame)
Date: Tue, 07 May 2013 23:51:01 +0300

> 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: "))

Message #49 received at 14254 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 14254 <at> debbugs.gnu.org
Subject: Re: bug#14254: 24.3; read-number fails to recognize faulty numbers
	(string-to-number to blame)
Date: Tue, 07 May 2013 17:31:29 -0400

> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.