erl_syntax roundtrip modifies some annotations

[michalmuskala]

Describe the bug The transformation erl_syntax_lib:map(fun erl_syntax:revert/1, Form) is not a no-op. In particular it seems to change the annotations of cons and nil nodes.

This was discovered by analysing the behaviour of the cth_readable_transform parse transform.

To Reproduce

1> Form = {cons, 1, {integer, 1, 1}, {cons, 2, {integer, 1, 1}, {nil, 3}}}.
{cons,1,{integer,1,1},{cons,2,{integer,1,1},{nil,3}}}
2> erl_syntax_lib:map(fun erl_syntax:revert/1, Form).
{cons,1,{integer,1,1},{cons,1,{integer,1,1},{nil,1}}}

Expected behavior The following should always hold: Form =:= erl_syntax:map(fun erl_syntax:revert/1, Form)

Affected versions Tested on OTP 24 rc 3, but most likely affects earlier versions as well.

[gomoripeti]

very interesting, looks like

• the erl_parse representation is the same for the below two (where the second one is a syntactic sugar of the first)

  1> Form = merl:quote("[1 | [2 | []]]").
  {cons,1,{integer,1,1},{cons,1,{integer,1,2},{nil,1}}}
  2> Form = merl:quote("[1, 2]").
  {cons,1,{integer,1,1},{cons,1,{integer,1,2},{nil,1}}}

• erl_syntax_lib:map converts everything to a tree and in case of lists it seems to prefer the compact/"syntactic sugar" format

  3> erl_syntax:list(erl_syntax:list_prefix(Form), erl_syntax:list_suffix(Form)).
  {tree,list,
    {attr,0,[],none},
    {list,[{integer,1,1},{integer,1,2}],none}}

  hence the annotation of cons and nil is lost.

I think this commit https://github.com/erlang/otp/commit/53746070b5de7d1e9f809d516648948b0a246561 added this compacting behaviour, but it looks like the annotation of the nil tail was lost even before.

[MarkoMin]

I just peeked into this issue and it seems like it is unsolvable in a backward compatible way.

The problem is that list_suffix/1 does this:

list_suffix(Node) ->
    case unwrap(Node) of
    {cons, _, _, Tail} ->
        case cons_suffix(Tail) of
        {nil, _} ->
            none;
        Tail1 ->
            Tail1
        end;
    Node1 ->
        (data(Node1))#list.suffix
    end.

%% skips sequences of conses; cf. cons_prefix/1 above
cons_suffix({cons, _, _, Tail}) ->
    cons_suffix(Tail);
cons_suffix(Tail) ->
    Tail.

It just ignores nil's position and returns none... The problem is that list_suffix/1 is exported. nil element will get the position of the last node in the list:

revert_list(Node) ->
    Pos = get_pos(Node),
    Prefix = list_prefix(Node),
    Suffix = case list_suffix(Node) of
        none ->
            LastPos = get_pos(lists:last(Prefix)),
            LastLocation = case erl_anno:end_location(LastPos) of
                undefined -> erl_anno:location(LastPos);
                Location -> Location
            end,
            revert_nil(set_pos(nil(), erl_anno:set_location(LastLocation, Pos)));
...

FWIW: the problem occurs only with when nil is the tail of the list; any other element as a tail won't lose it's position.

To solve this, suffix of the list should return {nil, Pos} instead of none and list record shouldn't allow none as a valid value of suffix field. Is it worth it breaking the API?