Skip to content
This repository was archived by the owner on Aug 15, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rebar.config
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
%% upstream version:
%% https://github.com/massemanet/redbug/pull/10
, [ {redbug, {git, "https://github.com/robertoaloi/redbug.git", {branch, "relax-beam-lib-error-matching"}}}
, {eflame, {git, "https://github.com/jfacorro/eflame.git", {branch, "various.improvements"}}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to bring in the upstream version, if possible.
What are the included changes?
Are you pull-requesting them to upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the PR with all the improvements.

]
}
, { escript_emu_args
Expand Down
2 changes: 1 addition & 1 deletion src/els_code_navigation.erl
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ find([Uri|Uris0], Kind, Data) ->
[] ->
find(lists:usort(include_uris(Document) ++ Uris0), Kind, Data);
Definitions ->
{ok, Uri, lists:last(Definitions)}
{ok, Uri, hd(lists:sort(Definitions))}
end;
{error, not_found} ->
find(Uris0, Kind, Data)
Expand Down
4 changes: 2 additions & 2 deletions src/els_dodger.erl
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@

-export([parse_file/1, quick_parse_file/1, parse_file/2,
quick_parse_file/2, parse/1, quick_parse/1, parse/2,
quick_parse/2, parse/3, quick_parse/3, parse_form/2,
quick_parse/2, parse/3, parse/4, quick_parse/3, parse_form/2,
parse_form/3, quick_parse_form/2, quick_parse_form/3,
format_error/1, tokens_to_string/1]).
format_error/1, tokens_to_string/1, normal_parser/2]).


%% The following should be: 1) pseudo-uniquely identifiable, and 2)
Expand Down
66 changes: 32 additions & 34 deletions src/els_parser.erl
Original file line number Diff line number Diff line change
Expand Up @@ -25,46 +25,47 @@ parse(Text) ->

-spec parse_file(file:io_device()) -> {ok, [poi()]}.
parse_file(IoDevice) ->
{ok, Forms} = els_dodger:parse(IoDevice, {1, 1}),
Tree = erl_syntax:form_list(Forms),
%% Reset file pointer position.
{ok, 0} = file:position(IoDevice, 0),
{ok, Extra} = parse_attribute_pois(IoDevice, [], {1, 1}),
{ok, NestedPOIs} = els_dodger:parse(IoDevice, {1, 1}, fun parse_form/3, []),
ok = file:close(IoDevice),
POIs = points_of_interest(Tree),
{ok, Extra ++ POIs}.
{ok, lists:flatten(NestedPOIs)}.

%%==============================================================================
%% Internal Functions
%%==============================================================================
-spec parse_form(file:io_device(), any(), [any()]) ->
{'ok', erl_syntax:forms()
| none, integer()}
| {'eof', integer()}
| {'error', any(), integer()}.
parse_form(Dev, L0, Options) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a re-implementation of els_dodger:parse_form/3. Why not to expose that one, instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was misreading the code. Ignore this comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative option would have been to add a callback function to the Options list which could have been executed from within the els_dodger, and have the parse_form/4 return the final accumulator together with the list of forms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would have required a change in the implementation of dodger which I was trying to avoid.

parse_form(Dev, L0, fun els_dodger:normal_parser/2, Options).

-spec parse_attribute_pois(io:device(), [poi()], erl_anno:location()) ->
{ok, [poi()]} | {error, any()}.
parse_attribute_pois(IoDevice, Acc, StartLoc) ->
case io:scan_erl_form(IoDevice, "", StartLoc) of
{ok, Tokens, EndLoc} ->
case erl_parse:parse_form(Tokens) of
{ok, Form} ->
POIs = find_attribute_pois(Form, Tokens),
parse_attribute_pois(IoDevice, POIs ++ Acc, EndLoc);
{error, _Error} ->
parse_attribute_pois(IoDevice, Acc, EndLoc)
-spec parse_form(file:io_device(), any(), function(), [any()]) ->
{'ok', erl_syntax:forms() | none, integer()}
| {'eof', integer()}
| {'error', any(), integer()}.
parse_form(Dev, L0, Parser, _Options) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use the original naming for the variables? E.g. IoDevice instead of Dev, StartLoc instead of L0, Tokens instead of Ts, etc?

case io:scan_erl_form(Dev, "", L0) of
{ok, Ts, L1} ->
case catch {ok, Parser(Ts, undefined)} of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use the deprecated catch. Use try/catch instead.

{ok, F} ->
POIs = [find_attribute_pois(F, Ts), points_of_interest(F)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have the single pass, it probably makes more sense to check here whether we are dealing with an attribute and execute the find_attribute_pois function only in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is done in the function itself, adding it here would only make this function harder to read and would bring no benefit IMO.

{ok, POIs, L1};
_ ->
{ok, [], L1}
end;
{eof, _} ->
{ok, Acc};
{error, ErrorInfo, EndLoc} ->
lager:warning( "Could not parse extra information [end_loc=p] ~p"
, [EndLoc, ErrorInfo]
),
{ok, Acc}
{error, _IoErr, _L1} = Err -> Err;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we are pattern matching on {ok, ...} above, maybe we can avoid the defensive style altogether here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result from this function is processed by the code in els_dodger which is pattern matching and handling each of these results though.

{error, _Reason} -> {eof, L0}; % This is probably encoding problem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the (missing an article) comment comes from the dodger, but I would get rid of it and, instead, add a comment to the top of this function explaining this is a simplified version of the respective dodger function.

{eof, _L1} = Eof -> Eof
end.

%%==============================================================================
%% Internal Functions
%%==============================================================================

-spec find_attribute_pois(erl_parse:abstract_form(), [erl_scan:token()]) ->
[poi()].
find_attribute_pois(Form, Tokens) ->
case erl_syntax:type(Form) of
attribute ->
case erl_syntax_lib:analyze_attribute(Form) of
case catch erl_syntax_lib:analyze_attribute(Form) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Please don't use the old-style catch.

{export, Exports} ->
%% The first atom is the attribute name, so we skip it.
[_|Atoms] = [T|| {atom, _, _} = T <- Tokens],
Expand Down Expand Up @@ -114,11 +115,8 @@ do_find_spec_points_of_interest(Tree, Acc) ->

-spec points_of_interest(tree()) -> [poi()].
points_of_interest(Tree) ->
lists:flatten(
erl_syntax_lib:fold(
fun(T, Acc) ->
[do_points_of_interest(T)|Acc]
end, [], Tree)).
FoldFun = fun(T, Acc) -> do_points_of_interest(T) ++ Acc end,
Copy link
Member

@robertoaloi robertoaloi Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the ++ in here? Or can we live with nested lists that we can flatten at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean at the end as in parse/1 or at the end of the fold? If it's the former then I would agree, if it's the latter, I think it's much clearer what's happening if we use the ++.

erl_syntax_lib:fold(FoldFun, [], Tree).

%% @edoc Return the list of points of interest for a given `Tree`.
-spec do_points_of_interest(tree()) -> [poi()].
Expand Down
2 changes: 1 addition & 1 deletion test/els_document_symbol_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ functions(Config) ->
} || {Name, {FromL, FromC}, {ToL, ToC}}
<- lists:append([functions()])],
?assertEqual(length(Expected), length(Symbols)),
Pairs = lists:zip(Expected, Symbols),
Pairs = lists:zip(lists:sort(Expected), lists:sort(Symbols)),
[?assertEqual(E, S) || {E, S} <- Pairs],
ok.

Expand Down