opened
kelvinst
username_0
Closes #6374
Warning on the use of length(x) == 0 on guards
issue
created
josevalim
username_1
Thank you, I have added some comments!
comment
created
entertainyou
username_2
So for guard like "length(x) == 1" still has to be traverse the whole list?
comment
created
kelvinst
username_0
@username_2 I've also thought about it. It is also not a good practice, but I guess the point is that `length(x) == 0` is something people are very used to do in other languages, which aren't a bad practice, since `[]` are not lists and yes arrays. @username_1 and @username_4, is that correct?
comment
created
michalmuskala
username_3
I started wondering if the place for this is really the compiler and not, for example, a linter.
comment
created
whatyouhide
username_4
@username_2 `length(x) == 0` is a common pattern that says "if the list is empty". Lists with one element can still be checked by matching on `[_]` more efficiently, but then when do we stop? Do we say that `length(x) == 2` can be optimized by doing `[_, _]`? I think `length(x) == 0` makes sense because it's simple, common, and you can replace it with `x == []` instead of pattern matching.
comment
created
kelvinst
username_0
@username_3 me too... 😕
comment
created
kelvinst
username_0
Is there any linter on `elixir` core? Or you are talking about `credo`?
comment
created
whatyouhide
username_4
There is no linter on Elixir core except, well, the compiler warnings :)
comment
created
michalmuskala
username_3
There's also a slight change in semantics that has to be considered. `length(x) == 0` will fail on improper lists, which `x == []` will just return false. For example:

```elixir
def foo(x) when length(x) == 0 or hd(x) == nil, do: :nothing
def foo(_), do: :something

def bar(x) when x == [] or hd(x) == nil, do: :nothing
def bar(x), do: :something
```
Gives different results in some situations:
```elixir
foo([nil | nil]) #=> :something
bar([nil | nil]) #=> :nothing
```
comment
created
josevalim
username_1
It is definitely debatable although it is not the only place where it happens in Elixir.

Elixir warns if you are piping from or to an expression that does not have parens. We warn if you use a variable with underscore more than once in the same pattern. A compiler would not warn on any of those as it is valid code. Some of those may have false positives but I would say they still have improved the experience of developers with the language, removing common pitfalls and time sinks.

It is also worth mentioning that compilers do typically include linters but they typically worry with lower level details than developing-facing ones.

This is a small warning that can tell a lot on how to think and work with lists in Elixir. As with the previous warnings, there is a chance of legit usage of those functionalities, but all of those cases can be rewritten in a way that is clearer in purpose.
comment
created
kelvinst
username_0
@username_3, nice catch. Do you think someone could be using `length(x)` for this reason? Expecting this kind of behavior?

And even tough, I guess it continues to be a bad practice anyways, if they use it expecting this kind of behavior, they should change it to something like:

```elixir
def bar(x) when x == [] or hd(x) == nil or tl(x) == nil, do: :nothing
def bar(x), do: :something
```
comment
created
michalmuskala
username_3
I wouldn't expect to see that kind of code, but the behaviour is there currently. I guess a warning is fine, but that's one of the reasons why the `length` check can't be optimised by the compiler.
comment
created
kelvinst
username_0
Yep... At least not replacing them by `list == []`.

There is actually ways to optimize, creating functions like `length_equals?` that traverse the items, but just while it needs to, and imitating the behaviour of length for malformed lists. But I guess that's not the intention for now, and I don't know if we can consider it a clean solution.
comment
created
michalmuskala
username_3
You need to traverse the whole list to be compatible with `length`, so there wouldn't be much of a difference.
comment
created
kelvinst
username_0
@username_3 you're right, sorry. :sad: Didn't noticed that the malformed list in your example was a `nil` as the tail, and to check it one should traverse the whole list anyway. Please ignore what I said.
comment
created
josevalim
username_1
@username_0 I have pushed two commits to your branch, one that slightly improves the error message and indentation and another that fixes the test I broke. :) As soon as tests pass we will merge it.
comment
created
kelvinst
username_0
Thanks @username_1! Looks great!
comment
created
josevalim
username_1
:heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:
comment