add library module for converting between erlfmt and syntax tools #237

richcarl · 2021-01-14T12:46:40Z

Allows roundtrip conversion from erlfmt:read_nodes() to syntax tools (or erl_parse) and back to erlfmt:format_nodes().

awalterschulze · 2021-01-18T10:10:21Z

This certainly looks useful for writing refactoring tools in erlang itself.

We are having a hard time deciding if this belongs in erlfmt or a separate library:
If we merge it in erlfmt, we would need to maintain it and if we don't merge it, then we would need to document our abstract_forms types better. To merge in erlfmt in would need a barrage of tests, that would make us confident that we can maintain it. Are you willing to do this work?

I only skimmed the code, but I saw a lot of TODOs are they intended to be fixed before merge or ...?

richcarl · 2021-01-18T11:09:14Z

No, the TODOs are meant for doing later, they're not blocking anything. But this code needs to find a home first, and I'm intending to do the work on it. Also hoping to work on reducing the internal differences between erlfmt and syntax tools so the conversion becomes less complicated in places.

awalterschulze · 2021-01-18T11:14:40Z

Do you also intend to add a barrage of tests, something in the order of https://github.com/WhatsApp/erlfmt/blob/master/test/erlfmt_format_SUITE.erl ?

Knowing this would help us to make a decision.

richcarl · 2021-01-18T11:32:12Z

That would probably be relatively easy to do, just adapting the existing tests and checking that the round-tripped code looks the same, so yes.

awalterschulze · 2021-01-18T14:29:00Z

That would be one way to get quite a large of coverage, but feel free to take some liberty on what you think is sensible.

Do you also maybe have thoughts on why you think this is the right home for this code?

richcarl · 2021-01-18T14:55:49Z

I think this is the right home, because the erlfmt format doesn't officially exist outside this app. By having the conversion functions here, nobody should need to know about it or depend on it. Also, it becomes easy to make synchronized changes in erlfmt internals and the conversion functions with a single commit.

awalterschulze · 2021-01-20T15:35:05Z

We have talked and you make a compelling argument :D
Lets consider this the future home of this code, but lets also get some tests before merging.

richcarl · 2021-01-26T13:14:14Z

I think this is ready now.

awalterschulze

Overall have to say it looks amazing. Really great work <3
The comments are mostly nitpicks and I am wondering what the strategy is for the TODOs.
Some of them seem related to syntaxtools and others to erlfmt.
Should we open issues to tackle them or what do you think?

awalterschulze · 2021-01-27T10:16:48Z

test/erlfmt_SUITE.erl

+    GroupProps = ?config(tc_group_properties, Config),
+    case proplists:get_bool(syntax_tools, GroupProps) of
+        true ->
+            put('$syntax_tools$', true);


Wow I didn't know about this magic.
I know this is a minimal diff, but thinking of the next person reading this, maybe it would be better to use a parameter, since each test already has access to Config. Something like:

is_syntax_tools(Config) -> proplists:get_bool(syntax_tools, ?config(tc_group_properties, Config)). some_test(Config) -> RoundTripWithSyntaxTools = is_syntax_tools(Config) ... parse_form(Form, RoundTripWithSyntaxTools)

But I think this was a great way to get test coverage and it is really impressive that everything is passing.

I like to pass things as parameters whenever I can, but all the test code would have to get the extra Options parameter passed all the way down to the parse_form/parse_forms functions, and that seemed too intrusive. I also thought about using meck to modify the behaviour of the test code, but that's also a bit yucky and causes a dep on meck. What do you prefer?

I think I prefer a parameter, since I don't think it needs to pass too far down.
But let's also hear what @michalmuskala has to say.

I think this is fine. It's not pretty, but it's practical. I actually wonder if we should try doing something in erlfmt_format_SUITE as well to leverage all the examples we have in there, though I'm not 100% sure how that would look like.

src/erlfmt_ast.erl

awalterschulze · 2021-01-27T10:25:23Z

src/erlfmt_ast.erl

+    end.
+
+st_to_erlfmt_1(Node) ->
+    %% TODO: should we convert full erl_syntax pos+annotation to erlfmt anno?


It seems like you are doing Anno conversions here, so maybe this TODO is already done or I don't understand it, sorry?

erl_syntax nodes have a separate annotation field (because the old annotation field used to be only for line numbers), and that could contain arbitrary "user data", so the question is whether to pick up any such annotations and inject them into the erlfmt annotation when going from st to erlfmt, or just abandon them (as done now).

Aha, well I can say that only the relative line numbers are taken into account by the formatting algorithm.
So then the question would be, what would be the use case to preserve them and is that worth the effort.

src/erlfmt_ast.erl

richcarl · 2021-01-27T12:43:19Z

I'm currently pondering what changes I can make in syntax tools that will preserve compatibility with existing code that's using it, but being better at representing the same things under the hood that erlfmt does. Some of those TODOs require that sort of change first. Some other things may be possible to just change on the erlfmt side (after consulting you) and for those I could open tickets if you like.

awalterschulze · 2021-01-27T13:36:38Z

Is it possible to label the TODOs in the code as syntax_tools TODOs and actual TODOs for this code, to try and make it clear what is do-able by someone only looking at this code? It would at least help me to discern between them.

michalmuskala · 2021-02-09T19:06:33Z

src/erlfmt_ast.erl

+-export([erlfmt_to_st/1, st_to_erlfmt/1]).
+
+% dialyzer hates erlfmt_parse:abstract_node()
+-type erlfmt() :: term().


We can probably do a bit better with tuple, but I'm not sure it matters that much

michalmuskala · 2021-02-09T19:16:55Z

src/erlfmt_ast.erl

+        %% `macro_call` nodes. Additionally it is less strict - it does not
+        %% enforce all clauses have the same name and arity.
+        {function, Pos, Clauses} ->
+            case get_function_name(Clauses) of


What happens if there's mixed elements? Something like:

function(1) -> ok; ?COMMON_HANDLER(function).

michalmuskala · 2021-02-09T19:16:55Z

src/erlfmt_ast.erl

+                        erl_syntax:named_fun_expr(
+                            erlfmt_to_st(Name),
+                            Clauses1
+                        ),


What happens if the clauses weren't consistently named? Erlfmt parses this fine:

fun ?FOO(1) -> ok; Foo(2) -> error; end

michalmuskala · 2021-02-09T19:17:51Z

src/erlfmt_ast.erl

+        %% * `{type, Anno, Args, Res}` for the anonymous function type
+        %%   `fun((...Args) -> Res)` where `Args` is a `args` node.


Are the fun types handled anywhere?

michalmuskala · 2021-02-09T19:24:25Z

src/erlfmt_ast.erl

+        {guard_or, Pos, Exprs} ->
+            AAnno = dummy_anno(),
+            erlfmt_to_st_1({tuple, Pos, [{atom, AAnno, '*guard_or*'} | Exprs]});
+        {guard_and, Pos, Exprs} ->
+            AAnno = dummy_anno(),
+            erlfmt_to_st_1({tuple, Pos, [{atom, AAnno, '*guard_and*'} | Exprs]});


Wouldn't those be represented as erl_syntax:conjuction/disjunction? http://erlang.org/doc/man/erl_syntax.html#conjunction-1

michalmuskala · 2021-02-09T19:41:06Z

src/erlfmt_ast.erl

+                    %% note that erlfmt only accepts raw_string as a form
+                    "\n<<<<\n" ++ RText = lists:reverse(Text0),
+                    case lists:reverse(RText) of
+                        "[[reparse]]" ++ Rest ->


It seems like the nodes tagged with [[reparse]] are not wrapped in >>>> markers

michalmuskala · 2021-02-09T19:43:21Z

src/erlfmt_ast.erl

+            [[Name], Args] = st_subtrees_to_erlfmt(Node),
+            case Name of
+                {remote, NPos, M, N} ->
+                    exit(remote_type),


Why the exits here for type conversions?

michalmuskala · 2021-02-09T19:48:10Z

test/erlfmt_SUITE.erl

                    {atom, _, ok}
                ]}
            ]},
            []},
-        parse_expr("try ok of _ -> ok catch _ -> ok; _:_ -> ok; _:_:_ -> ok end")
+        %% Note: formatting may drop the Trace part if it's just an underscore


You mean in erlfmt or during the rountrip through erl_syntax?

michalmuskala · 2021-02-09T20:03:49Z

test/erlfmt_SUITE.erl

+    GroupProps = ?config(tc_group_properties, Config),
+    case proplists:get_bool(syntax_tools, GroupProps) of
+        true ->
+            put('$syntax_tools$', true);


I think this is fine. It's not pretty, but it's practical. I actually wonder if we should try doing something in erlfmt_format_SUITE as well to leverage all the examples we have in there, though I'm not 100% sure how that would look like.

Use erlfmt instead of els_dodger for parsing. Uses modified version of erlfmt_ast from PR WhatsApp/erlfmt#237 to convert from erlfmt parse tree to erl_syntax:syntaxTree. Expected benefits are more precise locations and parsing exotic macros.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 14, 2021

richcarl force-pushed the syntaxtools-compat branch 2 times, most recently from 5930f24 to db87bdf Compare January 15, 2021 08:00

richcarl added 2 commits January 25, 2021 14:47

add library module for converting between erlfmt and syntax tools

ebd6ac1

add tests for syntax tools conversion

35c8687

richcarl force-pushed the syntaxtools-compat branch from db87bdf to 0bdb897 Compare January 25, 2021 14:35

Handle recent changes to try body/clauses in st

7158644

richcarl force-pushed the syntaxtools-compat branch from 0bdb897 to 7158644 Compare January 26, 2021 12:15

awalterschulze reviewed Jan 27, 2021

View reviewed changes

fixups

1331c4d

michalmuskala reviewed Feb 9, 2021

View reviewed changes

gomoripeti mentioned this pull request Apr 10, 2021

Use erlfmt for parsing erlang-ls/erlang_ls#979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add library module for converting between erlfmt and syntax tools #237

add library module for converting between erlfmt and syntax tools #237

richcarl commented Jan 14, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 20, 2021

richcarl commented Jan 26, 2021

awalterschulze left a comment •

edited

Loading

awalterschulze Jan 27, 2021

richcarl Jan 27, 2021

awalterschulze Jan 27, 2021

michalmuskala Feb 9, 2021

awalterschulze Jan 27, 2021

richcarl Jan 27, 2021

awalterschulze Jan 27, 2021

richcarl commented Jan 27, 2021

awalterschulze commented Jan 27, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

michalmuskala Feb 9, 2021

		%% * `{type, Anno, Args, Res}` for the anonymous function type
		%% `fun((...Args) -> Res)` where `Args` is a `args` node.

add library module for converting between erlfmt and syntax tools #237

Are you sure you want to change the base?

add library module for converting between erlfmt and syntax tools #237

Conversation

richcarl commented Jan 14, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 18, 2021

richcarl commented Jan 18, 2021

awalterschulze commented Jan 20, 2021

richcarl commented Jan 26, 2021

awalterschulze left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richcarl commented Jan 27, 2021

awalterschulze commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalterschulze left a comment •

edited

Loading