Use erlfmt for parsing #979

gomoripeti · 2021-04-10T13:32:48Z

Description

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:

more precise locations
parse exotic macros

TODO

fix conflict with Show macro definition on hover #975
update to latest erlfmt
clean up and squash

gomoripeti · 2021-04-11T22:59:58Z

In commit 608fa18 I needed to apply a workaround for hover macro definitions because the magic tuples from erlfmt_ast started to surface https://github.com/erlang-ls/erlang_ls/pull/979/files#diff-cc9d509044808779837f7175898eeab88501b2666fcf925adfc43abc1104d1d5R483-R484

robertoaloi · 2021-04-12T20:48:53Z

apps/els_lsp/src/els_erlfmt_ast.erl

@@ -0,0 +1,1290 @@
+%% Copyright (c) Facebook, Inc. and its affiliates.


Why forking it, instead of adding it as a dependency?

Ah, saw the branch now. What's the plan long term for this?

I hope that branch will eventually be merged and erlfmt_ast be a part of erlfmt and we can remove this temporary copy from erlang_ls

@michalmuskala Any insights on that?

robertoaloi · 2021-04-16T15:12:21Z

@gomoripeti I don't have a strong opinion on this. What we can do is to try out the new parser by merging this PR. We can then see how the "early adopters" (who point to HEAD) feel and then we decide is to definitely move to erlfmt or roll back. Yes, it would be nice to get rid of the fork. We should also look at removing the dodger completely (right now its's also used by the bound-variable-in-pattern code. So, if you squash this, we can get it in and follow up with the rest.

Should conflict with anything

…unction (upstream) Formerly standalone ones (in macro definitions) were converted to magic tuples.

In order to silence compiler warning warn_missing_spec_all

gomoripeti · 2021-04-17T12:53:01Z

Hi Roberto, sorry, I had little time to move this forward. I did the squash (kept erlfmt_ast changes separate so its easier to adapt erlfmt upstream changes)
I didn't have time to test this with a large code base like OTP. If you could give me a few days before merging.
For example testing with an old version of OTP 21 I hit this issue erlang/otp#2348 (resulting in parsing files with map accoss to crash, that's bad). Turns out it was fixed in a later patch for OTP 21, but there might be other glitches that need to be worked around. Also making sure performance does not drop.

- handle argumentless macro as type/opaque name - fix spec/callback location - support record_types in type context - support bitstring_type in type context - convert record field types in a type context - convert spec fun name - silence dialyzer about erl_syntax:function_type

Since OTP 24 (commit erlang/otp@f30514e#diff-9f3b412e8fbd809c6473fcd5bee30f63931c7349010640d76316cfcde504e733) `erl_syntax:set_pos` started to require a proper `erl_anno:anno()` instead of any term. `erlfmt_ast` used to just copy the `erlfmt_scan:anno()` map from the input to `syntaxTree()`, but that does not work any more. Now it is converted to an `erl_anno:anno()` to make dialyzer happy. (There is a bit of cheating with `end_location` in the hope that it will be a valid key in `erl_anno` soon.)

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.

Only store the range of the value in `define` POIs and take the text from the original document. Formerly els_dodger used to create a text tree for definition values it couldn't parse. However erlfmt can parse more cases and erlfmt_ast currently converts some of them to "magic tuples". This change is a workaround to avoid showing magic tuples. An example ``` -define(M, A when A > 1). ``` would show ``` ?M = {'*when*', A, A > 1} ```

Results from multiple diagnostics can arrive in arbitrary order.

gomoripeti · 2021-04-22T21:46:14Z

Tested indexing all of OTP. All files are processed without any crash.

The new implementation is somewhat slower, indexing on select OTP applications (in seconds)
(this is most probably not because of erlfmt_parse, but later steps. It is also possible that more forms can be parsed which are just skipped by els_dodger)
els_dodger
0.989364 "common_test"
1.10195 "compiler"
2.65633 "dialyzer"
2.163496 "kernel"
4.819365 "stdlib"

erlfmt
1.177932 "common_test"
1.320494 "compiler"
3.129172 "dialyzer"
2.58872 "kernel"
6.771605 "stdlib"

I would say this is ready to be merged from my side.

robertoaloi · 2021-04-23T09:40:21Z

Thanks for the analysis @gomoripeti !

Interesting results. Do you think we should proceed anyway? Performance degradation is minimal, but still...
We should also consider that the dodger is still in place, so this does not result in less code, either.
One thing we could check is how many POIs / Documents are identified with the two approaches to quantify the gain?

gomoripeti · 2021-04-23T11:05:02Z

I will give it a bit more thinking/analyses.
(I haven't removed the modules but els_dodger and els_io_string is not used any more (els_dodger call deleted), also there is less fiddling with string length in els_range, so some code is eliminated)

I will gather some concrete benefit examples, to be able to judge if its worth it. Few things that pop in my mind right now:

range of unnecessarily quoted atoms will be correct (eg 'deep' won't be highlighted as 'dee)
it also allows to remove the fiddling and "fundamental flaw" in els_rename_provider

gomoripeti · 2021-05-02T13:52:35Z

I tried to do a bit more profiling. While not full, I think

the els_parser part of the code is not slower.
erl_scan with text option (which is used by erlfmt to calculate end locations) is considerable slower than without the option compared to itself. This might not be the only reason of the new els_parser being slower but it's definitely a significant part. Even if we wouldn't use erlfmt_parse, we would need to use the text option to get proper end locations in one way or the other, so I think this is an acceptable price.

I tried to look at the diff in POIs

the count of POIs is not outstandingly different
lot of difference in actual range values
redundant atoms are created in different places (this makes comparison a bit hard)

the OTP test suites are good with edge cases that the old parser couldn't parse, but I think are rare in real, common code

concatenation of strings and macros, the whole function is missing with the old parser eg erl_eval_SUITE.erl

error_check("{" ?MODULE_STRING ",module_info}().",
            {badfun,{?MODULE,module_info}}),

complicated define with arg, c.erl

-define(RENDERABLE_FORMAT(Format),
        Format =:= ?NATIVE_FORMAT;
        binary_part(Format, 0, 5) =:= <<"text/">>).

old parser: missing folding range for two line functions with any char between dot and \n

f() ->
    ok. %% post comment

All in all, I think it's worth to include this change as it eliminates the need for some heuristics and assumptions

robertoaloi · 2021-05-03T11:02:23Z

Sounds great. I guess we can follow up with @michalmuskala @alanz and the rest of the crew to see if we can get rid of the fork for erlfmt.

gomoripeti force-pushed the erlfmt_phase2 branch from 6b39b65 to 86f3833 Compare April 11, 2021 13:30

robertoaloi reviewed Apr 12, 2021

View reviewed changes

robertoaloi approved these changes Apr 16, 2021

View reviewed changes

gomoripeti added 7 commits April 17, 2021 13:22

Adjust comment of skip_record_field_atom

da4d1dd

Should conflict with anything

vendor erlfmt_ast from syntaxtools-compat branch @ 1331c4d6

f3edd52

erlfmt_ast Fix try with no clauses or handler clauses (submit upstream)

163564d

erlfmt_ast: fix typo text_literal -> text_string (upstream)

0fe3ab1

erlfmt_ast: convert standalone guard_or/guard_and to disjunction/conj…

4032a9c

…unction (upstream) Formerly standalone ones (in macro definitions) were converted to magic tuples.

erlfmt_ast: remove code unused by erlang_ls

377bc67

erlfmt_ast: add spec by typer to all functions

c1bf9d3

In order to silence compiler warning warn_missing_spec_all

gomoripeti force-pushed the erlfmt_phase2 branch 2 times, most recently from b79ada8 to 0daf0fd Compare April 20, 2021 00:24

gomoripeti added 5 commits April 22, 2021 23:30

Use erlfmt for parsing

16e8339

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.

Sort results in diagnostics tests

8887a0d

Results from multiple diagnostics can arrive in arbitrary order.

gomoripeti force-pushed the erlfmt_phase2 branch from 31f5fcd to 8887a0d Compare April 22, 2021 21:40

robertoaloi merged commit fb4aada into erlang-ls:main May 3, 2021

robertoaloi mentioned this pull request May 3, 2021

Parser fails on ?STACKTRACE macro #993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use erlfmt for parsing #979

Use erlfmt for parsing #979

gomoripeti commented Apr 10, 2021

gomoripeti commented Apr 11, 2021

robertoaloi Apr 12, 2021

robertoaloi Apr 12, 2021

gomoripeti Apr 12, 2021

robertoaloi Apr 16, 2021

robertoaloi commented Apr 16, 2021

gomoripeti commented Apr 17, 2021

gomoripeti commented Apr 22, 2021 •

edited

Loading

robertoaloi commented Apr 23, 2021

gomoripeti commented Apr 23, 2021

gomoripeti commented May 2, 2021

robertoaloi commented May 3, 2021

		@@ -0,0 +1,1290 @@
		%% Copyright (c) Facebook, Inc. and its affiliates.

Use erlfmt for parsing #979

Use erlfmt for parsing #979

Conversation

gomoripeti commented Apr 10, 2021

Description

TODO

gomoripeti commented Apr 11, 2021

robertoaloi Apr 12, 2021

Choose a reason for hiding this comment

robertoaloi Apr 12, 2021

Choose a reason for hiding this comment

gomoripeti Apr 12, 2021

Choose a reason for hiding this comment

robertoaloi Apr 16, 2021

Choose a reason for hiding this comment

robertoaloi commented Apr 16, 2021

gomoripeti commented Apr 17, 2021

gomoripeti commented Apr 22, 2021 • edited Loading

robertoaloi commented Apr 23, 2021

gomoripeti commented Apr 23, 2021

gomoripeti commented May 2, 2021

robertoaloi commented May 3, 2021

gomoripeti commented Apr 22, 2021 •

edited

Loading