Skip to content

Remove no_parens when using capture with arity #14090

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Remove no_parens when using capture with arity #14090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10f2ef2
Remove no_parens when using capture with arity
sabiwara Dec 20, 2024
129b588
More concise implementation
sabiwara Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/elixir/src/elixir_fn.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ capture_expr(Meta, Expr, S, E, Escaped, Sequential) ->
case escape(Expr, E, Escaped) of
{_, []} when not Sequential ->
invalid_capture(Meta, Expr, E);
{{{'.', _, [_, _]} = Dot, _, Args}, []} ->
Meta2 = lists:keydelete(no_parens, 1, Meta),
Copy link
Contributor Author

@sabiwara sabiwara Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not strictly needed for the fix, but otherwise we're expanding to something like {:fn, [{:no_parens, true}], [{:->, [{:no_parens, true}], ... which feels weird and make it harder to test.

Copy link
Member

@josevalim josevalim Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally we want to compile this to:

Function.capture(expr, name, arity)

So something we get something like this back:

iex> &(if true, do: NaiveDateTime, else: DateTime).utc_now/0
&NaiveDateTime.utc_now/0

Note we already do this for variables:

iex> var = NaiveDateTime
iex> &var.utc_now/0
&NaiveDateTime.utc_now/0

So we probably need to generalize this branch of the code a bit more (i.e. it should apply whenever the left side of the . is an expression)?

Copy link
Member

@josevalim josevalim Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, ignore me, there is clearly a semantic difference here. Your version executes foo() in &foo().bar/0 when the function is invoked. Mine would execute it always.

I think this is actually a bug. We should not have accepted this format but we clearly do. My suggestion is to deprecate this feature in main.

Copy link
Member

@josevalim josevalim Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear beyond doubt:

&expr.fun/arity

Only works if:

  • expr is a variable or an atom at compile-time
  • fun is an atom at compile-time
  • arity is an integer at compile-time

If those conditions are not met and it is valid, we should deprecate it.

Copy link
Contributor Author

@sabiwara sabiwara Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I hesitated to propose something in this vein but was unsure if there would be other legit cases.
As you said assigning to a variable or using fn seems better either way.

Fn = {fn, Meta2, [{'->', Meta2, [[], {Dot, Meta2, Args}]}]},
{expand, Fn, S, E};
{EExpr, EDict} ->
EVars = validate(Meta, EDict, 1, E),
Fn = {fn, Meta, [{'->', Meta, [EVars, EExpr]}]},
Expand Down
5 changes: 5 additions & 0 deletions lib/elixir/test/elixir/kernel/expansion_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,6 +1203,11 @@ defmodule Kernel.ExpansionTest do
[{:->, [{:line, 1}], [[{:capture, [line: 1], nil}], {:capture, [line: 1], nil}]}]}
end

test "removes no_parens when expanding 0-arity capture to fn" do
assert expand(quote(do: &foo().bar/0)) ==
quote(do: fn -> foo().bar() end)
end

test "expands remotes" do
assert expand(quote(do: &List.flatten/2)) ==
quote(do: &:"Elixir.List".flatten/2)
Expand Down
6 changes: 6 additions & 0 deletions lib/elixir/test/elixir/kernel/fn_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ defmodule Kernel.FnTest do
assert (&mod.flatten/1) == (&List.flatten/1)
end

test "capture with module from local call" do
assert (&math_mod().pi/0).() == :math.pi()
end

defp math_mod, do: :math

test "local partial application" do
assert (&atb(&1, :utf8)).(:a) == "a"
assert (&atb(List.to_atom(&1), :utf8)).(~c"a") == "a"
Expand Down
Loading