Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Formats with digit_separators can't parse float numbers #66

Closed
Robert42 opened this issue Sep 3, 2021 · 3 comments
Closed

[BUG] Formats with digit_separators can't parse float numbers #66

Robert42 opened this issue Sep 3, 2021 · 3 comments
Assignees
Labels
bug Something isn't working high priority High priority

Comments

@Robert42
Copy link

Robert42 commented Sep 3, 2021

Description

Trying to define a custom format (a format containing digit separators), I couldn't get my number format to parse the string "42.0". After a while I've noticed, that those provided formats, which also contain digit separators, can't parse the same string either. See test case.

Prerequisites

  • Rust version: rustc 1.54.0 (a178d0322 2021-07-26)
  • lexical version: 6.0.0
  • lexical compilation features used: format

Test case

fn main() {
  const RUST: u128 = lexical::format::RUST_LITERAL;
  const JSON: u128 = lexical::format::JSON;
  const CXX: u128 = lexical::format::CXX17_LITERAL;

  let o = lexical::ParseFloatOptions::new();
  
  println!("{:?}", lexical::parse_with_options::<f64, _, JSON>("42.0", &o));
  
  // RUST_LITERAL
  println!("{:?}", lexical::parse_with_options::<f64, _, RUST>("42.0", &o));
  println!("{:?}", lexical::parse_with_options::<f64, _, RUST>("4_2.0", &o));
  
  // CXX17_LITERAL
  println!("{:?}", lexical::parse_with_options::<f64, _, CXX>("42.0", &o));
  println!("{:?}", lexical::parse_with_options::<f64, _, CXX>("4'2.0", &o));
}

I would expect all five println invocations to print Ok(42.0). But in the actual output, only the first one is able to parse the number.

Ok(42.0)
Err(EmptyInteger(2))
Err(EmptyInteger(3))
Err(EmptyMantissa(4))
Err(EmptyMantissa(5))

Additional Context

When I copy the RUST_LITERAL and CXX17_LITERAL definitions to my main function and comment out the digit_separator, the simple case can be parsed correctly:

  pub const CXX_NOSEP: u128 = lexical::NumberFormatBuilder::new()
//    .digit_separator(std::num::NonZeroU8::new(b'\''))
    .case_sensitive_special(true)
    .internal_digit_separator(true)
    .build();
  println!("{:?}", lexical::parse_with_options::<f64, _, CXX_NOSEP>("42.0", &o));

  pub const RUST_NO_SEP: u128 = lexical::NumberFormatBuilder::new()
//    .digit_separator(std::num::NonZeroU8::new(b'_'))
    .required_digits(true)
    .no_positive_mantissa_sign(true)
    .no_special(true)
    .internal_digit_separator(true)
    .trailing_digit_separator(true)
    .consecutive_digit_separator(true)
    .build();
  println!("{:?}", lexical::parse_with_options::<f64, _, RUST_NO_SEP>("42.0", &o));

prints

Ok(42.0)
Ok(42.0)
@Robert42 Robert42 added the bug Something isn't working label Sep 3, 2021
@Alexhuszagh Alexhuszagh added the high priority High priority label Sep 3, 2021
Alexhuszagh added a commit that referenced this issue Sep 3, 2021
@Alexhuszagh
Copy link
Owner

Closed as of lexical-util v0.8.1. Remember to run cargo update on any crates using lexical and it should work.

A working sample is as follows:

Cargo.toml

[package]
name = "lexical_test"
version = "0.1.0"
edition = "2018"

[dependencies.lexical]
version = "6.0"
features=["format"]

main.rs

fn main() {
  const RUST: u128 = lexical::format::RUST_LITERAL;
  const JSON: u128 = lexical::format::JSON;
  const CXX: u128 = lexical::format::CXX17_LITERAL;

  let o = lexical::ParseFloatOptions::new();

  println!("{:?}", lexical::parse_with_options::<f64, _, JSON>("42.0", &o));

  // RUST_LITERAL
  println!("{:?}", lexical::parse_with_options::<f64, _, RUST>("42.0", &o));
  println!("{:?}", lexical::parse_with_options::<f64, _, RUST>("4_2.0", &o));

  // CXX17_LITERAL
  println!("{:?}", lexical::parse_with_options::<f64, _, CXX>("42.0", &o));
  println!("{:?}", lexical::parse_with_options::<f64, _, CXX>("4'2.0", &o));
}

Tests for regressions have been added here.

@Robert42
Copy link
Author

Robert42 commented Sep 3, 2021

Wow, that was fast! Thank you.

The issue is now also gone in my actual project!

@Alexhuszagh
Copy link
Owner

Wow, that was fast! Thank you.

The issue is now also gone in my actual project!

Perfect, glad to hear it. Luckily, because of the re-structuring of the digit separator logic, this fix only required patching 3 lines of code, so once I saw this issue, it was quite an easy patch.

Let me know if there are any further issues, I've tried to extensively test functionality but sometimes bugs fall through the cracks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority High priority
Projects
None yet
Development

No branches or pull requests

2 participants