Repository: rust-bakery/nom (language: rust)

Issue:
Updating to Nom 7 has broken `double("NaN")`
```
╰─$ rustc -V                                                                                                                                                                           
rustc 1.54.0 (a178d0322 2021-07-26)
```

Given the following code:

```rust
use nom::{error::Error, number::complete::double};
fn main() {
    println!("{:?}", double::<_, Error<_>>("NaN"));
}
```

In Nom version 6,  this runs and returns:

```
Ok(("", NaN))
```

In Nom version 7, this returns:

```
Err(Error(Error { input: "NaN", code: Float }))
```

Maybe this is intentional? I'm not seeing anything in the release notes about it, however.




Discussion / hints:
https://github.com/Geal/nom/issues/1309:

> Does not handle special values (NaN, Infinity).

Personally I do not understand why we take Minimal-Lexical there are soo many con, unfortunatly I see the change when it was already done, Rust 1.55 just landed and have a new float parser that is expected to be fast https://blog.rust-lang.org/2021/09/09/Rust-1.55.0.html#faster-more-correct-float-parsing we should maybe considering use it instead.
> #1309:
> 
> > Does not handle special values (NaN, Infinity).
> 
> Personally I do not understand why we take Minimal-Lexical there are soo many con, unfortunatly I see the change when it was already done, Rust 1.55 just landed and have a new float parser that is expected to be fast https://blog.rust-lang.org/2021/09/09/Rust-1.55.0.html#faster-more-correct-float-parsing we should maybe considering use it instead.

I'm the author of that PR, and the author of minimal-lexical. Geal/nom isn't supporting only v1.55, only v1.48+, but ideally, once the MSRV increases, yes, this dependency should be removed. Currently, the performance gap between 1.48-1.54 versions of float parsing is very notable, so minimal-lexical is meant to patch the gap. Also, `NaN` parsing isn't what's handled by minimal-lexical, but I can submit a PR here to fix that.
Ok I know where this is a bug: the implementation does not check for special values right now, which minimal-lexical doesn't handle internally. I'll submit a PR shortly.
I'm concern with perf penalty this do, cause Gael compare bench with different feature it was an unfair benchmark.
> I'm concern with perf penalty this do, cause Gael compare bench with different feature it was an unfair benchmark.

I haven't seen Geal's exact benchmarks, but I can tell you the performance is a fair bit faster prior to 1.55.0 for a relatively comprehensive suite of floats. After 1.55.0, standard should be a bit faster **if** we don't have to tokenize the float (but this isn't the case), so minimal-lexical should actually still be faster, since core does a lot of duplicate work. This is because there is no support for a partial parser in core.

And, prior to 1.55.0, once again, the algorithm for float parsing was **substantially** worse, which is why I spent a lot of time implementing that. If after these fixes, you want me to benchmark the difference, I'd be happy to do so.
My problem is that while I don't want to say your work is not amazing but I would like a more neutral argument, since you are the creator of `minimal-lexical` and owner of `lexical-core` your have praise your own crate, that not wrong but that make me vigilant to what you claim, in the original issue your said:

> Stable on no_std (coming to lexical-core in v0.8).
> Uses the Eisel-Lemire algorithm, providing faster float parsing (coming to lexical-core in v0.8).

But lexical-core is now 0.8, mean your list of pro become small add that I don't really agree with some

> Faster compile times.

I don't care rust is slow anyway and `cargo check` do the job. Also `lexical-core` claim to be faster now

> Stable API with a single exported trait and function.

I don't know much so no opinion I expect `lexical-core` have similar.

> Dependency-free when disabling no_alloc feature.

Why dependency would be a problem specially https://crates.io/crates/lexical-core/0.8.0/dependencies only dependency are internal tool to split crates.

> Easier support for custom float formats (may be useful to support parsing JSON-conforming floats, which could be a valid use-case).

`double` purpose of nom is to parse standard float (I think same of std) so not a pro for nom

> Larger version support (minimal-lexical supports 1.31.0+, while lexical-core requires 1.37.0+).

lexical-core 0.8 require `1.51.0`, can't say it's not a pro but personally that a meh, the nom 7 would just have require a little more.

And we end with two cons:

> Must tokenize the float in Nom.
> Does not handle special values (NaN, Infinity).

I add one

> much less used and less maintained power

The main pro was benchmark, but what if are the number versus `lexical-core` 0.8 (obviously with `NaN` and `Infinity`)? If the number with the special value handle show lexical-core is better I will see no reason to not pick `lexical-core` again. If number show `minimal-lexical` win well nice. (that of course taking in account that nom will be the slowing part of the bench for `minimal-lexical`)
I don't mind either way to use lexical-core vs minimal-lexical (hi 👋👋 I maintain both, and I'm happy to ensure that people **don't** have to use my crates, which is why I implemented the PR into Rust core, and have made numerous PRs to `fast_float` (the C++ implementation) and `fast-float-rust` because I want the ecosystem to win.

> My problem is that while I don't want to say your work is not amazing but I would like a more neutral argument, since you are the creator of `minimal-lexical` your have praise your own crate, that not wrong but that make me vigilant to what you claim, in the original issue your said:
>
> But lexical-core is now 0.8, mean your list of pro become small add that I don't really agree with some

minimal-lexical is different than lexical-core. lexical-core v0.8 is certainly faster to compile than v0.7, but it can't compete with a dedicated, small, isolated crate.

> I don't care rust is slow anyway and `cargo check` do the job. Also `lexical-core` claim to be faster now

Many people do care about compile times.

> I don't know much so no opinion I expect `lexical-core` have similar.

lexical-core does have a relatively simple API, but once again, it's not possible to have only one function in lexical-core. The high-level API is ~4 parsing functions, although it's relatively moot: the main point is compile times.

> > Dependency-free when disabling no_alloc feature.
> 
> Why dependency would be a problem specially https://crates.io/crates/lexical-core/0.8.0/dependencies only dependency are internal tool to split crates.

Because dependencies add compile time issues? I added `libm` as a dependency to lexical-core v0.7 for stable on `no_std` and it alone added 2s to every build time. A lot of work went in to removing libm as a dependency entirely.

> > Easier support for custom float formats (may be useful to support parsing JSON-conforming floats, which could be a valid use-case).
> 
> `double` purpose of nom is to parse standard float (I think same of std) so not a pro for nom

I mean, sure, but it's also a useful potential feature later.

> > Larger version support (minimal-lexical supports 1.31.0+, while lexical-core requires 1.37.0+).
> 
> lexical-core 0.8 require `1.51.0`, can't say it's not a pro but personally that a meh, the nom 7 would just have require a little more.

nom7 has already maintained to supporting v1.48+, and lexical-core 0.8 **cannot** be re-written to support versions older than 1.51, due to the use of const generics integral to the new implementation.

> And we end with two cons:
> 
> > Must tokenize the float in Nom.
> > Does not handle special values (NaN, Infinity).
> 
> I add one
> 
> > much less used and less maintained power

minimal-lexical is actually used in a private fork by serde-json, so it's unquestionably more used. The maintainers are the same for both lexical-core and minimal-lexical, and maintenance plans have been put into motion if I cannot maintain.

> The main pro was benchmark, but what if are the number versus `lexical-core` 0.8 (obviously with `NaN` and `Infinity`)? If the number with the special value handle show lexical-core is better I will see no reason to not pick `lexical-core` again. If number show `minimal-lexical` win well nice.

Sure, but there is a compile time difference. minimal-lexical compiles on my computer in ~0.5s, while lexical-core v0.8 with only the float-parsing is ~1.5s. This is substantially faster than the nearly 3.5s of v0.7, but it's worth considering there's a tradeoff for both cases.
Anyway, I'm fine with any of the options:

1. Patching this bug (already doing that) and keeping minimal-lexical.
2. Moving to fast-float-rust, which is like lexical-core but much leaner (although it currently has a significant slower slow-path algorithm, since it uses a decimal class rather than big-integer arithmetic). I've opened an issue so this can be fixed, and am currently implementing a PR for both core and fast-float-rust to ensure these algorithm improvements benefit **everyone**.
3. Moving back to lexical-core, but v0.8 cannot be used which negates many of the upsides of the migration. v0.7 has some notable dependencies, such as arrayvec and libm, which do substantially increase build times. There's also slower algorithms used, which is partially why it was such a major re-write.
> Sure, but there is a compile time difference. minimal-lexical compiles on my computer in ~0.5s, while lexical-core v0.8 with only the float-parsing is ~1.5s. This is substantially faster than the nearly 3.5s of v0.7, but it's worth considering there's a tradeoff for both cases.

I'm not talking about compile time here, but benchmark in actually doing the job. That in my opinion is what matter the most in crate like nom.

> nom7 has already maintained to supporting v1.48+, and lexical-core 0.8 cannot be re-written to support versions older than 1.51, due to the use of const generics integral to the new implementation.

you didn't understand what I wanted to said, I mean that nom 7 would have require a most recent rust version if needed. that was not a big deal.

> 1. Patching this bug (already doing that) and keeping minimal-lexical.
> 
> 2. Moving to fast-float-rust, which is like lexical-core but much leaner (although it currently has a significant slower slow-path algorithm, since it uses a decimal class rather than big-integer arithmetic). I've opened an issue so this can be fixed, and am currently implementing a PR for both core and fast-float-rust to ensure these algorithm improvements benefit **everyone**.
> 
> 3. Moving back to lexical-core, but v0.8 cannot be used which negates many of the upsides of the migration. v0.7 has some notable dependencies, such as arrayvec and libm, which do substantially increase build times. There's also slower algorithms used, which is partially why it was such a major re-write.

1. Depend is that what gael want for number ? since there is no test for that maybe it's not the case, I think nom should handle the same of what std guarantee to handle. So the [`EBNF`](https://doc.rust-lang.org/std/primitive.f64.html#grammar) grammar. If people want something specific their will need to do their own parser and using your crate directly.
2. without good reason I don't see why we should do that
3. for nom 8 maybe. for nom 7 is too late, but again we need benchmark to choice and bench that take into account all features. Now we use minimal-lexical I will want a good reason to change to lexical-core :p
> I'm not talking about compile time here, but benchmark in actually doing the job. That in my opinion is what matter the most in crate like nom.

I understood, I pointed out numerous times I don't agree. Although compile time isn't the only factor, minimal-lexical is also fairly performant.

> but again we need benchmark to choice and bench that take into account all features. Now we use minimal-lexical I will want a good reason to change to lexical-core :p

I appreciate that. Anyway, I'll do my best to ensure float parsing is viable for everything. In the future, if the new slow-path algorithms are implemented in fast-float-rust, that might be the most logical option.
so, to sum up here:
- the new float parser in libstd is great, but as @Alexhuszagh pointed out, it is not compatible with nom's MSRV, so we'll use minimal-lexical for now
- we could go back to lexical-core, but that could only happen on the next MSRV raise, ie nom 8
- maybe it's faster with minimal-lexical, or lexical-core, or fast-float-rust. We won't know until an actual benchmark is done

I think we can wait a bit before considering a change of float parsing library.

Personally, I am not fond of arguing about the right way to parse floats in nom: the parsers were added for convenience (and for benchmarks, unfortunately), but there's no guarantee that they would parse exactly like people expect in their formats. So they have to provide a minimal feature set for prototyping, then users can make their own tokenizer and pass to something like minimal-lexical or libstd's new float parser, or rely directly on lexical-core and its various format options.
For the record, I have worked around this issue in our app so there is no urgency on my part to resolve this. Nor do I have a particularly strong opinion on the correct way it should be done.
https://github.com/Geal/nom/issues/1421#issuecomment-949917118 works around this issue as well. 

Fix patch:
diff --git a/src/number/complete.rs b/src/number/complete.rs
index 2592ff4cb..d23079e76 100644
--- a/src/number/complete.rs
+++ b/src/number/complete.rs
@@ -1426,6 +1426,40 @@ where
   )(input)
 }
 
+// workaround until issues with minimal-lexical are fixed
+#[doc(hidden)]
+pub fn recognize_float_or_exceptions<T, E: ParseError<T>>(input: T) -> IResult<T, T, E>
+where
+  T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>>,
+  T: Clone + Offset,
+  T: InputIter + InputTake + Compare<&'static str>,
+  <T as InputIter>::Item: AsChar,
+  T: InputTakeAtPosition,
+  <T as InputTakeAtPosition>::Item: AsChar,
+{
+  alt((
+    |i: T| {
+      recognize_float::<_, E>(i.clone()).map_err(|e| match e {
+        crate::Err::Error(_) => crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)),
+        crate::Err::Failure(_) => crate::Err::Failure(E::from_error_kind(i, ErrorKind::Float)),
+        crate::Err::Incomplete(needed) => crate::Err::Incomplete(needed),
+      })
+    },
+    |i: T| {
+      crate::bytes::complete::tag_no_case::<_, _, E>("nan")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+    |i: T| {
+      crate::bytes::complete::tag_no_case::<_, _, E>("inf")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+    |i: T| {
+      crate::bytes::complete::tag_no_case::<_, _, E>("infinity")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+  ))(input)
+}
+
 /// Recognizes a floating point number in text format and returns the integer, fraction and exponent parts of the input data
 ///
 /// *Complete version*: Can parse until the end of input.
@@ -1517,6 +1551,8 @@ where
   Ok((i, (sign, integer, fraction, exp)))
 }
 
+use crate::traits::ParseTo;
+
 /// Recognizes floating point number in text format and returns a f32.
 ///
 /// *Complete version*: Can parse until the end of input.
@@ -1537,7 +1573,7 @@ where
 pub fn float<T, E: ParseError<T>>(input: T) -> IResult<T, f32, E>
 where
   T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>> + Slice<Range<usize>>,
-  T: Clone + Offset,
+  T: Clone + Offset + ParseTo<f32> + Compare<&'static str>,
   T: InputIter + InputLength + InputTake,
   <T as InputIter>::Item: AsChar + Copy,
   <T as InputIter>::IterElem: Clone,
@@ -1546,6 +1582,7 @@ where
   T: AsBytes,
   T: for<'a> Compare<&'a [u8]>,
 {
+  /*
   let (i, (sign, integer, fraction, exponent)) = recognize_float_parts(input)?;
 
   let mut float: f32 = minimal_lexical::parse_float(
@@ -1558,6 +1595,15 @@ where
   }
 
   Ok((i, float))
+      */
+  let (i, s) = recognize_float_or_exceptions(input)?;
+  match s.parse_to() {
+    Some(f) => (Ok((i, f))),
+    None => Err(crate::Err::Error(E::from_error_kind(
+      i,
+      crate::error::ErrorKind::Float,
+    ))),
+  }
 }
 
 /// Recognizes floating point number in text format and returns a f64.
@@ -1580,7 +1626,7 @@ where
 pub fn double<T, E: ParseError<T>>(input: T) -> IResult<T, f64, E>
 where
   T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>> + Slice<Range<usize>>,
-  T: Clone + Offset,
+  T: Clone + Offset + ParseTo<f64> + Compare<&'static str>,
   T: InputIter + InputLength + InputTake,
   <T as InputIter>::Item: AsChar + Copy,
   <T as InputIter>::IterElem: Clone,
@@ -1589,6 +1635,7 @@ where
   T: AsBytes,
   T: for<'a> Compare<&'a [u8]>,
 {
+  /*
   let (i, (sign, integer, fraction, exponent)) = recognize_float_parts(input)?;
 
   let mut float: f64 = minimal_lexical::parse_float(
@@ -1601,6 +1648,15 @@ where
   }
 
   Ok((i, float))
+      */
+  let (i, s) = recognize_float_or_exceptions(input)?;
+  match s.parse_to() {
+    Some(f) => (Ok((i, f))),
+    None => Err(crate::Err::Error(E::from_error_kind(
+      i,
+      crate::error::ErrorKind::Float,
+    ))),
+  }
 }
 
 #[cfg(test)]
@@ -1941,6 +1997,7 @@ mod tests {
       "12.34",
       "-1.234E-12",
       "-1.234e-12",
+      "0.00000000000000000087",
     ];
 
     for test in test_cases.drain(..) {
@@ -1964,6 +2021,14 @@ mod tests {
       recognize_float(remaining_exponent),
       Err(Err::Failure(("", ErrorKind::Digit)))
     );
+
+    let (_i, nan) = float::<_, ()>("NaN").unwrap();
+    assert!(nan.is_nan());
+
+    let (_i, inf) = float::<_, ()>("inf").unwrap();
+    assert!(inf.is_infinite());
+    let (_i, inf) = float::<_, ()>("infinite").unwrap();
+    assert!(inf.is_infinite());
   }
 
   #[test]
@@ -2051,7 +2116,6 @@ mod tests {
 
   #[cfg(feature = "std")]
   fn parse_f64(i: &str) -> IResult<&str, f64, ()> {
-    use crate::traits::ParseTo;
     match recognize_float(i) {
       Err(e) => Err(e),
       Ok((i, s)) => {
diff --git a/src/number/streaming.rs b/src/number/streaming.rs
index d2d0f3ca9..7c53ccc47 100644
--- a/src/number/streaming.rs
+++ b/src/number/streaming.rs
@@ -1395,6 +1395,40 @@ where
   )(input)
 }
 
+// workaround until issues with minimal-lexical are fixed
+#[doc(hidden)]
+pub fn recognize_float_or_exceptions<T, E: ParseError<T>>(input: T) -> IResult<T, T, E>
+where
+  T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>>,
+  T: Clone + Offset,
+  T: InputIter + InputTake + InputLength + Compare<&'static str>,
+  <T as InputIter>::Item: AsChar,
+  T: InputTakeAtPosition,
+  <T as InputTakeAtPosition>::Item: AsChar,
+{
+  alt((
+    |i: T| {
+      recognize_float::<_, E>(i.clone()).map_err(|e| match e {
+        crate::Err::Error(_) => crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)),
+        crate::Err::Failure(_) => crate::Err::Failure(E::from_error_kind(i, ErrorKind::Float)),
+        crate::Err::Incomplete(needed) => crate::Err::Incomplete(needed),
+      })
+    },
+    |i: T| {
+      crate::bytes::streaming::tag_no_case::<_, _, E>("nan")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+    |i: T| {
+      crate::bytes::streaming::tag_no_case::<_, _, E>("inf")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+    |i: T| {
+      crate::bytes::streaming::tag_no_case::<_, _, E>("infinity")(i.clone())
+        .map_err(|_| crate::Err::Error(E::from_error_kind(i, ErrorKind::Float)))
+    },
+  ))(input)
+}
+
 /// Recognizes a floating point number in text format and returns the integer, fraction and exponent parts of the input data
 ///
 /// *Streaming version*: Will return `Err(nom::Err::Incomplete(_))` if there is not enough data.
@@ -1512,7 +1546,7 @@ pub fn float<T, E: ParseError<T>>(input: T) -> IResult<T, f32, E>
 where
   T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>>,
   T: Clone + Offset,
-  T: InputIter + InputLength + InputTake + crate::traits::ParseTo<i32>,
+  T: InputIter + InputLength + InputTake + crate::traits::ParseTo<f32> + Compare<&'static str>,
   <T as InputIter>::Item: AsChar,
   <T as InputIter>::IterElem: Clone,
   T: InputTakeAtPosition,
@@ -1520,6 +1554,7 @@ where
   T: AsBytes,
   T: for<'a> Compare<&'a [u8]>,
 {
+  /*
   let (i, (sign, integer, fraction, exponent)) = recognize_float_parts(input)?;
 
   let mut float: f32 = minimal_lexical::parse_float(
@@ -1532,6 +1567,15 @@ where
   }
 
   Ok((i, float))
+  */
+  let (i, s) = recognize_float_or_exceptions(input)?;
+  match s.parse_to() {
+    Some(f) => (Ok((i, f))),
+    None => Err(crate::Err::Error(E::from_error_kind(
+      i,
+      crate::error::ErrorKind::Float,
+    ))),
+  }
 }
 
 /// Recognizes floating point number in text format and returns a f64.
@@ -1556,7 +1600,7 @@ pub fn double<T, E: ParseError<T>>(input: T) -> IResult<T, f64, E>
 where
   T: Slice<RangeFrom<usize>> + Slice<RangeTo<usize>>,
   T: Clone + Offset,
-  T: InputIter + InputLength + InputTake + crate::traits::ParseTo<i32>,
+  T: InputIter + InputLength + InputTake + crate::traits::ParseTo<f64> + Compare<&'static str>,
   <T as InputIter>::Item: AsChar,
   <T as InputIter>::IterElem: Clone,
   T: InputTakeAtPosition,
@@ -1564,6 +1608,7 @@ where
   T: AsBytes,
   T: for<'a> Compare<&'a [u8]>,
 {
+  /*
   let (i, (sign, integer, fraction, exponent)) = recognize_float_parts(input)?;
 
   let mut float: f64 = minimal_lexical::parse_float(
@@ -1576,6 +1621,15 @@ where
   }
 
   Ok((i, float))
+  */
+  let (i, s) = recognize_float_or_exceptions(input)?;
+  match s.parse_to() {
+    Some(f) => (Ok((i, f))),
+    None => Err(crate::Err::Error(E::from_error_kind(
+      i,
+      crate::error::ErrorKind::Float,
+    ))),
+  }
 }
 
 #[cfg(test)]
@@ -2022,6 +2076,7 @@ mod tests {
       "12.34",
       "-1.234E-12",
       "-1.234e-12",
+      "0.00000000000000000087",
     ];
 
     for test in test_cases.drain(..) {
@@ -2045,6 +2100,14 @@ mod tests {
       recognize_float(remaining_exponent),
       Err(Err::Incomplete(Needed::new(1)))
     );
+
+    let (_i, nan) = float::<_, ()>("NaN").unwrap();
+    assert!(nan.is_nan());
+
+    let (_i, inf) = float::<_, ()>("inf").unwrap();
+    assert!(inf.is_infinite());
+    let (_i, inf) = float::<_, ()>("infinite").unwrap();
+    assert!(inf.is_infinite());
   }
 
   #[test]


Test patch: