Skip to content

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Apr 17, 2025

Fixes #14542.

changelog: [manual_is_variant_and]: Extend to check for boolean map comparisons

r? @samueltardieu

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2025

samueltardieu is on vacation.

Please choose another assignee.

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
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 Apr 17, 2025
@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from 0b94b9e to 8913c0e Compare April 17, 2025 15:55
@GuillaumeGomez
Copy link
Member Author

Fixed fmt

@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from 8913c0e to f65ce46 Compare April 18, 2025 08:42
@GuillaumeGomez
Copy link
Member Author

Let's reroll reviewer.

r? clippy

@rustbot rustbot assigned Centri3 and unassigned blyxyas May 2, 2025
@GuillaumeGomez
Copy link
Member Author

r? samueltardieu

@rustbot rustbot assigned samueltardieu and unassigned Centri3 May 26, 2025
Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

This can probably be simplified quite a bit, see inner comment.

Could you please also add a test with a generic type which takes a generic parameter such as bool and implements a map(), so that we can check that types are correctly filtered?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 26, 2025
@samueltardieu
Copy link
Member

Could you also add a test where the left, ==/!= and right expressions come from different contexts? I think you would need to check that they are all in the same context as expr.

@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from f65ce46 to 20590c9 Compare May 26, 2025 19:43
@GuillaumeGomez
Copy link
Member Author

Just thought about a case where we're comparing to a function returned value. In this case we don't want to lint so I strengthened the check to cover this case.

I also added macro check, not sure if it's what you had in mind though?

@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 from the author. (Use `@rustbot ready` to update this status) labels May 26, 2025
@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from 20590c9 to ed3c902 Compare May 26, 2025 19:47
Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

In the following tests:

macro_rules! mac {
    (some $e:expr) => { Some($e) };
    (some_map $e:expr) => { Some($e).map(|x| x % 2 == 0) };
    (map $e:expr) => { $e.map(|x| x % 2 == 0) };
    (eq $a:expr, $b:expr) => { $a == $b };
}

    let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
    let _ = mac!(some_map 2) == Some(true);
    let _ = mac!(map Some(2)) == Some(true);
    let _ = mac!(eq Some(2).map(|x| x % 2 == 0), Some(true));

the latest seems to fail. It should not lint when the equality operator comes from expansion. Comparing the contexts and parent_expr and expr should be enough to fix this.

Also, could you please merge your tests into manual_is_variant_and.rs rather than creating a new test file?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 27, 2025
@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from ed3c902 to 494605f Compare May 27, 2025 10:09
@GuillaumeGomez
Copy link
Member Author

Sure, done!

@GuillaumeGomez GuillaumeGomez force-pushed the extend-manual_is_variant_and branch from 494605f to 634f875 Compare May 27, 2025 10:15
@samueltardieu samueltardieu added this pull request to the merge queue May 28, 2025
Merged via the queue into rust-lang:master with commit 551870d May 28, 2025
11 checks passed
@GuillaumeGomez GuillaumeGomez deleted the extend-manual_is_variant_and branch May 28, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manual_is_variant_and should also trigger for some_option.map(|x| x.is_foo) == Some(true)
5 participants