Skip to content

1.81.0 not panic when running incorrect implementations of Ord #130178

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wubin28 opened this issue Sep 10, 2024 · 1 comment
Open

1.81.0 not panic when running incorrect implementations of Ord #130178

wubin28 opened this issue Sep 10, 2024 · 1 comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@wubin28
Copy link

wubin28 commented Sep 10, 2024

<!--
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.

@wubin28 wubin28 added the C-bug Category: This is a bug. label Sep 10, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 10, 2024
@ryanavella
Copy link

ryanavella commented Sep 10, 2024

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

@saethlin saethlin added T-libs Relevant to the library team, which will review and decide on the PR/issue. C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 10, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants