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

toml can't parse large u64 numbers #512

Closed
punkeel opened this issue Feb 7, 2023 · 4 comments
Closed

toml can't parse large u64 numbers #512

punkeel opened this issue Feb 7, 2023 · 4 comments
Labels
A-parse Area: Parsing TOML C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change.

Comments

@punkeel
Copy link

punkeel commented Feb 7, 2023

There seems to be a bug with parsing large u64 values, reproducible in Playground.

Code:

use serde::{Deserialize, Serialize};

#[derive(Debug, Deserialize, Serialize)]
struct U64Test {
    pub value: u64,
}

fn main() {
    let text = toml::to_string(&U64Test { value: 12760345446341154300 }).unwrap();
    println!("{text}");
    let new: U64Test = toml::from_str(&text).unwrap();
    println!("value = {0}", new.value);
}

Error:

Compiling playground v0.0.1 (/playground)
    Finished release [optimized] target(s) in 3.88s
     Running `target/release/playground`

value = 12760345446341154300

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: ErrorInner { kind: NumberInvalid, line: Some(0), col: 8, at: Some(8), message: "", key: [] } }', src/main.rs:11:46
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:64:14
   2: core::result::unwrap_failed
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/result.rs:1791:5
   3: playground::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I was also able to reproduce this bug locally on macOS.

Note: I suspect this is a different bug from #511. Feel free to close if you feel like it's a duplicate.

@epage epage added C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-parse Area: Parsing TOML labels Feb 7, 2023
@epage
Copy link
Member

epage commented Feb 7, 2023

This is expected behavior. Integers are parsed as i64, disallowing u64::MAX from being parsed.

We could possibly allow the parser to parse i128 instead but

  • That should only be opt-in
  • The i64 type is exposed in the parser's (toml_edit) API in a way that enabling this with a feature flag would be a breaking change
  • Making toml_edit::Value / toml_edit::Item / toml_edit::Document generic over this would negatively impact the usability of the API.

Without a path forward for this, I'm inclined to mark this as not-planned. Feel free to continue discussion and maybe we find a way to re-open and resolve this one day.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2023
@punkeel
Copy link
Author

punkeel commented Feb 7, 2023

Thanks for the extra details!

I expect an encoding library to be able to properly encode/decode the values I give it, and in particular, be able to decode the values it can encode. Breaking this contract leads to data loss.

In order of preference, I'd rather:

  • If it builds, toml supports it (aka: support u64).
  • If it builds, and toml doesn't fully support a type, throw an error when Encoding and Decoding. If u64::MAX can't be stored, raise the error promptly.
  • Store u64 as string? Please no :)
  • Having to opt into u64 support, with a warning that not all values can be encoded.

The current behavior feels wrong today, so in my opinion this is worth a breaking change

@epage
Copy link
Member

epage commented Feb 7, 2023

serde does not let us do compile-time errors for unsupported types.

And we are erroring when parsing the unsupported values as demonstrated in this Issue and I just fixed your other Issue so we now error when writing unsupported values.

I had considered dropping support for u64 as value-dependent errors are more likely to cause people problems but decided not to for now as (1) I didn't think it was worth a breaking change and (2) u64s can generally be useful still. We can always revisit that later.

@punkeel
Copy link
Author

punkeel commented Feb 7, 2023

Thanks a bunch @epage, both for this issue & the other one!

I can confirm the error is thrown as it should, and this alleviates my concerns! 🙏


thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: OutOfRange(Some("u64")) }', crates/my_project/src/main.rs:5:42
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
make: *** [crates/my_project/config.toml] Error 101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parse Area: Parsing TOML C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

2 participants