New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when variable is being expanded to function call #3268

Closed
josevalim opened this Issue Apr 14, 2015 · 27 comments

Comments

Projects
None yet
@josevalim
Member

josevalim commented Apr 14, 2015

Today, Elixir will transform var into var() in case var does not exist in the user context. I would like to propose those cases to emit a warning from today on.

The downsides is that we add a tiny inconsistency. We can make function call without parens unless it is a function with null-arity. On the other hand, we already have inconsistency in the sense we can only shadow function calls with null-arity by variables. So we are exchanging one by the other.

I have bitten by this issue twice today and I believe a warning to be a far compromise.

@lexmag

This comment has been minimized.

Member

lexmag commented Apr 16, 2015

Strongly 👍.

@ghost

This comment has been minimized.

ghost commented Apr 21, 2015

👍

@alco

This comment has been minimized.

Member

alco commented Apr 22, 2015

+1 from me.

@romul

This comment has been minimized.

Contributor

romul commented Jul 26, 2015

I agree with @lpil and dislike removing opportunity to call zero-arity functions without parentheses.

@josevalim josevalim removed this from the v1.1.0 milestone Aug 14, 2015

@jisaacstone

This comment has been minimized.

Contributor

jisaacstone commented Aug 14, 2015

+1 from me as well

@meyercm

This comment has been minimized.

meyercm commented Aug 19, 2015

I also agree with @lpil and @romul. Not only would my codebase require dozens if not hundreds of edits to suppress the warnings, but the superfluous paren would make me sad.

@eksperimental

This comment has been minimized.

Member

eksperimental commented Aug 20, 2015

It will push users to improve the quality of the code, as it will be less ambiguous, but I think it will affect the aesthetics of it.

@lexmag

This comment has been minimized.

Member

lexmag commented Oct 19, 2015

For a historical reference, how it (variable is used as function call) became a problem one more time:
https://botbot.me/freenode/elixir-lang/2015-10-19/?msg=52227800&page=5
\cc @eddwardo

@eddwardo

This comment has been minimized.

eddwardo commented Oct 19, 2015

Small example:

defmodule Foo do
  def bar do
    IO.puts "started bar" # IT WILL HANG HERE
    receive do
      :msg -> IO.puts "received msg"
    end
  end

  def start do
    ping_pid = spawn(__MODULE__, bar, []) # forgot to pass :bar
    ping_pid
  end
end
@pdilyard

This comment has been minimized.

pdilyard commented Nov 24, 2015

I've got a little collection of thoughts on this subject:

Would this mean you'd have to use parens in pipelines as well? I really like the way pipelines work without parens on 0-arity functions, for example:

my_var
|> do_something
|> do_something_else
|> do_a_third_thing(1, 2)

To suppress this warning, it looks like you'd have to rewrite the above as:

my_var
|> do_something()
|> do_something_else()
|> do_a_third_thing(1, 2)

In this case, the first example makes it obvious that values are being piped to functions (you can't pipe to anything else), and looks a little sweeter in my opinion.

Another thing to think about is the way a human can read Elixir code.

def n do
  10
end

def random_number do
  :random.uniform(10)
end

Obviously this is a contrived example, but one can think through this code as "I am defining n as 10", and "I am defining random_number as a random number between 1 and 10". In these cases it makes sense to be able to retrieve those defined values simply with n and random_number. They feel like values.

However, in the case of something like ExUnit.start/0, when you don't really care about the return value, only the side effects, it makes more sense to call it with parens (ExUnit.start()). That reads more as something like this "I just performed the action of starting ExUnit".

Lastly, if all functions with 0-arity should be called with parens, wouldn't it make more sense for the preferred style of defining a function to be like this:

def start() do
  :ok
end

start()

This would be more consistent than having a definition with one style and a call with another.

Anyway, my main point is that semantically, different styles make more sense in different scenarios. I'm not really sure what the answer is, as I definitely understand what @josevalim is saying (it's thrown me off before too), but those are my two cents 😄

@josevalim

This comment has been minimized.

Member

josevalim commented Nov 24, 2015

Would this mean you'd have to use parens in pipelines as well?

No, because are macros and the module compiler works on the AST after the macro is expanded. None of the code samples you have shown would need to change.

@josevalim josevalim closed this Nov 24, 2015

@josevalim josevalim reopened this Nov 24, 2015

@josevalim

This comment has been minimized.

Member

josevalim commented Nov 24, 2015

Sorry, closed by mistake.

@pdilyard

This comment has been minimized.

pdilyard commented Nov 24, 2015

Interesting, thanks for the explanation.

@alco

This comment has been minimized.

Member

alco commented Nov 25, 2015

Still +1 to the original issue. I have adopted the following style for function calls long ago:

  • never use parentheses for remote calls with 0 arity. Examples:

    ExUnit.start
    
    list
    |> Stream.map(& &1 * &1)
    |> Enum.reverse
    
  • use parentheses in all other cases. This includes remote functions with arity 1 or more and local/imported functions regardless of the arity.

    this_returns_a_value()
    |> Enum.filter(...)
    |> some_local_function()
    |> other_local_function(arg)
    |> IO.iodata_to_binary
    

I suggest everyone else do the same. Even if this issue doesn't make it into Elixir 1.x, it will surely be part of Elixir 2.0. So you have a chance to save yourself some work in the future by adopting a good habit today :)

@pdilyard

This comment has been minimized.

pdilyard commented Nov 25, 2015

👍 for @alco is saying. That goes along the lines of what I was trying to get across in the second half of my long comment above.

@lpil

This comment has been minimized.

Contributor

lpil commented Nov 25, 2015

Does this also apply to macros?

test("""
Long test desc here
""') do
  # ...
end

test """
Long test desc here
""" do
  # ...
end
@pdilyard

This comment has been minimized.

pdilyard commented Nov 25, 2015

@lpil I don't think so, because the proposed warning would only be given for 0-arity functions.

@lpil

This comment has been minimized.

Contributor

lpil commented Nov 25, 2015

Oh, right, of course. :)

@lexmag lexmag self-assigned this Nov 28, 2015

@ghost

This comment has been minimized.

ghost commented Dec 18, 2015

+1 for a warning

@Tuxified

This comment has been minimized.

Contributor

Tuxified commented Jun 9, 2016

+1 for warning, fyi: Credo already has a warning for this

@lexmag lexmag closed this in #3517 Jun 11, 2016

@lexmag lexmag changed the title from Warn if variable is used as function call to Warn when variable is being expanded to function call Jun 16, 2016

@paulcsmith

This comment has been minimized.

Contributor

paulcsmith commented Jun 22, 2016

I see how this could help, but I also prefer keeping parens off zero arty calls. I wonder if instead of warning when parens are missing, the warning would come up when there is a local variable name and a function with zero arity in the same scope.

Example:

  conn = conn

  conn = get(conn, ...etc.)

In this case the call to conn() is ambiguous and a bit confusing. I don't think the solution is to add parens though, the solution would be to rename the function to build_conn or something else. So I think that adding parens just masks the problem without really fixing the confusion.

That may not prevent all the bugs, but might at least prevent some of the more confusing situations?

@whatyouhide

This comment has been minimized.

Member

whatyouhide commented Jun 22, 2016

I am strongly in favour of this warning because it makes Elixir code so much easier to read. Imagine you have this code:

def foo() do
  # 50 lines of code
  my_function(something)
  # ...
end

Is something a variable or a private/imported 0-arity function? You can't know and you'll have to look through the 50 lines above the call to my_function/1 to find out. And bear in mind, it's not just a matter of finding something = ..., because you may have deeply nested pattern matches for example where something gets bound. With the current warning, we'll have to write my_function(something()), which will destroy every ambiguity.

That said, @paulcsmith Elixir's issue tracker is reserved for bugs so maybe we can move this discussion to the Elixir mailing list or to elixirforum? 😃

@paulcsmith

This comment has been minimized.

Contributor

paulcsmith commented Jun 22, 2016

@whatyouhide Sure! I thought I saw a discussion label on this issue so I posted here. It seems most people are strongly in favor of adding the parens though so it probably isn't worth. Just wanted to voice an opinion in case it resonated with anyone :)

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 22, 2016

In this case the call to conn() is ambiguous and a bit confusing. I don't
think the solution is to add parens though, the solution would be to rename
the function to build_conn or something else. So I think that adding parens
just masks the problem without really fixing the confusion.

The call to conn() is never ambiguous. Only "conn". :)

@paulcsmith

This comment has been minimized.

Contributor

paulcsmith commented Jun 22, 2016

@josevalim Yeah that's a good point! Thanks for responding

@romul

This comment has been minimized.

Contributor

romul commented Jun 22, 2016

Imagine you have this code:

def foo() do

50 lines of code

my_function(something)

...

end

I'd prefer to have a warning if there is a function contains more than 50 lines )))

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 22, 2016

@romul if you have a comment related to this feature in particular, please add so, otherwise if you want to discuss other improvements and warnings, you should try the elixir-lang-core mailing list. Let's stay on topic please.

ajvondrak added a commit to ajvondrak/remote_ip that referenced this issue Feb 1, 2017

Bump patch version to silence Elixir 1.4 warnings
While `mix test` seems to work just fine using Elixir 1.4, it spits out
a bunch of warnings for zero-arity function calls (cf.
elixir-lang/elixir#3268).

This was fixed by #2. Those changes are still compatible with Elixir
1.3, so no need for a major version bump. But Elixir 1.4 users probably
don't appreciate all these warnings.

tielur added a commit to tielur/alice that referenced this issue Feb 4, 2017

rasoliveira added a commit to cultureamp/microstatus that referenced this issue Apr 6, 2017

@tonydaly tonydaly referenced this issue Apr 6, 2017

Closed

Fixing warnings #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment