Skip to content

fix: add fallback case in generated PartialEq impl #13732

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

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Dec 6, 2022

Partially fixes #13727.

When generating PartialEq implementations for enums, the original code can already generate the following fallback case:

_ => std::mem::discriminant(self) == std::mem::discriminant(other),

However, it has been suppressed in the following example for no good reason:

enum Either<T, U> {
    Left(T),
    Right(U),
}

impl<T, U> PartialEq for Either<T, U> {
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Self::Left(l0), Self::Left(r0)) => l0 == r0,
            (Self::Right(l0), Self::Right(r0)) => l0 == r0,
            // _ => std::mem::discriminant(self) == std::mem::discriminant(other),
            // ^ this completes the match arms!
        }
    }
}

This PR has removed that suppression logic.

Of course, the PR could have suppressed the fallback case generation for single-variant enums instead, but I believe that this case is quite rare and should be caught by #[warn(unreachable_patterns)] anyway.

After this fix, when the enum has >1 variants, the following fallback arm will be generated :

  • _ => false, if we've already gone through every case where the variants of self and other match;
  • The original one (as stated above) in other cases.

Note: The code example is still wrong after the fix due to incorrect trait bounds.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2022
@rami3l rami3l force-pushed the fix/gen-partial-eq branch from b44f07a to fed74c8 Compare December 6, 2022 13:55
@rami3l rami3l requested a review from lowr December 7, 2022 05:33
@lowr
Copy link
Contributor

lowr commented Dec 8, 2022

LGTM :) Maintainers that have write permissions should review in a few days.

@jonas-schievink
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 12, 2022

📌 Commit 57fb18e has been approved by jonas-schievink

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 12, 2022

⌛ Testing commit 57fb18e with merge 3a7215b...

@bors
Copy link
Contributor

bors commented Dec 12, 2022

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing 3a7215b to master...

@bors bors merged commit 3a7215b into rust-lang:master Dec 12, 2022
@lnicola
Copy link
Member

lnicola commented Dec 12, 2022

changelog fix (first contribution) add fallback case to generated PartialEq impl

@rami3l rami3l deleted the fix/gen-partial-eq branch December 12, 2022 15:11
bors added a commit that referenced this pull request Jan 9, 2023
fix: add generic `TypeBoundList` in generated derivable impl

Potentially fixes #13727.

Continuing with the work in #13732, this fix tries to add correct type bounds in the generated `impl` block:

```diff
  enum Either<T, U> {
      Left(T),
      Right(U),
  }

- impl<T, U> PartialEq for Either<T, U> {
+ impl<T: PartialEq, U: PartialEq> PartialEq for Either<T, U> {
      fn eq(&self, other: &Self) -> bool {
          match (self, other) {
              (Self::Left(l0), Self::Left(r0)) => l0 == r0,
              (Self::Right(l0), Self::Right(r0)) => l0 == r0,
              _ => false,
          }
      }
  }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect PartialEq code assist
6 participants