Upgrade/deprecate remote nil calls #9769
Conversation
lib/elixir/src/elixir_dispatch.erl
Outdated
| @@ -365,6 +365,12 @@ check_deprecated(Meta, 'Elixir.System', stacktrace, 0, #{contextual_vars := Vars | |||
| "move System.stacktrace/0 inside a rescue/catch", | |||
| elixir_errors:erl_warn(?line(Meta), ?key(E, file), Message) | |||
| end; | |||
|
|
|||
| check_deprecated([{no_parens,true}, _] = Meta, _, _, 0, E) -> | |||
There was a problem hiding this comment.
We want to do this warning here instead https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_expand.erl#L404.
lib/elixir/src/elixir_parser.yrl
Outdated
| @@ -713,6 +713,16 @@ build_op({_Kind, Location, 'not in'}, Left, Right) -> | |||
| build_op({_Kind, Location, Op}, Left, Right) -> | |||
| {Op, newlines_op(Location) ++ meta_from_location(Location), [Left, Right]}. | |||
|
|
|||
| build_unary_op({capture_op, Location, Op}, {_,_,[{_,[{no_parens,true} | _],_}, 0]} = Expr) -> | |||
There was a problem hiding this comment.
We cannot rely on {no_parens, true} being the first element in the metadata. Instead you should use lists:keyfind/3 to look through all elements.
There was a problem hiding this comment.
Since the capture can be built with macros and not only a literal I think we should check for parenthesis in the expansion pass like you do for the Foo.bar syntax.
lib/elixir/src/elixir_dispatch.erl
Outdated
|
|
||
| check_deprecated([{no_parens,true}, _] = Meta, _, _, 0, E) -> | ||
| Message = | ||
| "Calling remote function with no arguments without parentheses is deprecated", |
There was a problem hiding this comment.
Error messages start in lowercase. :)
|
@josevalim @ericmj thank you for you comments! I've changed stuff according to your suggestions and added tests. |
| Code.eval_string(""" | ||
| defmodule TestMod do | ||
| def foo(), do: nil | ||
| end |
There was a problem hiding this comment.
There is no need to define a new module. Instead, let's use a zero arity function in Elixir's codebase, such as System.pid.
| purge(TestMod) | ||
| end | ||
|
|
||
| test "deprecate remote nullary zero-arity calls without parens in a macro" do |
There was a problem hiding this comment.
We can remove this test. :) It is not actually exercising a different branch in the codebase.
| Code.eval_string(""" | ||
| defmodule TestMod do | ||
| def foo(), do: nil | ||
| end |
There was a problem hiding this comment.
Same as above, no need to define a new module. :)
| purge(TestMod) | ||
| end | ||
|
|
||
| test "deprecate nullary remote zero-arity capture with parens in a macro" do |
There was a problem hiding this comment.
Same as above, we can remove this test. :)
|
Thanks @esse ! I have dropped some comments to improve error messages and tests! |
Co-Authored-By: José Valim <jose.valim@gmail.com>
|
Thanks @josevalim - I applied all of your suggestions :) |
|
💚 💙 💜 💛 ❤️ |
In relation to the #9510 and since we're on Elixir 1.11-dev, I've done here two points from this issue:
Since it's my first PR touching erlang code, I would be awesome if someone could tell if this is the right approach.
@josevalim let me know if that approach is correct, and if so - I will tomorrow write tests for that :)