The is_not_empty() method as more clearly alternative for !is_empty()

libs
1

I think it's a good idea to add such a method for strings and an array, maybe it will also be useful for other collections. What do you think?

1 Like
2

What's wrong with !is_empty()?

3 Likes
3

One concrete example of where !is_empty() can be a problem:

  • A developer writes a condition in the form if vec.len() > 0.
  • Clippy helpfully notes that 'using is_empty is clearer and more explicit'.
  • The developer doesn't pay enough attention to the example code that Clippy gives, or simply mistypes, and replaces their condition with if vec.is_empty().
  • Whoops, they've just flipped their conditional...

I saw this exact thing happen at work (albeit in a Java codebase with SonarQube linting), and it silently caused a massive bug, so it's not a hypothetical!

Now, whether this is enough to justify the addition of a new method, I'm not sure! You could argue that people should just be more careful :stuck_out_tongue: But I do notice that when I'm scanning code, I always have to do a double take at is_empty calls to see if they actually mean the opposite - the ! is easy to miss.

12 Likes
4

It seems like most of the time it's the opposite of the method I want, so !vec.is_empty() is more common than vec.is_empty().

So in a way, the real problem is with !

  • it's not very visible.
  • precedence/associativity requires extra care when reading !…very long expression….is_empty().

But I'm not sure if there's a sensible general solution for !

  • if not vec.is_empty() would be clearer in this case, but if not a && b seems ambiguous (is it like if let that applies to the whole statement?)

  • bool could add inherent .not() method. vec.is_empty().not() is readable, but reads funny.

8 Likes
5

One of the points is still very difficult to read a long call chain with the !

if !request.params.config.key.value.is_empty()

or

if request.params.config.key.value.is_not_empty()

the second variant is more helpful for us when we scanning the code.

8 Likes
6

What about as @kornel suggested above?

if request.params.config.key.value.is_empty().not()
1 Like
7

Just to share an anecdote: I sometimes implement a simple extension trait that provides .not() for bool, and have often wished it was in standard library. I find the non-visibility of ! jarring and think it goes against the "strive for correctness" culture of Rust. Edit: Oh, that's embarrassing :smiley:

2 Likes
8 
use std::ops::Not;
println!("{:?}", true.not());

:wink:

37 Likes
9

Are you saying this already exists or this is what it would look like if implemented?

Never mind. :dizzy_face:

10

This is "the solution" I personally use; as @Nemo157 showed, by using ops::Not we get the .not() method, which is indeed more readable than a small prefix sigil; the only issue is that many programmers are already used to !condition so it is not that likely that changing this be deemed necessary.

Again, yet another topic related to: Crates that use the "inherent trait method" pattern - The Rust Programming Language Forum

At some point @Centril mentioned that the Not trait could be added to the prelude, but I don't know how the situation may have evolved.

5 Likes
11

is_empty().not() works fine. I created a simple example to show the visual difference between these approaches (play.rust-lang.org) Could you tell which method is most preferable for you?

12

It didn't; someone would need to push for that through an RFC, PR, or whatnot.

1 Like
13

If find the is_empty().not() to be FAR, FAR superior in readability and recognizability to is_not_empty().

1 Like
14

I agree not.

31 Likes
15

(Instead of .not(), how about .nicht(), as we all know German negates things with a "sike" at the end. Or .sike() for that matter. /s)

11 Likes
16

If ever there were a case for micro-dependencies, this might be it -- there's no sike crate yet...

1 Like
17

Can I ask why .is_not_empty() instead of .not_empty()? The not can be considered the inverse of is and is less likely to be confusing when skimming the code.

7 Likes
18

Having two "obvious" ways of doing something is not a good idea. This is a similar problem to how, in C++, if (ptr) and if (ptr != nullptr) are frequently equivalent, and choice is a matter of style. If the aim is to improve readability, your gains will be small if there are two obvious ways to do something.

In general I'm fairly happy about Rust's judgement in what convenience functions to add; I typically compare with Scala, whose collections often have lots of redundant functions... .isEmpty and .nonEmpty among them!

5 Likes
19

The scenario mentioned by @17cupsofcoffee has happened to me several times, TBH (e.g. clippy telling me to use is_empty() over a len() check, and me accidentally flipping the condition).

It wouldn't surprise me for my code to have cases where insufficient test coverage has even let this cause bugs in my code :frowning:. That's not to say this is anybody's fault other than my own, but an replacement for s.len() != 0 that's 1-to-1 identical in meaning seems like it would pull its own weight.

Regarding multiple ways to do something, as mentioned by @mcy, Rust already has many of these in e.g. the Option API (obviously map_or is the same as map + unwrap_or, and, for that matter, unwrap_or is redundant with unwrap_or_else -- same for most other other non-_else methods). Which is to say, I don't find that argument compelling.

1 Like
20

I just did a quick test of clippy:

warning: length comparison to zero
 --> src/main.rs:4:8
  |
4 |     if v.len() > 0 {
  |        ^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!v.is_empty()`
  |
  = note: #[warn(clippy::len_zero)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero

That seems to pretty clearly suggest !v.is_empty().

But if this is a common source of errors, could we enhance the text of that clippy message to make it clearer, such as moving the negation earlier in the error message?

11 Likes