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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 21, 2018
Merged

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

merged 1 commit into from
May 21, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented May 16, 2018 •
edited
Loading

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:

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.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2018
@kennytm
Copy link
Member Author

kennytm commented May 16, 2018

r? @oli-obk

Do you think we need to crater according to #50811 (comment)?

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank May 16, 2018
@kennytm kennytm force-pushed the fix-50756-miri-bad-float-behavior branch from 038a100 to cf3c554 Compare May 16, 2018 20:40
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm kennytm force-pushed the fix-50756-miri-bad-float-behavior branch from cf3c554 to 3a2bd0b Compare May 16, 2018 21:02
@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2018

Well... the clean way would be a future compat lint. And we definitely need some team to sign off on it.

That said: anyone depending on floating point corner cases for const eval things is somewhat asking for getting their code broken.

We could restore the original behaviour by making const eval decide on a way to do these edge cases depending on whether it is called in a true const environment or a promoted env, that would be horrible, but doable. Though I personally would like to see this PR merged as it is.

@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label May 16, 2018
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 17, 2018
@pietroalbini pietroalbini mentioned this pull request May 17, 2018
Closed
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/lang @rust-lang/compiler -- This PR is a candidate for stable backporting (1.26.1) as it fixes a bug in const eval; if you object, please note that here.

@kennytm kennytm added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2018
@kennytm kennytm force-pushed the fix-50756-miri-bad-float-behavior branch from 3a2bd0b to 50bc72d Compare May 17, 2018 09:18
@nikomatsakis nikomatsakis mentioned this pull request May 17, 2018
Closed
@nikomatsakis
Copy link
Contributor

Discussed in the @rust-lang/compiler meeting. General consensus was that NaN > Nan being true is definitely incorrect behavior and should be fixed. The only reason to hold up would be to warn users that may be affected, but we can't leave the semantics broken.

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 21, 2018
@pietroalbini
Copy link
Member

Ping from triage @oli-obk! How should this PR move forward?

@oli-obk
Copy link
Contributor

oli-obk commented May 21, 2018

@bors r+

@bors
Copy link
Contributor

bors commented May 21, 2018

📌 Commit 50bc72d has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 21, 2018
@Mark-Simulacrum
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented May 21, 2018

⌛ Testing commit 50bc72d with merge cb20f68...

bors added a commit that referenced this pull request May 21, 2018
@bors
Auto merge of #50812 - kennytm:fix-50756-miri-bad-float-behavior, r=o…
cb20f68 
…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`.
@bors
Copy link
Contributor

bors commented May 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing cb20f68 to master...

@bors bors merged commit 50bc72d into rust-lang:master May 21, 2018
@kennytm kennytm deleted the fix-50756-miri-bad-float-behavior branch May 22, 2018 05:46
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 24, 2018
@pietroalbini pietroalbini mentioned this pull request May 24, 2018
Merged
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 24, 2018
Merged
bors added a commit that referenced this pull request May 25, 2018
@bors
Auto merge of #51045 - Mark-Simulacrum:stable-point, r=alexcrichton
827013a 
Stable point release (1.26.1)

This includes all items on [the wishlist](#50756), plus #50694, which backported cleanly.

The target date is May 29th, Tuesday next week.

cc @rust-lang/compiler @oli-obk @eddyb  -- I backported #50812, but it wasn't a clean patch so review would be appreciated.
bors added a commit that referenced this pull request Jun 1, 2018
@bors
Auto merge of #51037 - pietroalbini:beta-backports, r=oli-obk
13abb91 
[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
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 2, 2018
bors added a commit that referenced this pull request Jun 3, 2018
@bors
Auto merge of #51037 - pietroalbini:beta-backports, r=pietroalbini
eb01d0f 
[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
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NaN > NaN returns true
8 participants
@kennytm @rust-highfive @oli-obk @Mark-Simulacrum @nikomatsakis @pietroalbini @bors @estebank