Upgrade `equatable_if_let` to style #7777

HKalbasi · 2021-10-06T13:29:03Z

Continue of #7762

changelog: upgrade equatable_if_let to style and add pedantic lint equatable_matches

After reading some more about StructuralPartialEq I got that my previous understanding of it was completely wrong and it should be checked recursively. So now it can detect which pattern supports equality and that false positive is fixed.

Also I added a separate lint for matches! based on this comment. Lint for if let is style and lint for matches! is pedantic, but probably both of them need discussion.

rust-highfive · 2021-10-06T13:29:06Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

flip1995 · 2021-10-06T13:40:20Z

Not sure if this should be a warn-by-default lint. Doing something like

if let Some(42) = some_very.long().expr.chain[1..4].that_is().hard_to.read() { ... }

Is pretty common in Rust, especially for Options and Results and therefore not really harder to read IMO. I even prefer this version over

if some_very.long().expr.chain[1..4].that_is().hard_to.read() == Some(42) { ... }

because I can see at the start of the line what the expected value is, before reading the whole expression. I would even make this argument for other types than Option or Result.

So I rather tend towards pedantic here. But maybe that's just me.

HKalbasi · 2021-10-06T14:28:42Z

What about

if Some(42) == some_very.long().expr.chain[1..4].that_is().hard_to.read() { ... }

in this case? And also what do you think about

if let 42 = some_very.long().expr.chain[1..4].that_is().hard_to.read() { ... }

I'm agree that in the long case it doesn't make much difference. In these cases bottleneck of readablity is somewhere else.

Also there is a good amount of data in the dogfooding fixes of previous PR which we can see for deciding.

By the way, I don't now why CI is unhappy. It pass tests on my local system.

flip1995 · 2021-10-06T14:40:09Z

What about

if Some(42) == some_very.long().expr.chain[1..4].that_is().hard_to.read() { ... }

in this case? And also what do you think about

if let 42 = some_very.long().expr.chain[1..4].that_is().hard_to.read() { ... }

The first would go against the "yoda condition" argument in the lint documentation. It might be better, but the difference is 2 characters. Maybe it's just that I'm more used to seeing if let Some(_) ... and that's why this looks more natural to me. I just feel like this lint is pretty pedantic.

The second example looks weird to me, because you usually don't pattern match on just integers.

Looking thorugh the dogfood fixes, I especially liked the changes from if let Ordering::Greater = _ to if _ == Ordering::Greater and the like. I think only applying this lint on unit-variants and primitives can be warn-by-default. I don't really like it for more complex matches. pattern matching is such a core concept in Rust, that if let E::ABC(4, 2) = expr just feels more natural to me than if E::ABC(4, 2) == expr.

Matching on a unit-variant is more like comparing (==) values (like integers), but matching on more complex things is more like pattern matching to me (if let).

Manishearth · 2021-10-06T15:29:48Z

Going to r? @flip1995 on this one since he's already posted a bit. I agree with him on this though.

HKalbasi · 2021-10-06T17:15:32Z

I see two things here:

Pattern before expression (Yoda condition) is more readable in some cases
Some values are pattern-ish, so equality is less readable in those cases

Generally, writing value first and pattern after reads better (When nine hundred years old you reach), but since in your example subject isn't readable itself, it doesn't improve anything. And yes, since we see if let everyday, it is not very strong argument but I think even now people prefer thing == Some(42) to Some(42) == thing (probably this is the reason some people write if matches! even though it is more verbose).

About the second point, I agree that there are some values that are more like patterns, but not all non unit variants. I feel Constant::Int(1) is definitely value, Some(42) is somehow value and Err(Error::Variant) feels more like pattern. So we should draw a line between pattern like and value like values and split it into two lints? Or just make it pedantic? I can't draw a good line because my feelings are not consistent but unit variant line seems better than nothing.

cc authors of issues @adamchalmers @clarfonthey if they have more to say.

camsteffen · 2021-10-06T20:18:10Z

I like the idea of style for unit-variants.

I have a theory that this may be a minor perf issue, if only for unoptimized builds. Using == requires two instances of a type where pattern matching is just reading (the discriminant of) a single instance.

enum MyEnum {
    LittleGuy,
    BigGuy([u8; 100]),
}

x == MyEnum::LittleGuy may be less optimal than if let MyEnum::LittleGuy = x in unoptimized builds. I think this could be reason enough to only lint (by default) on enums with all unit-like variants. And also reason enough to make anything else restriction.

HKalbasi · 2021-10-06T20:49:40Z

Code generated by #[derive(PartialEq)] for MyEnum is:

#[automatically_derived]
#[allow(unused_qualifications)]
impl ::core::cmp::PartialEq for MyEnum {
    #[inline]
    fn eq(&self, other: &MyEnum) -> bool {
        {
            let __self_vi = ::core::intrinsics::discriminant_value(&*self);
            let __arg_1_vi = ::core::intrinsics::discriminant_value(&*other);
            if true && __self_vi == __arg_1_vi {
                match (&*self, &*other) {
                    (&MyEnum::BigGuy(ref __self_0),
                     &MyEnum::BigGuy(ref __arg_1_0)) =>
                    (*__self_0) == (*__arg_1_0),
                    _ => true,
                }
            } else { false }
        }
    }
    #[inline]
    fn ne(&self, other: &MyEnum) -> bool {
        {
            let __self_vi = ::core::intrinsics::discriminant_value(&*self);
            let __arg_1_vi = ::core::intrinsics::discriminant_value(&*other);
            if true && __self_vi == __arg_1_vi {
                match (&*self, &*other) {
                    (&MyEnum::BigGuy(ref __self_0),
                     &MyEnum::BigGuy(ref __arg_1_0)) =>
                    (*__self_0) != (*__arg_1_0),
                    _ => false,
                }
            } else { true }
        }
    }
}

So if your point is it would compare 100 byte in debug builds, it doesn't seems so.

camsteffen · 2021-10-06T20:54:13Z

No my point is that it adds another 100 byte variable to the stack.

But I also think the non-unit cases may be too controversial for pedantic purely from a readability/style standpoint.

HKalbasi · 2021-10-06T21:19:22Z

hmm yes it seems pattern matching has better performance without optimization.

So I will change both of them to restriction.

flip1995 · 2021-10-07T08:45:24Z

I don't really see a blocker of a perf impact on unoptimized builds, as long as there is no perf impact for optimzed builds. So if the only argument for not putting the unit-variant-only version into style, then I would say put it into style anyway. That is, if we really want to split up the lint on the unit-variant boundary. If we want to keep everything in one lint, I don't have a strong opinion about pedantic vs restriction.

flip1995 · 2021-10-07T08:54:11Z

By the way, I don't now why CI is unhappy. It pass tests on my local system.

Forgot to reply to this yesterday: If you run the tests with cargo test --features internal-lints, you'll also see the test fails locally.

To run the same tests that are ran in CI, use cargo test --features internal-lints,deny-warnings,metadata-collector-lint

HKalbasi · 2021-10-07T11:56:12Z

split up the lint on the unit-variant boundary

Or maybe a sensitivity configuration with some levels like "primitive", "unit-variant" and "all", with "unit-variant" as default?

camsteffen · 2021-10-07T14:43:21Z

I don't really see a blocker of a perf impact on unoptimized builds

Well maybe my "unoptimized" point is kinda moot - that is actually a common tradeoff for abstractions in Rust.

Also I added a separate lint for matches!

Just noticed this. Don't forget to write a "changelog:".

Or maybe a sensitivity configuration with some levels like "primitive", "unit-variant" and "all", with "unit-variant" as default?

I guess that makes sense considering that it affects multiple lints? I'd be okay with that and pedantic.

flip1995 · 2021-10-07T15:53:41Z

Or maybe a sensitivity configuration with some levels like "primitive", "unit-variant" and "all", with "unit-variant" as default?

I guess that makes sense considering that it affects multiple lints? I'd be okay with that and pedantic.

Yes, I agree. A configuration for those lints is probably a good idea. The question is what the default should be. unit-variant (which I would just call unit)?

camsteffen · 2021-10-08T14:14:18Z

The question is what the default should be. unit-variant (which I would just call unit)?

I think that's a good default. I'm a little unsure about just "unit". equatable_patterns: unit (if that's a good config name?) makes me think ()...but I don't know...maybe simple or unit-like? I don't think there's a formalized name for it.

flip1995 · 2021-10-08T14:37:02Z

Ah right, that can be confusing. I think primitive, simple and all would be the best names.

HKalbasi · 2021-10-12T20:31:23Z

I added a configuration.

Just noticed this. Don't forget to write a "changelog:".

There is a changelog, but it needs update after we decide about style and pedantic.

bors · 2021-10-13T12:20:06Z

☔ The latest upstream changes (presumably #7788) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-10-29T20:46:36Z

☔ The latest upstream changes (presumably #7895) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995 · 2021-11-02T14:48:29Z

Oh wow, this discussion is already 3 weeks old. Sorry for the late reply!

I would say, keep the configuration, but put this lint into pedantic anyway. I just read through the discussion again and noticed that most arguments for putting this as warn-by-default started with "some people". And I also have the feeling that this is really opinionated and not clear that if x == Ordering::Greater is better style than if let Ordering::Greater = x.

flip1995 · 2021-11-02T14:56:15Z

clippy_utils/src/ast_utils.rs

@@ -278,7 +278,7 @@ pub fn eq_item_kind(l: &ItemKind, r: &ItemKind) -> bool {
        },
        (Trait(box TraitKind(la, lu, lg, lb, li)), Trait(box TraitKind(ra, ru, rg, rb, ri))) => {
            la == ra
-                && matches!(lu, Unsafe::No) == matches!(ru, Unsafe::No)
+                && matches!(lu, Unsafe::Yes(_)) == matches!(ru, Unsafe::Yes(_))


Wait, all the conditions are reversed in this file?

I at first changed it to lu == ru which was wrong (I assumed there is Unsafe::No and Unsafe::Yes), and this style better shows why it is wrong so next person won't try lu == ru.

flip1995 · 2021-11-02T14:59:05Z

tests/ui-toml/equatable_pattern/all.rs

@@ -0,0 +1,136 @@
+// run-rustfix


This seems to be a 1:1 copy of the original test file. But this file should rather test that the lint also works for the All variant and not so much that it still works for all other cases (one or two test cases are fine for that, but copying everything doesn't make much sense).

flip1995 · 2021-11-02T15:02:51Z

clippy_utils/src/higher.rs

+    pub exp: &'tcx Expr<'tcx>,
+}
+
+impl MatchesExpn<'tcx> {


Ah, we don't really want to keep doing this: #7843

Is this only required to generate the suggestion? If so, I'd prefer to just have a non-auto-applicable suggestion in the matches case with Applicability::HasPlaceholder

Parameters of matches! macro are needed to detect if this pattern is equatable.

Ah, that's unfortunate. We really have to figure out how to do this without relying on the exact expansion of the macro...

clippy_lints/src/utils/conf.rs

HKalbasi · 2021-11-03T06:49:00Z

I made it pedantic, but style with sensitivity = primitive is also an option. I think it is clear that if let "foo" = bar is bad style.

flip1995 · 2021-11-03T09:41:49Z

I made it pedantic, but style with sensitivity = primitive is also an option. I think it is clear that if let "foo" = bar is bad style.

I agree. I think the Simple config value is the better default though. So for me the question is, if style+primitive or pedantic+simple is the better option.

bors · 2021-11-05T14:55:43Z

☔ The latest upstream changes (presumably #7897) made this pull request unmergeable. Please resolve the merge conflicts.

camsteffen · 2021-11-08T00:59:06Z

Yet another option is to make a if_let_primitive lint for style. I think I'd like that option best. My second pick would be simple+pedantic. The biggest downside IMO is with the primitive+style option because a lot of users won't discover the config or care to change it.

Also I would put "aggregate primitives" in the "simple" category. Don't know if that has been considered.

HKalbasi · 2021-11-08T09:30:54Z

I don't know how many people use pedantic lints, but primitive+style would be better for default users than simple+pedantic. if_let_primitive (and possibly matches_primitive) are duplicate lints to me, but they will solve the problem of noise/visibility.

Also I would put "aggregate primitives" in the "simple" category. Don't know if that has been considered.

There are currently in "all".

blyxyas · 2023-10-06T15:51:55Z

Closing this because the original issue (#1716) was resolved by #9368

rust-highfive assigned Manishearth Oct 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 6, 2021

HKalbasi force-pushed the master branch from d0305b5 to cbbe9e9 Compare October 6, 2021 14:00

rust-highfive assigned flip1995 and unassigned Manishearth Oct 6, 2021

HKalbasi force-pushed the master branch from cbbe9e9 to edfd2b9 Compare October 7, 2021 11:41

HKalbasi force-pushed the master branch from c9531d0 to bf634cb Compare October 12, 2021 12:53

HKalbasi force-pushed the master branch 3 times, most recently from bfcc766 to c614402 Compare October 14, 2021 14:27

HKalbasi force-pushed the master branch 2 times, most recently from 298c8ed to 21dce73 Compare October 30, 2021 15:51

flip1995 reviewed Nov 2, 2021

View reviewed changes

HKalbasi force-pushed the master branch from 21dce73 to 5d40439 Compare November 3, 2021 06:27

upgrade equatable_if_let to style

57b620a

HKalbasi force-pushed the master branch from 5d40439 to 57b620a Compare November 3, 2021 06:34

dswij mentioned this pull request Sep 19, 2022

Improvement for equatable_if_let #9368

Merged

blyxyas closed this Oct 6, 2023

flip1995 mentioned this pull request Feb 16, 2024

is operator for pattern-matching and binding rust-lang/rfcs#3573

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade `equatable_if_let` to style #7777

Upgrade `equatable_if_let` to style #7777

HKalbasi commented Oct 6, 2021

rust-highfive commented Oct 6, 2021

flip1995 commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

flip1995 commented Oct 6, 2021

Manishearth commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

camsteffen commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

camsteffen commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

flip1995 commented Oct 7, 2021 •

edited

Loading

flip1995 commented Oct 7, 2021

HKalbasi commented Oct 7, 2021

camsteffen commented Oct 7, 2021

flip1995 commented Oct 7, 2021

camsteffen commented Oct 8, 2021

flip1995 commented Oct 8, 2021

HKalbasi commented Oct 12, 2021

bors commented Oct 13, 2021

bors commented Oct 29, 2021

flip1995 commented Nov 2, 2021

flip1995 Nov 2, 2021

HKalbasi Nov 3, 2021

flip1995 Nov 2, 2021

flip1995 Nov 2, 2021

HKalbasi Nov 3, 2021

flip1995 Nov 3, 2021

HKalbasi commented Nov 3, 2021

flip1995 commented Nov 3, 2021

bors commented Nov 5, 2021

camsteffen commented Nov 8, 2021

HKalbasi commented Nov 8, 2021

blyxyas commented Oct 6, 2023

Upgrade equatable_if_let to style #7777

Upgrade equatable_if_let to style #7777

Conversation

HKalbasi commented Oct 6, 2021

rust-highfive commented Oct 6, 2021

flip1995 commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

flip1995 commented Oct 6, 2021

Manishearth commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

camsteffen commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

camsteffen commented Oct 6, 2021

HKalbasi commented Oct 6, 2021

flip1995 commented Oct 7, 2021 • edited Loading

flip1995 commented Oct 7, 2021

HKalbasi commented Oct 7, 2021

camsteffen commented Oct 7, 2021

flip1995 commented Oct 7, 2021

camsteffen commented Oct 8, 2021

flip1995 commented Oct 8, 2021

HKalbasi commented Oct 12, 2021

bors commented Oct 13, 2021

bors commented Oct 29, 2021

flip1995 commented Nov 2, 2021

flip1995 Nov 2, 2021

Choose a reason for hiding this comment

HKalbasi Nov 3, 2021

Choose a reason for hiding this comment

flip1995 Nov 2, 2021

Choose a reason for hiding this comment

flip1995 Nov 2, 2021

Choose a reason for hiding this comment

HKalbasi Nov 3, 2021

Choose a reason for hiding this comment

flip1995 Nov 3, 2021

Choose a reason for hiding this comment

HKalbasi commented Nov 3, 2021

flip1995 commented Nov 3, 2021

bors commented Nov 5, 2021

camsteffen commented Nov 8, 2021

HKalbasi commented Nov 8, 2021

blyxyas commented Oct 6, 2023

Upgrade `equatable_if_let` to style #7777

Upgrade `equatable_if_let` to style #7777

flip1995 commented Oct 7, 2021 •

edited

Loading