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

NaN > NaN returns true #50811

Closed
jimblandy opened this issue May 16, 2018 · 10 comments · Fixed by #50812
Closed

NaN > NaN returns true #50811

jimblandy opened this issue May 16, 2018 · 10 comments · Fixed by #50812
Assignees
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jimblandy
Copy link
Contributor

jimblandy commented May 16, 2018

The following tests do not pass for me, but I think they should, as comparisons with NaN should always simply evaluate to false:

#[test]
fn test_zero_zero() {
    assert_eq!(0.0/0.0 < 0.0/0.0, false);
    assert_eq!(0.0/0.0 > 0.0/0.0, false);
}

#[test]
fn test_nan_nan() {
    assert_eq!(std::f64::NAN < std::f64::NAN, false);
    assert_eq!(std::f64::NAN > std::f64::NAN, false);
}

fn main() {
    println!("Hello, world!");
}

I get this result on both debug and release builds. I'm on an x86_64 running Linux, rustc 1.27.0-nightly (f0fdaba 2018-05-15).

@jorendorff
Copy link
Contributor

jorendorff commented May 16, 2018

Playground link for this test.

https://play.rust-lang.org/?gist=4d6f7f54553b065615caac65b3cb6222&version=nightly&mode=debug

Affects both debug and release, Nightly and Stable.

@varkor
Copy link
Member

varkor commented May 16, 2018

Playground:

fn main() {
    let operator = std::f64::NAN > std::f64::NAN;
    let method = std::f64::NAN.gt(&std::f64::NAN);
    println!("{} {}", operator, method); // false false
    assert_eq!(operator, method); // doesn't fail
    assert_eq!(std::f64::NAN > std::f64::NAN, std::f64::NAN.gt(&std::f64::NAN)); // fails
}

I think this is an issue with assert_eq! rather than the floating-point comparisons, but I haven't dug into it yet.

@kennytm kennytm added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels May 16, 2018
@kennytm
Copy link
Member

kennytm commented May 16, 2018

This seems to be a regression introduced by 1.26.

@kennytm kennytm added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label May 16, 2018
@kennytm
Copy link
Member

kennytm commented May 16, 2018

Reduced, related to rvalue promotion I guess.

extern { fn abort() -> !; }

fn main() {
    let f = &(std::f64::NAN > std::f64::NAN);
    if *f {
        unsafe { abort(); }
    }
}

Edit: Or maybe a bug in Miri (cc @oli-obk)

extern { fn abort() -> !; }

static B: bool = std::f64::NAN > std::f64::NAN;

fn main() {
    if B {
        unsafe { abort(); }
    }
}

@kennytm
Copy link
Member

kennytm commented May 16, 2018

Bisection between c83fa5d...097efa9 gives #46882 as the regression PR, as expected.

@kennytm kennytm added the A-const-eval Area: Constant evaluation (MIR interpretation) label May 16, 2018
@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2018

I guess this logic, which I blindly copied, was never correct to begin with:

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/operator.rs#L188

I think this is how old const eval did it, and we didn't want to regress that. cc @eddyb

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2018

40b118c#diff-580fb7d0b0ede57fb2ce24a5dd33910cL50

Has the comment about existing bad behaviour.

@kennytm
Copy link
Member

kennytm commented May 16, 2018

@oli-obk The old logic used unwrap, meaning NaN > NaN would just ICE (?). I think we can safely replace them directly as PrimVal::from_bool(l > r) etc.

Edit: 🤔 The unwrap is in 40b118c#diff-e847a6da94440f7bf6783206efe4275bL138

@kennytm
Copy link
Member

kennytm commented May 16, 2018

The incorrect behavior mentioned in #50811 (comment) was this:

let x: [u8; (std::f64::NAN > std::f64::NAN) as usize] = [1];

If we fix the float comparison behavior, this code will no longer compile. But I think this is clearly a bug fix and breaking code like this is acceptable.

@nikomatsakis
Copy link
Contributor

Fixed in #50812

bors added a commit that referenced this issue May 21, 2018
…li-obk

Fix issue #50811 (`NaN > NaN` was true).

Fix #50811

Make sure the float comparison output is consistent with the expected behavior when NaN is involved.

----

Note: This PR is a **BREAKING CHANGE**. If you have used `>` or `>=` to compare floats, and make the result as the length of a fixed array type, like:

```rust
use std::f64::NAN;
let x: [u8; (NAN > NAN) as usize] = [1];
```

then the code will no longer compile. Previously, all float comparison involving NaN will just return "Greater", i.e. `NAN > NAN` would wrongly return `true` during const evaluation. If you need to retain the old behavior (why), you may replace `a > b` with `a != a || b != b || a > b`.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue May 24, 2018
Fix rust-lang#50811

Make sure the float comparison output is consistent with the expected behavior when NaN is involved.

----

Note: This PR is a **BREAKING CHANGE**. If you have used `>` or `>=` to compare floats, and make the result as the length of a fixed array type, like:

```rust
use std::f64::NAN;
let x: [u8; (NAN > NAN) as usize] = [1];
```

then the code will no longer compile. Previously, all float comparison involving NaN will just return "Greater", i.e. `NAN > NAN` would wrongly return `true` during const evaluation. If you need to retain the old behavior (why), you may replace `a > b` with `a != a || b != b || a > b`.
bors added a commit that referenced this issue Jun 1, 2018
[beta] Process backports

Merged and approved:

* #50812: Fix issue #50811 (`NaN > NaN` was true).
* #50827: Update LLVM to `56c931901cfb85cd6f7ed44c7d7520a8de1edf97`
* #50879: Fix naming conventions for new lints
* #51011: rustdoc: hide macro export statements from docs
* #51051: prohibit turbofish in `impl Trait` methods
* #51052: restore emplacement syntax (obsolete)
* #51146: typeck: Do not pass the field check on field error
* #51235: remove notion of Implicit derefs from mem-cat

r? @ghost
bors added a commit that referenced this issue Jun 3, 2018
[beta] Process backports

Merged and approved:

* #50812: Fix issue #50811 (`NaN > NaN` was true).
* #50879: Fix naming conventions for new lints
* #51011: rustdoc: hide macro export statements from docs
* #51051: prohibit turbofish in impl Trait methods
* #51052: restore emplacement syntax (obsolete)
* #51146: typeck: Do not pass the field check on field error
* #51235: remove notion of Implicit derefs from mem-cat

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants