Inconsistency in `string-to-number'

Inconsistency in `string-to-number'

[Davis Herring]

(string-to-number "1:") => 1
(string-to-number "1.2:") => 1
(string-to-number "1.2") => 1.2

Because of the call to isfloat_string in Fstring_to_number, the lenience
shown integers is not extended to floats, with the bizarre result that the
function succeeds but ignores part of its input.

Should there be a lenient form of isfloat_string that suppresses the check
for whitespace or NUL after the float, or is this not part of
string-to-number's "documented behavior"?  (It isn't actually documented
either to allow garbage after ints or to forbid it after floats.)

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Inconsistency in `string-to-number'

[Eli Zaretskii]

> Date: Thu, 23 Apr 2009 17:16:45 -0700 (PDT)
> From: "Davis Herring" <herring@lanl.gov>
> 
> (string-to-number "1:") => 1
> (string-to-number "1.2:") => 1
> (string-to-number "1.2") => 1.2

Looks like a bug to me.  Either (string-to-number "1:") should return
zero, as if the string was not a number at all, or
(string-to-number "1.2:") should return 1.2.

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

> Looks like a bug to me.  Either (string-to-number "1:") should return
> zero, as if the string was not a number at all

That, and clarifying the docstring, would be the most sensible
solution. Currently, "It ignores leading spaces and tabs." seems to
imply that nothing more is ignored. Also, "If the base used is not 10,
floating point is not recognized." is unclear about what the expected
result would be.

Obviously, any such change should be done post 23.1; there's almost
1,000 uses of `string-to-number' in lisp/*.el. It's hard to know how
many of them assume that "1:" will be interpreted as "1".

    Juanma

Re: Inconsistency in `string-to-number'

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 24 Apr 2009 13:45:50 +0200
> Cc: herring@lanl.gov, emacs-devel@gnu.org
> 
> Obviously, any such change should be done post 23.1; there's almost
> 1,000 uses of `string-to-number' in lisp/*.el. It's hard to know how
> many of them assume that "1:" will be interpreted as "1".

Which is why I thing we should fix it the other way around: to
recognize "1.2:" as a number.  That cannot possibly break anything, so
it should be safe to do before the release, IMO.

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

> Which is why I thing we should fix it the other way around: to
> recognize "1.2:" as a number. That cannot possibly break anything, so
> it should be safe to do before the release, IMO.

It's a long shot, but there could conceivably be code that relies in
truncation of "float + non-space", i.e., that expects ("1.2:" => 1).
So it's not true that it cannot possibly break anything; just
unlikely.

Yes, code that relies on "1.2:" => 1 is abusing a non-documented
quirk; but code that relies on "1:" => 1 is also doing the same thing.

(That said, I'm not going to push for one behavior over the other.)

    Juanma

Re: Inconsistency in `string-to-number'

[Stefan Monnier]

>> Looks like a bug to me.  Either (string-to-number "1:") should return
>> zero, as if the string was not a number at all
> That, and clarifying the docstring, would be the most sensible solution.

Except that it will break existing code.


        Stefan

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

On Fri, Apr 24, 2009 at 15:22, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Except that it will break existing code.

<sigh> I know, as I explicitly acknowledged that fact in my message...
Also, it was Eli who suggested it.

Still, that code relies in undocumented behavior. As the "1:" vs.
"1.2:" vs. "1.2" difference demonstrates, it's just luck that it
works.

One way or the other, the docstring should be fixed.

    Juanma

Re: Inconsistency in `string-to-number'

[Stefan Monnier]

>> Except that it will break existing code.
> <sigh> I know, as I explicitly acknowledged that fact in my message...
> Also, it was Eli who suggested it.

Don't take it personally, I was just pointing out the part which I found
to be most important.  I know I have written code which relies on this
behavior (tho I can't remember where ... hmm ... probably there's some
of it in diff-mode.el).

> Still, that code relies in undocumented behavior.  As the "1:" vs.
> "1.2:" vs. "1.2" difference demonstrates, it's just luck that
> it works.

No doubt this is a bug.  It's a long-standing one, tho, so it's not
a regression in Emacs-23.

Eli said:
> Which is why I thing we should fix it the other way around: to
> recognize "1.2:" as a number.  That cannot possibly break anything, so
> it should be safe to do before the release, IMO.

Actually it could also break code (tho it seems unlikely).  I wouldn't
mind postponing the fix to Emacs-23.2.  In the mean time, we need to
improve the docstring.


        Stefan

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

On Fri, Apr 24, 2009 at 16:06, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Don't take it personally, I was just pointing out the part which I found
> to be most important.

No, of course no. Excuse me if my answer sounded grumpy.

> No doubt this is a bug.  It's a long-standing one, tho, so it's not
> a regression in Emacs-23.

Agreed.

> I wouldn't
> mind postponing the fix to Emacs-23.2.  In the mean time, we need to
> improve the docstring.

I agree on both counts.

    Juanma

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

On Fri, Apr 24, 2009 at 16:06, Stefan Monnier<monnier@iro.umontreal.ca> wrote:

> Actually it could also break code (tho it seems unlikely).  I wouldn't
> mind postponing the fix to Emacs-23.2.  In the mean time, we need to
> improve the docstring.

The docstring was not improved, and we're now on the 23.2 track, so
time for a reprieve.

Reminder: The problem is the following inconsistency:

  (string-to-number "1:")  =>  1
  (string-to-number "1.2:")  =>  1
  (string-to-number "1.2") =>  1.2

Alas, the docstring talks about "leading spaces and tabs", but says
nothing about trailing chars.

Currently the behavior, depending of the first non-digit after the
number, is as follows:

 - \0, \s, \r, \n, \f and \t  => the number is read as a float (if
base == 10) or integer, as intended.
 - Any other char: the number is always interpreted as an integer.

Possibilities:

 0) Do nothing except clarifying the docs.
     Pro: easier of all fixes.
     Cons: inconsistency.

 1) Disallow any trailing char.
     Pro: follows the doc (sort of).
     Cons: incompatibility with current uses of undocumented "1:", etc.

 2) Allow only whitespace: the same chars that the float case admits right now.
     Pro: quite intuitive (IMO), easy to implement.
     Cons: Same as 1)

 3) Allow any trailing char.
     Pro: forgiving.
     Cons: (unlikely) incompatibility with uses of undocumented "1.2:" => 1

I like 2), because it seems cleaner to just allow whitespace all
around the number; it has a certain risk of incompatibility, though.
1) and 3) would require adding a new parameter to
lread.c:isfloat_string() or somesuch; not hard, but not very clean.

Thoughts?

    Juanma

Re: Inconsistency in `string-to-number'

[Stefan Monnier]

>> Actually it could also break code (tho it seems unlikely).  I wouldn't
>> mind postponing the fix to Emacs-23.2.  In the mean time, we need to
>> improve the docstring.
> The docstring was not improved, and we're now on the 23.2 track, so
> time for a reprieve.

Sorry, your email got misfiled.

> Reminder: The problem is the following inconsistency:

>   (string-to-number "1:")  =>  1
>   (string-to-number "1.2:")  =>  1
>   (string-to-number "1.2") =>  1.2

> Alas, the docstring talks about "leading spaces and tabs", but says
> nothing about trailing chars.

> Currently the behavior, depending of the first non-digit after the
> number, is as follows:

>  - \0, \s, \r, \n, \f and \t  => the number is read as a float (if
> base == 10) or integer, as intended.
>  - Any other char: the number is always interpreted as an integer.

> Possibilities:

>  0) Do nothing except clarifying the docs.
>      Pro: easier of all fixes.
>      Cons: inconsistency.

That would be documenting a bad behavior, so that would only be
acceptable if the new doc says "if there's something after the number,
you're on your own".

>  1) Disallow any trailing char.
>      Pro: follows the doc (sort of).
>      Cons: incompatibility with current uses of undocumented "1:", etc.

That's not a good option, no.  I know there is code out there in use
that relies on this behavior (I'm probably guilty myself), but I don't
know how to find it to fix it, so I'd rather stay away from this.

>  2) Allow only whitespace: the same chars that the float case admits right now.
>      Pro: quite intuitive (IMO), easy to implement.
>      Cons: Same as 1)

Indeed, same as above.

>  3) Allow any trailing char.
>      Pro: forgiving.
>      Cons: (unlikely) incompatibility with uses of undocumented "1.2:" => 1

That would be my choice.  The behavior is still fairly regular, so the
doc shouldn't be too scary, and it works about as well as now.

> I like 2), because it seems cleaner to just allow whitespace all
> around the number; it has a certain risk of incompatibility, though.
> 1) and 3) would require adding a new parameter to
> lread.c:isfloat_string() or somesuch; not hard, but not very clean.

I'd need to see the patch to pronouce myself on the uncleanliness.


        Stefan

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

On Tue, Dec 1, 2009 at 21:15, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Sorry, your email got misfiled.

No problem.

>>  3) Allow any trailing char.
>>      Pro: forgiving.
>>      Cons: (unlikely) incompatibility with uses of undocumented "1.2:" => 1
>
> That would be my choice.  The behavior is still fairly regular, so the
> doc shouldn't be too scary, and it works about as well as now.

OK.

> I'd need to see the patch to pronouce myself on the uncleanliness.

Attached.

    Juanma



diff --git a/src/lisp.h b/src/lisp.h
index 2052dfa..3bdecc5 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2795,7 +2795,7 @@ extern Lisp_Object Vcurrent_load_list;
 extern Lisp_Object Vload_history, Vload_suffixes, Vload_file_rep_suffixes;
 extern int openp P_ ((Lisp_Object, Lisp_Object, Lisp_Object,
 		      Lisp_Object *, Lisp_Object));
-extern int isfloat_string P_ ((char *));
+extern int isfloat_string P_ ((char *, int));
 extern void map_obarray P_ ((Lisp_Object, void (*) (Lisp_Object, Lisp_Object),
 			     Lisp_Object));
 extern void dir_warning P_ ((char *, Lisp_Object));
diff --git a/src/lread.c b/src/lread.c
index 97b9410..f9b51a8 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -3026,7 +3026,7 @@ read1 (readcharfun, pch, first_in_list)
 		    }
 		  }
 	      }
-	    if (isfloat_string (read_buffer))
+	    if (isfloat_string (read_buffer, 0))
 	      {
 		/* Compute NaN and infinities using 0.0 in a variable,
 		   to cope with compilers that think they are smarter
@@ -3244,8 +3244,9 @@ substitute_in_interval (interval, arg)
 #define EXP_INT 16

 int
-isfloat_string (cp)
+isfloat_string (cp, ignore_trailing)
      register char *cp;
+     int ignore_trailing;
 {
   register int state;

@@ -3299,7 +3300,8 @@ isfloat_string (cp)
       cp += 3;
     }

-  return (((*cp == 0) || (*cp == ' ') || (*cp == '\t') || (*cp ==
'\n') || (*cp == '\r') || (*cp == '\f'))
+  return ((ignore_trailing
+           || (*cp == 0) || (*cp == ' ') || (*cp == '\t') || (*cp ==
'\n') || (*cp == '\r') || (*cp == '\f'))
 	  && (state == (LEAD_INT|DOT_CHAR|TRAIL_INT)
 	      || state == (DOT_CHAR|TRAIL_INT)
 	      || state == (LEAD_INT|E_CHAR|EXP_INT)
diff --git a/src/data.c b/src/data.c
index ce2d842..0f47556 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2353,11 +2353,11 @@ digit_to_number (character, base)
 DEFUN ("string-to-number", Fstring_to_number, Sstring_to_number, 1, 2, 0,
        doc: /* Parse STRING as a decimal number and return the number.
 This parses both integers and floating point numbers.
-It ignores leading spaces and tabs.
+It ignores leading spaces and tabs, and all trailing chars.

 If BASE, interpret STRING as a number in that base.  If BASE isn't
 present, base 10 is used.  BASE must be between 2 and 16 (inclusive).
-If the base used is not 10, floating point is not recognized.  */)
+If the base used is not 10, STRING is always parsed as integer.  */)
      (string, base)
      register Lisp_Object string, base;
 {
@@ -2392,7 +2392,7 @@ If the base used is not 10, floating point is
not recognized.  */)
   else if (*p == '+')
     p++;

-  if (isfloat_string (p) && b == 10)
+  if (isfloat_string (p, 1) && b == 10)
     val = make_float (sign * atof (p));
   else
     {

Re: Inconsistency in `string-to-number'

[Juanma Barranquero]

On Fri, Dec 4, 2009 at 04:21, Juanma Barranquero <lekktu@gmail.com> wrote:

> Attached.

And mangled by Gmail. Luckily, it's trivial.

    Juanma

Re: Inconsistency in `string-to-number'

[Stefan Monnier]

>> I'd need to see the patch to pronouce myself on the uncleanliness.
> Attached.

Looks bearable.  Feel free to install it,


        Stefan