rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98k stars 12.68k forks source link

1.81.0 not panic when running incorrect implementations of Ord #130178

Open wubin28 opened 1 month ago

wubin28 commented 1 month ago

<!-- Thank you for filing a bug report! 🐛 Please provide a short summary of the bug, along with any information you feel relevant to replicating the bug. -->

I tried this code by running rustup run 1.81.0 cargo run:

use std::cmp::Ordering;

#[derive(Debug)]
struct BadOrd(i32);

impl PartialEq for BadOrd {
    fn eq(&self, other: &Self) -> bool {
        // Intentionally inconsistent equality
        self.0 % 2 == other.0 % 2
    }
}

impl Eq for BadOrd {}

impl PartialOrd for BadOrd {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        // Violates consistency, transitivity, and duality
        if self.0 % 2 == 0 && other.0 % 2 != 0 {
            Some(Ordering::Less)
        } else if self.0 % 2 != 0 && other.0 % 2 == 0 {
            Some(Ordering::Greater)
        } else if self.0 == other.0 {
            Some(Ordering::Equal)
        } else {
            None
        }
    }
}

impl Ord for BadOrd {
    fn cmp(&self, other: &Self) -> Ordering {
        // Inconsistent with PartialOrd and violates total ordering
        if self.0 < other.0 {
            Ordering::Greater
        } else if self.0 > other.0 {
            Ordering::Less
        } else {
            Ordering::Equal
        }
    }
}

fn main() {
    let mut vec = vec![BadOrd(3), BadOrd(2), BadOrd(4), BadOrd(1)];

    println!("Before sorting: {:?}", vec);

    vec.sort(); // This will likely panic due to inconsistent ordering

    println!("After sorting: {:?}", vec);

    // These assertions will fail, demonstrating incorrect ordering
    assert!(BadOrd(1) < BadOrd(2));
    assert!(BadOrd(2) > BadOrd(1));
    assert!(BadOrd(2) == BadOrd(2));

    println!("All assertions passed!");
}

I expected to see this happen: explanation The program would panic on the line "vec.sort();" since "the new sort algorithms try to detect incorrect implementations of Ord that prevent them from being able to produce a meaningfully sorted result, and will now panic on such cases rather than returning effectively randomly arranged data.", according to the "New sort implementations" in https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html.

Instead, this happened: explanation The program paniced on the line "assert!(BadOrd(1) < BadOrd(2));". Here is the output:

Before sorting: [BadOrd(3), BadOrd(2), BadOrd(4), BadOrd(1)] After sorting: [BadOrd(2), BadOrd(4), BadOrd(3), BadOrd(1)] thread 'main' panicked at src/main.rs:53:5: assertion failed: BadOrd(1) < BadOrd(2) note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

Meta

rustc --version --verbose:

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.81.0
LLVM version: 18.1.7

rustc 1.82.0-beta.2 (c7c49f44a 2024-09-04)
binary: rustc
commit-hash: c7c49f44a7bb561dd9317e14908a1e50fa478ce5
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.82.0-beta.2
LLVM version: 19.1.0

rustc 1.83.0-nightly (c2f74c3f9 2024-09-09)
binary: rustc
commit-hash: c2f74c3f928aeb503f15b4e9ef5778e77f3058b8
commit-date: 2024-09-09
host: aarch64-apple-darwin
release: 1.83.0-nightly
LLVM version: 19.1.0
Backtrace

``` RUST_BACKTRACE=1 rustup run 1.81.0 cargo run Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.16s Running `target/debug/new_sort_implementations_in_1_81_0_stable_rust` Before sorting: [BadOrd(3), BadOrd(2), BadOrd(4), BadOrd(1)] After sorting: [BadOrd(2), BadOrd(4), BadOrd(3), BadOrd(1)] thread 'main' panicked at src/main.rs:53:5: assertion failed: BadOrd(1) < BadOrd(2) stack backtrace: 0: rust_begin_unwind at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5 1: core::panicking::panic_fmt at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14 2: core::panicking::panic at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:148:5 3: new_sort_implementations_in_1_81_0_stable_rust::main at ./src/main.rs:53:5 4: core::ops::function::FnOnce::call_once at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. ```

ryanavella commented 1 month ago

The docs for sort have this to say:

May panic if the implementation of Ord for T does not implement a total order.

The release announcement you have linked is a little more vague, but I interpret the "will now panic" portion as "will now panic [in some cases where it didn't previously]."

EDIT: #130122 seems to be related, where there was some confusion over the new wording in the docs