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

Suggest swapping LHS and RHS when RHS impls PartialEq<lhs_ty> #132404

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

makai410
Copy link
Contributor

Closes: #130495
r? @fee1-dead

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 31, 2024
@makai410 makai410 changed the title Suggest swapping LHS and RHS for PartialEq Suggest swapping the LHS and RHS of the equality when the RHS impls 'PartialEq<T>' Oct 31, 2024
@makai410 makai410 changed the title Suggest swapping the LHS and RHS of the equality when the RHS impls 'PartialEq<T>' Suggest swapping the LHS and RHS of the equality when the RHS impls PartialEq<lhs_ty> Oct 31, 2024
@makai410 makai410 changed the title Suggest swapping the LHS and RHS of the equality when the RHS impls PartialEq<lhs_ty> Suggest swapping LHS and RHS when RHS impls PartialEq<lhs_ty> Oct 31, 2024
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add an UI test as described in https://rustc-dev-guide.rust-lang.org/tests/ui.html

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2024
@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from 6998c73 to 1048e02 Compare November 1, 2024 07:54
@rust-log-analyzer

This comment has been minimized.

@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from 1048e02 to bc41d76 Compare November 1, 2024 08:28
@makai410 makai410 requested a review from fee1-dead November 1, 2024 09:45
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2024
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2024
@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from bc41d76 to 739f29c Compare November 1, 2024 16:27
@makai410
Copy link
Contributor Author

makai410 commented Nov 1, 2024

Thank you for the detailed and thoughtful review! Your suggestions really helped improve the code. Much appreciated.

@makai410 makai410 requested a review from fee1-dead November 2, 2024 03:32
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2024
@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from 739f29c to ca81f0b Compare November 2, 2024 07:48
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2024
@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from ca81f0b to 7eadcaf Compare November 4, 2024 07:40
@makai410 makai410 requested a review from fee1-dead November 4, 2024 14:02
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2024
@makai410
Copy link
Contributor Author

makai410 commented Nov 4, 2024

Just found when the integer type is not explicitly specified, the suggestion will not be emitted; instead, the following diagnostic message will be emitted:

error[E0277]: can't compare `{integer}` with `T`
  --> $DIR/partialeq_suggest_swap.rs:10:9
   |
LL |     123 == T(123);
   |         ^^ no implementation for `{integer} == T`
   |
   = help: the trait `PartialEq<T>` is not implemented for `{integer}`
   = help: the following other types implement trait `PartialEq<Rhs>`:
             f128
             f16
             f32
             f64
             i128
             i16
             i32
             i64
           and 8 others

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.

I think we should consider this case, as 123 will be inferred as i32, making T(123) == 123 valid in this context, as far as I can tell.

Also the similar case is when the type is String:

error[E0277]: can't compare `String` with `T`
  --> $DIR/partialeq_suggest_swap.rs:10:26
   |
LL |     String::from("text") == T(String::from("text"));
   |                          ^^ no implementation for `String == T`
   |
   = help: the trait `PartialEq<T>` is not implemented for `String`
   = help: the following other types implement trait `PartialEq<Rhs>`:
             `String` implements `PartialEq<&str>`
             `String` implements `PartialEq<Cow<'_, str>>`
             `String` implements `PartialEq<str>`
             `String` implements `PartialEq`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.

@makai410 makai410 force-pushed the suggest-swap-lhs-rhs branch from 7eadcaf to 5791a66 Compare November 5, 2024 02:10
@fee1-dead
Copy link
Member

no, that case does not need to be handled right now

@makai410
Copy link
Contributor Author

makai410 commented Nov 5, 2024

The message can't compare {Self} with {Rhs} is defined in the standard library via #[rustc_on_unimplemented]

#[lang = "eq"]
#[stable(feature = "rust1", since = "1.0.0")]
#[doc(alias = "==")]
#[doc(alias = "!=")]
#[rustc_on_unimplemented(
message = "can't compare `{Self}` with `{Rhs}`",
label = "no implementation for `{Self} == {Rhs}`",
append_const_msg
)]
#[rustc_diagnostic_item = "PartialEq"]
pub trait PartialEq<Rhs: ?Sized = Self> {
/// Tests for `self` and `other` values to be equal, and is used by `==`.

So we may have to extend the #[rustc_on_unimplemented] if we want this suggestion to be emitted.

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 6, 2024

📌 Commit 5791a66 has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2024
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks. The remaining cases could be done as a followup.

@bors
Copy link
Contributor

bors commented Nov 6, 2024

⌛ Testing commit 5791a66 with merge 4d215e2...

@bors
Copy link
Contributor

bors commented Nov 6, 2024

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 4d215e2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2024
@bors bors merged commit 4d215e2 into rust-lang:master Nov 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 6, 2024
@makai410 makai410 deleted the suggest-swap-lhs-rhs branch November 6, 2024 14:35
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4d215e2): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.4% [-4.1%, -2.6%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.5% [-4.1%, 2.1%] 3

Cycles

Results (primary -1.3%, secondary -5.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.3% [-2.0%, -0.9%] 3
Improvements ✅
(secondary)
-5.8% [-7.9%, -2.2%] 3
All ❌✅ (primary) -1.3% [-2.0%, -0.9%] 3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.805s -> 780.212s (-0.08%)
Artifact size: 335.24 MiB -> 335.27 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest swapping LHS and RHS for PartialEq
6 participants