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

Fix handling of NaN sign throughout parsing and serialization #637

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Oct 26, 2023

I learned recently in rust-lang/miri#3139 that as produces NaN with a nondeterministic sign, and separately that the sign of f64::NAN is not specified (may be a negative NaN). As of rust-lang/rust#116551 Miri has begun intentionally exercising these cases.

This PR fixes places where -nan would incorrectly be deserialized as a positive NaN, nan or +nan would be deserialized as a negative NaN, negative NaN would be serialized as nan, or positive NaN would be serialized as -nan. It adds tests to confirm the expected sign of NaN values, and improves existing tests related to NaN.

@dtolnay
Copy link
Contributor Author

dtolnay commented Oct 26, 2023

Pre-commit failure appears unrelated: #638.

@epage
Copy link
Member

epage commented Oct 26, 2023

Wow, that is terrible. Thanks for not just letting me know, but fixing ! I've at least opened rust-lang/rust-clippy#11717 and rust-lang/rust-clippy#11718 in the hopes that we can enforce catching of these problems in the future.

@epage
Copy link
Member

epage commented Oct 26, 2023

To double check my understanding, we should also have a copysign on this use of nan, right?

https://github.com/toml-rs/toml/blob/main/crates/toml_edit/src/parser/numbers.rs#L304

EDIT: I'm fine merging as-is and fixing it myself if you want as some improvement is better than no improvement; I just want to make sure I understand.

@dtolnay
Copy link
Contributor Author

dtolnay commented Oct 26, 2023

Indeed; good call.

@epage
Copy link
Member

epage commented Oct 26, 2023

Thanks! This last change made me realize I need to go request another pedantic clippy lint: nan constants without a sign.

@epage epage merged commit 7f4f543 into toml-rs:main Oct 26, 2023
13 checks passed
@dtolnay dtolnay deleted the nansign branch October 26, 2023 16:30
@RalfJung
Copy link

Note that there are other bits in the NaN (observable via to_bits()) that are still potentially lost in the conversion.

I am somewhat surprised that the sign of the NaN is considered so significant that there were a bunch of tests checking it, and that the code is being adjusted to preserve it. What is the motivation for that? rust-lang/rfcs#3514 should possibly be amended to at least document this as a downside of the non-deterministic sign semantics.

@epage
Copy link
Member

epage commented Oct 26, 2023

I am somewhat surprised that the sign of the NaN is considered so significant that there were a bunch of tests checking it, and that the code is being adjusted to preserve it. What is the motivation for that

The sign of nan is specified in the TOML spec, so to be able to properly preserve it, we need to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants