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

Emit warning when variable is being expanded to function call #3517

Merged
merged 1 commit into from Jun 11, 2016

Conversation

Projects
None yet
6 participants
@lexmag
Member

lexmag commented Jul 21, 2015

Closes #3268.

@josevalim

This comment has been minimized.

Member

josevalim commented Jul 21, 2015

I think the goal is to warn only if inside a function, i.e. E#elixir_env.function is not nil.

@@ -313,6 +313,8 @@ expand({Name, Meta, Kind} = Var, #{vars := Vars} = E) when is_atom(Name), is_ato
compile_error(Meta, ?m(E, file), "expected var \"~ts\"~ts to expand to an existing variable "
"or be part of a match", [Name, elixir_scope:context_info(Kind)]);
true ->
Message = io_lib:format("variable ~ts is used as function call", [Name]),

This comment has been minimized.

@josevalim

josevalim Jul 21, 2015

Member

We need to add to the error message: ", please use parentheses to remove the ambiguity and solve this warning"

@lexmag lexmag force-pushed the lexmag:var-fun-warn branch 2 times, most recently from 52697ad to 1ef9c99 Jul 21, 2015

@josevalim

This comment has been minimized.

Member

josevalim commented Aug 14, 2015

@lexmag just a heads up: let's move this to 1.3. There are already quite some changes in 1.1 and 1.2 is meant to be completely compatible. So 1.3 is when we can start pushing this.

@lexmag lexmag force-pushed the lexmag:var-fun-warn branch from 1ef9c99 to e4f6272 Sep 7, 2015

@lexmag lexmag added this to the v1.3.0 milestone Dec 29, 2015

@lexmag lexmag modified the milestones: v1.4.0, v1.3.0 Mar 17, 2016

@lexmag lexmag force-pushed the lexmag:var-fun-warn branch 6 times, most recently from 5b2317f to f201ee8 Jun 8, 2016

@lexmag lexmag changed the title from [WIP] Warn if variable is used as function call to Emit warning when variable is being expanded to function call Jun 11, 2016

@lexmag

This comment has been minimized.

Member

lexmag commented Jun 11, 2016

I'm still wondering if it'll be considered inconsistent if we emit this warning only inside functions.

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 11, 2016

What do you mean by only inside functions? We should always emit it,
regardless of __ENV__.function is nil or not, no?

@lexmag

This comment has been minimized.

Member

lexmag commented Jun 11, 2016

Yes, but previously (Jul 21, 2015) we thought "to warn only if inside a function".

The only downside of not checking if "function isn't nil" is the following:
screen shot 2016-06-11 at 18 32 44

@lexmag lexmag force-pushed the lexmag:var-fun-warn branch from f201ee8 to a43ab74 Jun 11, 2016

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 11, 2016

:shipit:

@lexmag lexmag merged commit 96da837 into elixir-lang:master Jun 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lexmag lexmag deleted the lexmag:var-fun-warn branch Jun 11, 2016

@danhper

This comment has been minimized.

Contributor

danhper commented Jun 22, 2016

When compiling a project using the master branch, I found out this warning, which I think is good in almost all cases.
However, I am a little concerned about DSLs.
For example, to define an Ecto schema, this forces to write:

  schema "my_schema" do
    field :my_field, :string

    timestamps()
  end

which I think does not look very DSL like.
So basically I think this change would remove the syntax-sugar for all booleans attributes in DSLs.

I did not look at the implementation details yet, but do you think it would be possible and worth to handle such a case?

@whatyouhide

This comment has been minimized.

Member

whatyouhide commented Jun 22, 2016

@tuvistavie timestamps in the DLS you showed presents the same problem that this warning is trying to solve: if you have timestamps = :foo a few lines up, it will override your timestamp function (macro) call. Personally I don't think it's worth to handle such a case (provided it is possible).

@danhper

This comment has been minimized.

Contributor

danhper commented Jun 22, 2016

timestamps in the DLS you showed presents the same problem

I understand you point, and the point of this warning,
but often a DSL is used as something declarative,
where overriding something by mistake is very unlikely to happen.
For example, let's say we have a DSL to write HTML.

html do
  body do
    h1 "foobar"
    br()
  end
end

Having to make an explicit function call here kind of looses the beauty of the DSL in my opinion.

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 22, 2016

@tuvistavie it depends on the DSL. Some DSLs will bypass this warning by simply annotating those variables as generated or by translating the AST. For example, foo |> bar |> baz won't warn because bar is never really called as is.

@danhper

This comment has been minimized.

Contributor

danhper commented Jun 22, 2016

@josevalim Thank you for the explanation.
So I suppose the best thing to do is to handle this at the DSL level
by changing these kind of calls into macros and making sure we don't trigger the warning, right?

@josevalim

This comment has been minimized.

Member

josevalim commented Jun 22, 2016

Yes, that's a possibility, definitely. We will continue collecting feedback on this in any case. :)

@ivanvotti

This comment has been minimized.

ivanvotti commented Aug 14, 2016

I use Elixir 1.3.2 and I don't see the warning when I try this code:

defmodule WarningTest do
  defp do_stuff, do: 'stuff'

  def my_func do
    do_stuff
    a = do_stuff
  end
end
@lexmag

This comment has been minimized.

Member

lexmag commented Aug 14, 2016

@ivanvotti this warning will be part of Elixir v1.4.0.

@hellerve

This comment has been minimized.

hellerve commented Feb 1, 2017

Late to the party, but I cannot seem to find this in the v1.4.0 changelog. Is there any reason this was omitted?

@josevalim

This comment has been minimized.

Member

josevalim commented Feb 1, 2017

[Kernel] Warn if variable is used as a function call

https://github.com/elixir-lang/elixir/blob/v1.4/CHANGELOG.md

@hellerve

This comment has been minimized.

hellerve commented Feb 1, 2017

Okay, searched for bare words and arity. Sorry for the noise, then.

@josevalim

This comment has been minimized.

Member

josevalim commented Feb 1, 2017

No problem!

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