Fix manual_is_variant_and condition generation

[samueltardieu]

When comparing x.map(func) == Some(bool_lit), the value of bool_lit was ignored, despite the fact that its value should determine the value of the proposed expression.

func can be either a closure or a path. For the latter, η-expansion will be used if needed to invert the result of the function call.

changelog: [manual_is_variant_and]: fix inverted suggestions that could lead to code with different semantics

Fixes #15202

Summary Notes

• Beta-nomination by samueltardieu

Managed by @rustbot—see help for details

[rustbot]

r? @y21

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

[samueltardieu]

@rustbot note Beta-nomination
@rustbot label +beta-nominated +I-suggestion-causes-bug

Those extensions to manual_is_variant_and were introduced in Rust 1.89, now in beta. They cause compilable suggestions with bogus semantics. It would be preferable that they don't reach the stable version.

[samueltardieu]

Lintcheck run shows one fixed diagnostic which would have caused a bug if applied as-is (in cargo sources).

[GuillaumeGomez]

It's pretty common for closure/function paths to be used and needing to be handled by clippy. We should maybe provide a utils function to handle it, no?

[samueltardieu]

It's pretty common for closure/function paths to be used and needing to be handled by clippy. We should maybe provide a utils function to handle it, no?

Yes, probably, but usages may be different, for example here we need to invert the boolean result of the closure/function.

Btw, my latest push is only some cleanup to ease later maintenance, no change in functionality.

[GuillaumeGomez]

It's pretty common for closure/function paths to be used and needing to be handled by clippy. We should maybe provide a utils function to handle it, no?

Yes, probably, but usages may be different, for example here we need to invert the boolean result of the closure/function.

Having a function to generate an enum closure/path would likely be a good first step and also likely to be useable everywhere. For the more specific parts, we can see afterwards.

[samueltardieu]

Having a function to generate an enum closure/path would likely be a good first step and also likely to be useable everywhere. For the more specific parts, we can see afterwards.

An Expr variant for arbitrary expressions would also have to be added in addition to closure + path, as in:

fn make_func() -> impl Fn(u32) -> u32 {
    |x| x+1
}

fn main() {
    let a = Some(41);
    println!("{:?}", a.map(make_func()));
}

[samueltardieu]

Anyway, I agree with your proposal. However, I would like to see it implemented after this is merged, so that this can be backported to beta, rather than before, because the backport would be much more involved.

[GuillaumeGomez]

Not planning to do it right away. I plan to move your current enum, so I need this PR to be merge first. :3

[y21]

I have to say this case stretches the "readability" point of the lint's suggestion a little bit IMO 😄 This took me a bit to mentally parse and prove that it's equivalent. The error message of the lint is also kinda terse and doesn't add any extra information.

But, those are pre-existing issues I guess, so not something we have to figure out here

[samueltardieu]

@y21 I made the change you requested, is it ok to merge it so that it gets synced tomorrow? I'd like it to get a larger test base since it should be backported in beta before the release.

[y21]

Sorry for the wait, yeah looks good to me now 👍

[flip1995]

This is too big of a PR to backport to beta. Instead, in the beta backport PR I'll move this lint to nursery.

[samueltardieu]

This is too big of a PR to backport to beta. Instead, in the beta backport PR I'll move this lint to nursery.

I understand, but this is unfortunate. Wouldn't it be possible to revert the problematic commit which introduced the problem instead, so that we don't put the whole lint into nursery?

That would be commit 763a7bd

[flip1995]

I'll try. If the revert goes through cleanly, I'll do that instead.

[flip1995]

rust-lang/rust#144742