credo icon indicating copy to clipboard operation
credo copied to clipboard

Is not aware of imports

Open turion opened this issue 6 years ago • 2 comments

Environment

  • Credo version (mix credo -v): 1.0.5
  • Erlang/Elixir version (elixir -v): Elixir 1.9.1 (compiled with Erlang/OTP 21)
  • Operating system: NixOS unstable

What were you trying to do?

Consider this module, where we've defined our own local length function.

defmodule Foo do
  @moduledoc false

  import Kernel, except: [length: 1]

  def length("foo") do
    23
  end

  def length(_) do
    0
  end

  def foo(x) do
    if length(x) == 0 do
      IO.puts("yes")
    end
  end
end

Expected outcome

No error

Actual outcome

  Warnings - please take a look                                                 
┃ 
┃ [W] ↗ length(x) == 0 is expensive. Prefer Enum.empty?/1 or list == []
┃       lib/foo.ex:15:8 #(Foo.foo)

turion avatar Nov 21 '19 12:11 turion

You've got a point there. Thx for reporting! 👍

We'll have to see how to handle this.

rrrene avatar Nov 23 '19 20:11 rrrene

I have been thinking about this one for a while. At first I was thinking how we could disable the warning automatically when an import overrides length, however then I realized that even if the length function is overridden (dangerous thing to do btw, I would highly discourage it), the warning in many cases may still apply, i.e. there is probably still a better way to check if the thing is empty than calculating the full length of it first. It is true that there isn't in the example you gave, but real world examples are most likely going to have a more efficient empty? function you can implement and use.

My suggestion for this issue is that it be closed as wontfix. In the case that you are overriding length and you definitely don't have a better is_empty? function you can use, you should add # credo:disable-for-this-file Credo.Check.Warning.ExpensiveEmptyEnumCheck, but IMO we shouldn't be assuming this is the case and doing it automatically.

TheFirstAvenger avatar Jan 25 '20 22:01 TheFirstAvenger

I apologize for the age/inactivity on this issue. I should have done a better job at resolving this properly. 😥

Please feel free to re-open this issue at your discretion.

rrrene avatar Feb 03 '23 21:02 rrrene