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

Lint that encourages people to implement PartialOrd in terms of Ord when applicable #10744

Closed
JoJoJet opened this issue May 4, 2023 · 1 comment · Fixed by #10788
Closed
Assignees
Labels
A-lint Area: New lints

Comments

@JoJoJet
Copy link

JoJoJet commented May 4, 2023

What it does

When a type manually implements both PartialOrd and Ord, both implementations must agree i.e. behave the same way. Usually, the function bodies of ord and partial_ord are identical, save for wrapping the return expression in Some() for partial_ord. This is unnecessarily repetitive and makes the code more prone to error during refactoring, as one may forget to update one of the implementations. The best way of implementing these traits manually is to implement PartialOrd in terms of Ord, which reduces code duplication and guarantees that they function in the same way.

This lint should not trigger if the bounds on the impl for Ord are more restrictive than the bounds on PartialOrd.

Real-world example: bevyengine/bevy#8529 (comment).

Lint Name

duplicate_manual_partial_ord_impl

Category

style, complexity

Advantage

  • The code is less repetitive.
  • Implementations of PartialOrd and Ord are guaranteed to agree. This is safer for future refactoring, since you can't forget to update one of the two impls.

Drawbacks

Will trigger on code that isn't broken which may be annoying, but following the lint's advice will always (as far as I can tell) make the code more robust.

Example

impl Ord for F {
    fn cmp(&self, other: &Self) -> Ordering {
        match self.a.cmp(&other.b) {
            // .. some complex logic
        }
    }
}

impl PartialOrd for F {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(match self.a.cmp(&other.b) {
            // .. some complex logic
        })
    }
}

Could be written as:

impl Ord for F { /* Unchanged */ }

impl PartialOrd for F {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}
@JoJoJet JoJoJet added the A-lint Area: New lints label May 4, 2023
@Centri3
Copy link
Member

Centri3 commented May 15, 2023

I feel like this could be in correctness, like with if_same_then_else and others

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants