Skip to content
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

Code.Fragment.surround_context miss-categorises atom keys and &1 capture variables #13818

Closed
lukaszsamson opened this issue Sep 9, 2024 · 4 comments · Fixed by #13847
Closed

Comments

@lukaszsamson
Copy link
Contributor

Elixir and Erlang/OTP versions

Erlang/OTP 26 [erts-14.2.5.2] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]

Elixir 1.17.2 (compiled with Erlang/OTP 26)

Operating system

any

Current behavior

  1. It returns local_or_var for atom keys in list, map, etc
iex(5)> Code.Fragment.surround_context("%{foo: 1}", {1, 3})
%{context: {:local_or_var, ~c"foo"}, begin: {1, 3}, end: {1, 6}}

for ordinary atoms it returns unquoted_atom

iex(8)> Code.Fragment.surround_context(":foo", {1, 1})
%{context: {:unquoted_atom, ~c"foo"}, begin: {1, 1}, end: {1, 5}}
  1. It is finding only & operator but in this context & is not an operator but a part of a special variable &123
iex(1)> Code.Fragment.surround_context("&123", {1, 1})
%{context: {:operator, ~c"&"}, begin: {1, 1}, end: {1, 2}}
iex(2)> Code.Fragment.surround_context("&123", {1, 2})
:none

Expected behavior

  1. I would expect it to return unquoted_atom or more specific unquoted_atom_key
  2. Here more appropriate would be local_or_var or something dedicated
@josevalim
Copy link
Member

For 1, I agree, we should probably return keyword key or similar.

2 is correct. & is a unary operator, as seen in its AST: {:&, [], [123]}.

@lukaszsamson
Copy link
Contributor Author

2 is correct. & is a unary operator, as seen in its AST: {:&, [], [123]}.

The docs call them value placehonders and limit & operator to capture only. Looking from the code level point of view it's a symbol &123 and it gets expanded to a variable. Technically, it may by an operator on AST level, but it behaves like one only wrapped in parens

iex(25)> a = & &(1) + 1
error: nested captures are not allowed. You cannot define a function using the capture operator & inside another function defined via &. Got invalid nested capture: &(1 + 1)
└─ iex:25

** (CompileError) cannot compile code (errors have been logged)

iex(25)> a = & 1 + & 1 + 1
error: nested captures are not allowed. You cannot define a function using the capture operator & inside another function defined via &. Got invalid nested capture: &(1 + 1)
└─ iex:25

** (CompileError) cannot compile code (errors have been logged)

iex(17)> a = & (& 1) + 1
#Function<42.105768164/1 in :erl_eval.expr/6>

iex(19)> a = & (&(1)) + 1
#Function<42.105768164/1 in :erl_eval.expr/6>

Contrary to that when & is used as operator it behaves as other operators

iex(13)> & inspect/1
&Kernel.inspect/1
iex(14)> &(inspect/1)
&Kernel.inspect/1
iex(15)> &(inspect)/1
&Kernel.inspect/1

@josevalim
Copy link
Member

josevalim commented Sep 9, 2024

They are operators with different precedence. You can see if if you add quotes:

iex(5)> quote do: & &(1) + 1
{:&, [],
 [
   {:&, [],
    [{:+, [context: Elixir, imports: [{1, Kernel}, {2, Kernel}]], [1, 1]}]}
 ]}

The reason why it is invalid, it is because it is parsed as & &(1 + 1) (which is important for & without parens to work). So for instance, if you flip the argument order, it works:

iex(7)> & 1 + &(1)
#Function<42.105768164/1 in :erl_eval.expr/6>
iex(8)> v().(2)
3

There could be an argument here that, because &123 is treated with higher precedence by the parser, we could reflect this in surround context, but it does not change the fact it is an operator, and it does not change the fact &(1) is also valid.

@josevalim
Copy link
Member

because &123 is treated with higher precedence by the parser, we could reflect this in surround context

Let's have &123 as its own thing, since it could be useful in an editor.

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

Successfully merging a pull request may close this issue.

2 participants