Skip to content

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