Suggest swapping LHS and RHS when RHS impls `PartialEq<lhs_ty>` by makai410 · Pull Request #132404 · rust-lang/rust

makai410 · 2024-10-31T12:01:10Z

Closes: #130495
r? @fee1-dead

fee1-dead

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

compiler/rustc_hir_typeck/src/op.rs

tests/ui/partialeq_suggest_swap.rs

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

makai410 · 2024-11-01T16:34:54Z

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

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

tests/ui/partialeq_suggest_swap.rs

makai410 · 2024-11-04T15:33:11Z

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`.

fee1-dead · 2024-11-05T02:54:22Z

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

makai410 · 2024-11-05T03:15:57Z

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

rust/library/core/src/cmp.rs

Lines 238 to 249 in fbab782

    
           #[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 · 2024-11-06T10:16:30Z

@bors r+

bors · 2024-11-06T10:16:34Z

📌 Commit 5791a66 has been approved by fee1-dead

It is now in the queue for this repository.

fee1-dead

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

bors · 2024-11-06T11:49:56Z

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

bors · 2024-11-06T14:28:32Z

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

rust-timer · 2024-11-06T16:28:28Z

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%)

rustbot assigned fee1-dead Oct 31, 2024

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 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 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 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

fee1-dead requested changes Oct 31, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/op.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_typeck/src/op.rs Outdated Show resolved Hide resolved

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 force-pushed the suggest-swap-lhs-rhs branch from 6998c73 to 1048e02 Compare November 1, 2024 07:54

makai410 commented Nov 1, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/op.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

makai410 force-pushed the suggest-swap-lhs-rhs branch from 1048e02 to bc41d76 Compare November 1, 2024 08:28

makai410 requested a review from fee1-dead November 1, 2024 09:45

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

fee1-dead requested changes Nov 1, 2024

View reviewed changes

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 force-pushed the suggest-swap-lhs-rhs branch from bc41d76 to 739f29c Compare November 1, 2024 16:27

makai410 requested a review from fee1-dead November 2, 2024 03:32

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 force-pushed the suggest-swap-lhs-rhs branch from 739f29c to ca81f0b Compare November 2, 2024 07:48

fee1-dead requested changes Nov 3, 2024

View reviewed changes

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 force-pushed the suggest-swap-lhs-rhs branch from ca81f0b to 7eadcaf Compare November 4, 2024 07:40

makai410 requested a review from fee1-dead November 4, 2024 14:02

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

suggest swapping the equality

5791a66

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

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

fee1-dead approved these changes Nov 6, 2024

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2024

bors merged commit 4d215e2 into rust-lang:master Nov 6, 2024

rustbot added this to the 1.84.0 milestone Nov 6, 2024

makai410 deleted the suggest-swap-lhs-rhs branch November 6, 2024 14:35

makai410 mentioned this pull request Nov 6, 2024

Suggest swapping the equality when the message can't compare occurs #132695

Closed

bors mentioned this pull request Nov 6, 2024

Suggest using matches or adding == on x == a || b || c #128159

Closed

Uh oh!

Conversation

makai410 commented Oct 31, 2024

Uh oh!

fee1-dead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

makai410 commented Nov 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

makai410 commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fee1-dead commented Nov 5, 2024

Uh oh!

makai410 commented Nov 5, 2024

Uh oh!

fee1-dead commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

fee1-dead left a comment

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

rust-timer commented Nov 6, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

makai410 commented Nov 4, 2024 •

edited

Loading