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

Add new eq_should_be_match lint #14435

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #13791.

Details on why, for performance reasons, we should add this lint are described in the issue. So in short, if it's not a primitive check, it suggests to use matches!. The "funny" part was making it work to suggest grouping matches! suggestions when possible.

changelog: Add new eq_should_be_match lint

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @dswij

rustbot has assigned @dswij.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 18, 2025
@GuillaumeGomez
Copy link
Member Author

r? @samueltardieu

@rustbot rustbot assigned samueltardieu and unassigned dswij Mar 18, 2025
@samueltardieu
Copy link
Contributor

I haven't looked at the code in details yet, but I don't seem to see where you ensure that equality and matching is equivalent. Shouldn't this work only when PartialEq is auto-derived?

It looks like this would trigger the lint in places where it shouldn't (untested), as equality and match are no longer equivalent:

impl PartialEq for Foo {
    fn eq(&self, other: &Self) -> bool {
        self.first == other.first
    }
}

@samueltardieu
Copy link
Contributor

samueltardieu commented Mar 19, 2025

Also, in which case is it faster, and on which platforms? I just tried the following code, and compare_eq and compare_match generate the very same code, and are even unified to one function with two names:

#[inline(never)]
#[unsafe(no_mangle)]
pub fn compare_eq(a: [u32; 2]) -> bool {
    a == [10, 20]
}

#[inline(never)]
#[unsafe(no_mangle)]
pub fn compare_match(a: [u32; 2]) -> bool {
  matches!(a, [10, 20])
}

When compiled with optimizations, on x86_64 as well as aarch64, the assembly contains:

	.globl	compare_match
	.type	compare_match,@function
.set compare_match, compare_eq

@Alexendoo
Copy link
Member

There can be a difference in codegen, e.g. with slices instead of arrays or with tweaked array types ([u64; 2]). The original issue has an example under Evidence (collapsed)

But this style of micro optimisation should definitely not be a lint, there are infinite ways you can slightly tweak code to slightly change codegen and they vary by compiler version

If one version is worse codegen than the other then this should be opened as a rust-lang/rust issue

@Alexendoo Alexendoo closed this Mar 30, 2025
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
5 participants