Rust-clippy: len_zero for > 0 is not objectively correct

[mahkoh]

Clippy suggests

212 |                 if node.children.len() > 0 {
    |                    ^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!node.children.is_empty()`

but

if !node.children.is_empty() {

is harder to read and comprehend than the original:

1. It includes a double negation
2. The ! is far away from the verb is_empty. It's easily overlooked.
3. ! is in the wrong position grammatically.

I would suggest splitting len_zero in two parts and disabling it by default for > 0.

Furthermore, maybe add a lint against prefix ! in combination with long expressions.

[giraffate]

@rustbot modify labels: +A-suggestion

[ebroto]

Thanks for taking the time to report this!

I'm not sure about this one. I agree with the lint documentation and think the lint is relevant also for the > 0 cases. I will add the needs-discussion tag so other people can give their opinion, but I would prefer that the lint stays like it is today.

---

It includes a double negation

I don't think the quality of being empty qualifies as a negation.

The ! is far away from the verb is_empty. It's easily overlooked.

IMO this is a good candidate for a restriction lint for codebases that find this problematic. The code is correct, but some people may want to avoid this construct.

! is in the wrong position grammatically.

I guess this applies to any method called is_*. The standard library has many methods using this nomenclature.

[mahkoh]

I don't think the quality of being empty qualifies as a negation.

You're right. I think what I meant to say is that x.len() > 0 is a positive expression whereas !x.is_empty() is a negative expression. I think positive expressions are significantly easier to understand than negative ones.

I guess this applies to any method called is_*. The standard library has many methods using this nomenclature.

Very true unfortunately. I've taken to writing ptr != ptr::null() instead of !ptr.is_null() because the ! is too flimsy for my taste and there is no ptr.is_not_null(). I would even write chained().method().calls() == false instead of !chained().method().calls() for the same reason. (Once again another postfix operator [postfix !] would be the solution.)

Note that I think if !have and such is fine because the expression being negated is short.

[flip1995]

This came up before and the result of the discussion was: !...is_empty() is a pretty common pattern and therefore it is ok to suggest this. However, some people don't agree with this style, and we suggest to allow this lint in those situations.

I can't find the issue about this though...

[mahkoh]

I guess this applies to any method called is_*. The standard library has many methods using this nomenclature.

I've hit it with the big hammer: https://crates.io/crates/isnt

With this, the immediate issue is solved. If someone still wants to address the "! in front of a long expression" question, please open a new issue.