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

Weird f32 PartialEq behavior on 1.64 -> 1.65 #115071

Closed
BKDaugherty opened this issue Aug 21, 2023 · 3 comments
Closed

Weird f32 PartialEq behavior on 1.64 -> 1.65 #115071

BKDaugherty opened this issue Aug 21, 2023 · 3 comments
Labels
C-bug Category: This is a bug.

Comments

@BKDaugherty
Copy link

BKDaugherty commented Aug 21, 2023

I am trying to upgrade my company's toolchain from 1.64 to 1.71. I noticed that we had some odd code that was compiling before and is no longer compiling. I got it down to a change between 1.64 and 1.65, and my first thought is that this might be affected by this PR: #98655

Notably that PR mentions that...

Observable behaviour may change if a user has defined a type A with an
inconsistent PartialEq and then defines a type B that contains an
A and also derives PartialEq. Such code is already buggy and
preserving bug-for-bug compatibility isn't necessary.

But I would imagine neither bool nor f32 would have an inconsistent PartialEq? I apologize if this is something that already is covered by that statement and I'm misunderstanding.

Such code is already buggy and
preserving bug-for-bug compatibility isn't necessary.

It does seem like my code is already buggy, but wanted to file this anyways in case someone else comes across it.

So this compiles on 1.64, and doesn't on 1.65.

#[derive(PartialEq, Eq)]
struct WithBool {
    pub a: Option<bool>,
    pub b: Option<f32>,
}

Notably, none of the following compiles on 1.64.

#[derive(Debug, PartialEq, Eq)]
struct WithoutBool {
    pub b: Option<f32>,
}
#[derive(Debug, PartialEq, Eq)]
struct WithBoolNoOption {
    pub a: bool,
    pub b: Option<f32>,
}
#[derive(Debug, PartialEq, Eq)]
struct WithBoolNoOptionF32 {
    pub a: Option<bool>,
    pub b: f32,
}

Code

I tried this code:

#[derive(PartialEq, Eq)]
struct WithBool {
    pub a: Option<bool>,
    pub b: Option<f32>,
}

I expected to see this happen: It would compile on both or not compile on both.

Instead, this happened: It only compiles on 1.64 (which surprised me given I didn't think f32 impl'd Eq).

Version it worked on

This compiled on Rust 1.64, but did not compile on Rust 1.65.

It most recently worked on: Rust 1.64

Version with regression

rustc --version --verbose:

brendondaugherty@C6W2M6LRF4 f32_not_eq % rustc --version --verbose
rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: aarch64-apple-darwin
release: 1.65.0
LLVM version: 15.0.0

Backtrace

The compiler did not crash.

Backtrace

<backtrace>

@BKDaugherty BKDaugherty added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 21, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 21, 2023
@saethlin saethlin added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 21, 2023
@compiler-errors
Copy link
Member

compiler-errors commented Aug 21, 2023

Yeah, the code you provided should have never passed in the first place. f32 does not implement Eq intentionally, and it seems like a bug of the Eq derive that was fixed sometime between 1.64 -> 1.65.

Looks like the derived Eq impl changed:

// 1.64:
impl ::core::cmp::Eq for WithBool {
    fn assert_receiver_is_total_eq(&self) -> () {
        let _: ::core::cmp::AssertParamIsEq<Option<bool>>;
    }
}

// 1.65
impl ::core::cmp::Eq for WithBool {
    fn assert_receiver_is_total_eq(&self) -> () {
        let _: ::core::cmp::AssertParamIsEq<Option<bool>>;
        let _: ::core::cmp::AssertParamIsEq<Option<f32>>;
    }
}

@compiler-errors
Copy link
Member

compiler-errors commented Aug 21, 2023

So this actually regressed from fail -> pass in 1.64: #98758

Then was fixed in 1.65 in #103176

I'm gonna close this PR because it's clear where the regression occurred and then was fixed according to those PRs above. Thanks for the bug report, and sorry that rustc mistakenly accepted your buggy code. Hope it's not too hard to fix 😅 ❤️

@BKDaugherty
Copy link
Author

Ez to fix! Thanks for showing me the paper trail and quick response! Cool to see the context!

@apiraino apiraino removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants