Replacing els_dodger with a different parser

[gomoripeti]

Recently I've been more and more tempted to look at the parser of erlfmt as a replacement for the current epp_dodger-based one. erlfmt_parser has the following advantages:

• better macro handling: macros are part of the grammar and don't have to do clever tricks to circumvent erl_parse
  I wanted to be able to parse the following code and have a POI for ?InnerMacro

?OuterMacro([{deep_term, ?InnerMacro}]).`

• better start/end location handling: erlfmt returns end_location for every AST element so don't have to do tricks in els_parser and assumptions in els_range. Handles edge cases like

?abc. %% macro poi has a length of 4 from ? to c
? abc. %% macro poi should have a length of 5 from ? to c

• returns raw_string and start/end location for unparsable sections, as opposed to an error element only: This can be used to handle incomplete code, by tokenizing the raw_string. As an example would be possible to suggest variable Input in the below example (today the function can't be parsed and results in no POIs)

f(Input) ->
  Inp<cursor>

Most of this could be probably addressed by further work in els_parser and hacking els_dodger further, but then will have to diverge from epp_dodger and feels duplicate work.

Disadvantages:

• erlfmt_parser uses a modified version of the standard Erlang AST so tools like erl_syntax don't (fully) work on it.
• I absolutely don't want this change to influence the later decision to choose which formatter erlang_ls will use in the future. (I personally don't like the formatting of erlfmt). Having two formatters as dependency of erlang_ls might be confusing.

What do you all think?

[elbrujohalcon]

You have to convince WhatsApp folks to extract that parser to its own lib... Like katana-code. Maybe erlcode...

[michalmuskala]

Would it really make that much of a difference to split it into multiple libraries? It adds a lot of additional fiddling with bumping versions and complexities around ensuring different components stay compatible.
I don't think the overhead of 2 extra modules (that are involved in formatting itself rather than parsing) would be noticeable.

We already expose APIs to just use the parser with erlfmt:read_nodes or erlfmt:read_nodes_string.

[elbrujohalcon]

I can't speak for erlang-ls, but from my personal perspective, it does make a difference.
It's likely that the parsing library stays relatively stable for quite a long time while the formatter changes more frequently.
And it's also nice to be able to read the rebar3 tree of a project and see that it depends on a parsing library and not on a formatter.

[gomoripeti]

thanks Michal and Brujo, for moving the discussion forward. I don't have a strong opinion. I think erlfmt is a great piece of code, its parsing half as a standalone lib could serve very well as a next generation, solid base for all kinds of tools working with source code. But I understand the inconvenience in practice of having two very small libs.

some alternative ideas (kinda bad ideas :) )

• a new syntax_tools2 library could contain the two parts of erlfmt and possibly other related tools. (for example I was thinking about a module converting erlfmt_parse:abstract_node() into erl_syntax:syntaxTree(), this would help with compatibility) This would look better in rebar3 tree but I actually prefer more, smaller libs.
• erlfmt could be converted into an umbrella repo. This would help with keeping the components compatible while still creating multiple apps/hex packages. (Last time I checked rebar3 did not have great support for one hex pkg in an umbrella depending on the other)

[elbrujohalcon]

I have a better idea: convince the OTP team to replace erl_syntax with erlfmt_parse

[plux]

While working on the feature to rename functions I've come to realize that the current parser is not sufficient.
Currently there's a lot of faulty assumptions that erlang_ls needs to make due to the insufficient data from the parser.
The parser will return the start of a POI and we need to reverse engineer where the end location is, which may or may not be accurate with the actual source code

Some examples that I've thought about where calculation of the end location will be wrong:

%% Numbers separated with underscore
100_000_000
%% quoted atoms 
'foo'
%% Additional whitespace
fun   foo/1

I think it could be interesting to try to plug in the erlfmt parser as an experiment.

[gomoripeti]

I took a few stabs at using erlfmt in els_parser and the results look promising. This adapter by Richard Carlsson into erl_syntax trees definitely saved a lot of time WhatsApp/erlfmt#237

First I tried how much erlfmt_ast can be a drop in replacement for els_dodger, keeping the reverse-engineering, but there are a few differences still, mostly related to start locations. (I patched erlfmt_ast to handle types the same way as our els_dodger is patched) On the version which also takes end_location from erlfmt I'm close to fixing all the diffs and test cases.

Listing the diffs for the record if anyone would like to try the replacement in other tools:

• export_type entries have different representation, hence analyze_attribute returns different result
• more atoms have non-zero location, hence more redundant atom POIs are created, which breaks some functionality in erlang_ls (zero location atoms are ignored by els_parser)
• erl_scan:tokens EndLocation is different than erlfmt end_location for spec/folding_range
• start location of attribute forms (els_dodger starts after '-' while erlfmt starts at '-')
• start location of application and module_qualifier (els_dodger puts it on ':' while erlfmt starts at module name)
• start location of record access/update (els_dodger starts at '#' while erlfmt starts at Expr, erlfmt_ast does not have exact location of '#')
• erlfmt can parse much more macros, we cannot use erl_syntax_lib:analyze_attribute and friends which don't support macros
• recommenting can modify start/end location of some erlfmt syntax_trees