implement PartialEq<VecDeque<U>> for Vec<T>, &[T], &mut [T], [T; N], &[T; N] and &mut [T; N]

[iAeternus]

View all comments

This PR is a response to #152830. It implement PartialEq<VecDeque> for Vec, &[T], &mut [T], [T; N] and &mut [T; N]. To be symmetrical with the standard library's __impl_slice_eq1, I added the __impl_slice_eq2 macro to generate the implementation. Additionally, I added the reverse equality unit test (test_partial_eq_vecdeque_reverse), modified too_generic_eval_ice.current.stderr, and passed the local tests.
This PR does not involve the comparison between Cow vs Vec and Cow vs &mut [T].
PS:
This is my first time participating in this project. If there are any mistakes, please feel free to correct me.

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates
• Random selection from Mark-Simulacrum, jhpratt, joboet

[workingjubilee]

...who says "corrigendum"? This isn't an academics paper, this is a PR. Your PR's description should be as succinct as you can make it, and written by hand in your own words. See https://forge.rust-lang.org/how-to-start-contributing.html#etiquette

[workingjubilee]

This PR is already addressed by #149045

[workingjubilee]

Wait, this isn't addressed by that, this is VecDeque not Vec.

[joboet]

r? libs-api

This will need an FCP.

This is my first time participating in this project. If there are any mistakes, please feel free to correct me.

Welcome! As it stands, the PR description is very bloated and reads like the LLM-generated robot-speak that it is (if you read it through, every information is repeated twice). That comes across as quite rude; after all, why should anyone spend time on your work if you didn't invest your?

So please, write your contributions yourself. As far as I can tell from your profile, English isn't your first language (it isn't for me either). Using translation tools is completely fine, but please make sure that the generated output is close to what you had originally written. Try prompting the LLM to keep that in mind – context matters, even and especially for mindless stochastic parrots. And in the end, even the most nuanced prompt cannot and should not replace a manual review of the output.

@rustbot author
until then.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[iAeternus]

r? libs-api

This will need an FCP.

This is my first time participating in this project. If there are any mistakes, please feel free to correct me.

Welcome! As it stands, the PR description is very bloated and reads like the LLM-generated robot-speak that it is (if you read it through, every information is repeated twice). That comes across as quite rude; after all, why should anyone spend time on your work if you didn't invest your?

So please, write your contributions yourself. As far as I can tell from your profile, English isn't your first language (it isn't for me either). Using translation tools is completely fine, but please make sure that the generated output is close to what you had originally written. Try prompting the LLM to keep that in mind – context matters, even and especially for mindless stochastic parrots. And in the end, even the most nuanced prompt cannot and should not replace a manual review of the output.

@rustbot author until then.

Thank you for your suggestion. I have rewritten the PR description. From now on, I will describe the PR in my own words.

[iAeternus]

@rustbot ready

[iAeternus]

Starting a crater run to check if this breaks type inference in the ecosystem.

@bors try

[rust-bors]

@iAeternus: 🔑 Insufficient privileges: not in try users

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[iAeternus]

I have re-examined whether __impl_slice_eq2! is necessary in this PR. __impl_slice_eq1! assumes that self is a VecDeque, since it calls self.as_slices(). In the reversed PartialEq implementations, Vec, slices, and arrays are on the self side, while VecDeque is on the other side. Therefore, the existing __impl_slice_eq1! cannot be reused simply by swapping the macro parameters.
I considered three possible approaches:

1. Add a separate __impl_slice_eq2! macro.
2. Add another rule to __impl_slice_eq1!, for example: ([$($vars:tt)*] reverse $lhs:ty, $rhs:ty, $($constraints:tt)*) and invoke it as: __impl_slice_eq1! { [] reverse Vec<T, A>, VecDeque<U, A>, }
3. Extract the duplicated logic from __impl_slice_eq1! and __impl_slice_eq2! into a helper.

I chose the first approach because, for such a small patch, a separate macro for the reversed comparison is straightforward and easy to understand. The other approaches introduce additional complexity without providing much benefit here. Given the standard library's preference for keeping the implementation simple and localized, I think the first approach is sufficient.

[iAeternus]

Updated the PR title to include the missing &[T; N]. This implementation was already included in the PR; it was simply omitted from the title.

[iAeternus]

Hi @dtolnay, just following up on this PR.

The Cow half of #152830 was merged in #156160, while the VecDeque half is still handled separately by this PR. The implementation has been rebased and updated, and the PR is ready for review.

@joboet previously mentioned that this change will need an FCP. Please let me know if there is anything else I should do to move this forward.

[clarfonthey]

r? libs

@rustbot label I-libs-nominated

We should be fine just discussing this at the meeting, but most likely we will need a crater run to verify this doesn't cause inference failures.

[iAeternus]

I previously tried to start a crater run for this PR, but I don't have "@bors try" permissions.

[rust-bors]

Unknown command "try"". Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

[clarfonthey]

@craterbot check

[craterbot]

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ask in t-infra on Zulip
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[clarfonthey]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: e4d5758 (e4d5758286bf2cf6f415802f2cf861747a7c6c8c)
Base parent: c999cef (c999cef531ea9059e189e82fe0e82c5daf249bc9)

[clarfonthey]

@craterbot check

[craterbot]

👌 Experiment pr-152972 created and queued.
🤖 Automatically detected try build e4d5758
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[iAeternus]

One thing I noticed: __impl_slice_eq2! currently uses the same stability attributes as __impl_slice_eq1!. I originally copied the attribute over when adding the new macro and didn't notice this.
I noticed that #156160 used a new stability feature with CURRENT_RUSTC_VERSION for the newly added reverse impls. I wonder if we should follow the same approach here.

[clarfonthey]

Yes, you should. The documentation should reflect the latest version.

[clarfonthey]

(note: the error is because you are using the same stability attribute for the new and old impls. I think you might need to move the attribute to be part of the macro call instead to fix this)