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

cmp::max changed behavior in nightly #64710

Closed
emilio opened this issue Sep 23, 2019 · 5 comments
Closed

cmp::max changed behavior in nightly #64710

emilio opened this issue Sep 23, 2019 · 5 comments

Comments

@emilio
Copy link
Contributor

emilio commented Sep 23, 2019

This program (playground) changed behavior between 97e58c0...1dd1884.

And broke bindgen as a result in funny ways.

use std::cmp;

#[derive(Copy, Clone, Debug, PartialEq, Eq, Ord)]
pub enum SizednessResult {
    NonZeroSized,
    DependsOnTypeParam,
    ZeroSized,
}

impl cmp::PartialOrd for SizednessResult {
    fn partial_cmp(&self, rhs: &Self) -> Option<cmp::Ordering> {
        use self::SizednessResult::*;

        match (*self, *rhs) {
            (x, y) if x == y => Some(cmp::Ordering::Equal),
            (NonZeroSized, _) => Some(cmp::Ordering::Greater),
            (_, NonZeroSized) => Some(cmp::Ordering::Less),
            (DependsOnTypeParam, _) => Some(cmp::Ordering::Greater),
            (_, DependsOnTypeParam) => Some(cmp::Ordering::Less),
            _ => unreachable!(),
        }
    }
}

fn main() {
    println!("{:?}", cmp::max(SizednessResult::ZeroSized, SizednessResult::NonZeroSized))
}

I suspect #64047.

@emilio
Copy link
Contributor Author

emilio commented Sep 23, 2019

cc @timvermeulen

@emilio
Copy link
Contributor Author

emilio commented Sep 23, 2019

Sorry, changelog is 97e58c0...ed8b708, which discards the llvm upgrade.

@emilio
Copy link
Contributor Author

emilio commented Sep 23, 2019

Looking at this further, I think what's going on is that Ord and PartialOrd don't agree. This may just be a bug in bindgen, who expected (looks like, I didn't write that code) PartialOrd::partial_cmp to be called instead of the Ord trait...

@Centril
Copy link
Contributor

Centril commented Sep 23, 2019

@emilio which means that this issue should be closed since it's a contract violation on bindgen's end?

@SimonSapin
Copy link
Contributor

Yes, I think this std change only exposed an existing bug in bindgen. While it is a behavior change, it likely only occurs with contradictory impls:

https://doc.rust-lang.org/std/cmp/trait.Ord.html#how-can-i-implement-ord

Implementations of PartialEq, PartialOrd, and Ord must agree with each other.

Closing, thanks for investigating @emilio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants