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

fix: Correctly generate default PartialEq::ne #12821

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

SpecialMike
Copy link
Contributor

Fixes #12779

For the Generate default members assist on the PartialEq trait, the assist will now give the default implementation instead of generating a function.

@Veykril
Copy link
Member

Veykril commented Jul 20, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2022

📌 Commit 1c32fcf has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 20, 2022

⌛ Testing commit 1c32fcf with merge c001b9c...

@bors
Copy link
Contributor

bors commented Jul 20, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing c001b9c to master...

@bors bors merged commit c001b9c into rust-lang:master Jul 20, 2022
@lowr
Copy link
Contributor

lowr commented Jul 20, 2022

Sorry to bother after this is merged, but I think this is inadequate. While this fixes the exact problem described in #12779, Implement default members on PartialOrd gives you wrong implementation as well.

Considering those codgen don't take traits' default members into account, I'd argue they were never intended for Implement default members but only for Implement missing members. How about not calling try_gen_trait_body() for Implement default members instead of filtering out every default members in codegen?

@lnicola
Copy link
Member

lnicola commented Jul 20, 2022

Implement default members on PartialOrd gives you wrong implementation as well.

Wrong? Just wondering, I'm probably missing something.

@lowr
Copy link
Contributor

lowr commented Jul 20, 2022

I get the following with the latest commit:

struct S { data: i32 }

// generated by `Implement default members`
impl PartialOrd for S {
    fn lt(&self, other: &Self) -> bool {
        self.data.partial_cmp(&other.data)
    }

    fn le(&self, other: &Self) -> bool {
        // Pattern `Some(Less | Eq)` optimizes worse than negating `None | Some(Greater)`.
        // FIXME: The root cause was fixed upstream in LLVM with:
        // https://github.com/llvm/llvm-project/commit/9bad7de9a3fb844f1ca2965f35d0c2a3d1e11775
        // Revert this workaround once support for LLVM 12 gets dropped.
        !matches!(self.partial_cmp(other), None | Some(Greater))
    }

    fn gt(&self, other: &Self) -> bool {
        matches!(self.partial_cmp(other), Some(Greater))
    }

    fn ge(&self, other: &Self) -> bool {
        matches!(self.partial_cmp(other), Some(Greater | Equal))
    }
}

The body of PartialOrd::lt() is for PartialOrd::partial_cmp(). I expect that to be like this: matches!(self.partial_cmp(other), Some(Less)).

@lnicola
Copy link
Member

lnicola commented Jul 20, 2022

Oh, right, good point.

@SpecialMike
Copy link
Contributor Author

I had that same thought this morning about other traits, I can take on the change to not call try_gen_trait_body for default members as well. Does a new issue need to be made for this, or should the original issue be reopened?

@lowr
Copy link
Contributor

lowr commented Jul 20, 2022

@SpecialMike I should have mentioned I was already working on this, sorry about that.

bors added a commit that referenced this pull request Jul 24, 2022
…ykril

fix: don't replace default members' body

cc #12779, #12821
addresses #12821 (comment)

`gen_trait_fn_body()` only attempts to implement required trait member functions, so we shouldn't call it for `Implement default members` assist.

This patch also documents the precondition of `gen_trait_fn_body()` and inserts `debug_assert!`, but I'm not entirely sure if the assertions are appropriate.
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

Successfully merging this pull request may close these issues.

Implement default members implements PartialEq::ne incorrectly
5 participants