28.0-rc2 erl_syntax:revert/1 doesn't roundtrip annotations for lists

[jcpetruzza]

Describe the bug
There is a regression in erl_syntax:revert/1: annotating the bindings on a syntax-tree and immediately reverting leads to the source position annotation for nil getting lost.

To Reproduce

-module(repro).

-export([
    run/0,
    foo/0
]).

foo() ->
  [a, b, c].

foo_syntax_tree() ->
    {ok, {_, [{abstract_code, AbsCode}]}} = beam_lib:chunks(code:which(?MODULE), [abstract_code]),
    {raw_abstract_v1, Forms} = AbsCode,
    [IsAny] = [Clause || {function, _, foo, 0, [Clause]} <- Forms],
    IsAny.

run() ->
    Original = foo_syntax_tree(),
    io:format("Original:~n~p~n", [Original]),
    Annotated = erl_syntax_lib:annotate_bindings(Original, ordsets:new()) ,
    Reverted = erl_syntax:revert(Annotated),
    io:format("Reverted:~n~p~n", [Reverted]),
    ok.

$ bin/erl
Erlang/OTP 28 [RELEASE CANDIDATE 2] [erts-15.2.3] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]

Eshell V15.2.3 (press Ctrl+G to abort, type help(). for help)
1> {ok, _} = c(repro, [debug_info]), l(repro), repro:run().
Original:
{clause,{8,1},
        [],[],
        [{cons,{9,3},
               {atom,{9,4},a},
               {cons,{9,7},
                     {atom,{9,7},b},
                     {cons,{9,10},{atom,{9,10},c},{nil,{9,11}}}}}]}
Reverted:
{clause,{8,1},
        [],[],
        [{cons,{9,3},
               {atom,{9,4},a},
               {cons,{9,7},
                     {atom,{9,7},b},
                     {cons,{9,10},{atom,{9,10},c},{nil,0}}}}]}
ok
2>

Expected behavior
In Reverted, we should see {nil, {9,11}}, instead we see {nil, 0}

Affected versions
28-rc2 (9834e60)

[michalmuskala]

Could this be duplicate of #4802, or is this specific behaviour new in 28?

[jcpetruzza]

This was working in OTP 27 (and on master back in December):

$ erl           
Erlang/OTP 27 [erts-15.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]

Eshell V15.2 (press Ctrl+G to abort, type help(). for help)
1> {ok, _} = c(repro, [debug_info]), l(repro), repro:run().
Original:
{clause,{8,1},
        [],[],
        [{cons,{9,3},
               {atom,{9,4},a},
               {cons,{9,7},
                     {atom,{9,7},b},
                     {cons,{9,10},{atom,{9,10},c},{nil,{9,11}}}}}]}
Reverted:
{clause,{8,1},
        [],[],
        [{cons,{9,4},
               {atom,{9,4},a},
               {cons,{9,7},
                     {atom,{9,7},b},
                     {cons,{9,10},{atom,{9,10},c},{nil,{9,10}}}}}]}
ok
2> 

[jcpetruzza]

Regression seems due to b4bc274, as reverting it makes the issue go away

[sverker]

@richcarl Can you look at this? Your commit b4bc274 seems to be the culprit.

[richcarl]

It seems to just be my fix working as intended. As the commit stated, "avoids setting a wrong location for the nil". In the erl_syntax representation, the nil is not preserved as a separate thing (which could perhaps be considered a bug or at least a misfeature, but that's how it works). When reverting, a nil has to be made up. Previously, this used the start of the last element before the nil - which is why you see {nil,{9,10}} in the OTP 27 reverted version, from the c atom - but that has other bad effects such as telling an editor that the list stops there, before the last element has ended. For example, if the last element is not just c, but a case expression split over multiple lines. So the solution for now is to set a 0, which should always be considered as "no information", not a line number.

I had some plans to move forward with better general end position support in erl_syntax, but there are some questions about which modules in OTP may and may not make assumptions about how position info with end points are encoded.

[richcarl]

Note that it didn't roundtrip in OTP 27 either, it replaced 11 with 10.

[jhogberg]

I agree with @richcarl, this is a bug in the (public and documented) erl_syntax representation. We don't have the bandwidth to work on this right now however, so help is appreciated.