Merge pull request erlang-ls#909 from gomoripeti/fix_type_attributes

Handle macros in type attributes

apps/els_core/src/els_dodger.erl

@@ -877,8 +877,9 @@ rewrite(Node) ->
  _ ->
  Node
  end;
- tuple ->
- case erl_syntax:tuple_elements(Node) of
+ Type when Type =:= tuple;
+ Type =:= tuple_type ->
+ case tuple_elements(Node, Type) of
  [MagicWord, A | As] ->
  case erl_syntax:type(MagicWord) of
  atom ->
@@ -899,9 +900,15 @@ rewrite(Node) ->
  rewrite_1(Node)
  end.
 
+-spec tuple_elements(erl_syntax:syntaxTree(), atom()) -> [erl_syntax:syntaxTree()].
+tuple_elements(Node, tuple) ->
+ erl_syntax:tuple_elements(Node);
+tuple_elements(Node, tuple_type) ->
+ erl_syntax:tuple_type_elements(Node).
+
 -spec rewrite_1(erl_syntax:syntaxTree()) -> erl_syntax:syntaxTree().
 rewrite_1(Node) ->
- case erl_syntax:subtrees(Node) of
+ case subtrees(Node) of
  [] ->
  Node;
  Gs ->
@@ -911,6 +918,67 @@ rewrite_1(Node) ->
  erl_syntax:copy_pos(Node, Node1)
  end.
 
+%% @doc Return the list of all subtrees of a syntax tree with special handling
+%% for type attributes.
+%%
+%% Background: In erl_parse AST the arguments of a wild attribute are
+%% represented as plain terms. To make response consistent
+%% `attribute_arguments/1' returns the abstract format of those terms. However
+%% `erl_syntax' doesn't know that `callback', `spec', `type' and `opaque' are
+%% not wild attributes and have their arguments partially in abstract format
+%% already. So `erl_syntax' returns the AST of the AST for these attributes. To
+%% fix this we need to convert them back with `concrete/1' to be able to
+%% properly traverse them. This is necessary to be able to find and rewrite
+%% special expressions representing macros.
+-spec subtrees(erl_syntax:syntaxTree()) -> [[erl_syntax:syntaxTree()]].
+subtrees(Node) ->
+ case is_type_attribute(Node) of
+ {true, AttrName} ->
+ [[erl_syntax:attribute_name(Node)],
+ type_attribute_arguments(Node, AttrName)];
+ false ->
+ erl_syntax:subtrees(Node)
+ end.
+
+-spec is_type_attribute(erl_syntax:syntaxTree()) -> {true, atom()} | false.
+is_type_attribute(Node) ->
+ case erl_syntax:type(Node) of
+ attribute ->
+ NameNode = erl_syntax:attribute_name(Node),
+ case erl_syntax:type(NameNode) of
+ atom ->
+ AttrName = erl_syntax:atom_value(NameNode),
+ case lists:member(AttrName, [callback, spec, type, opaque]) of
+ true ->
+ {true, AttrName};
+ false ->
+ false
+ end;
+ _ ->
+ false
+ end;
+ _ ->
+ false
+ end.
+
+-spec type_attribute_arguments(erl_syntax:syntaxTree(), atom())
+ -> [erl_syntax:syntaxTree()].
+type_attribute_arguments(Node, AttrName) when AttrName =:= callback;
+ AttrName =:= spec ->
+ [Arg] = erl_syntax:attribute_arguments(Node),
+ {FA, DefinitionClauses} = erl_syntax:concrete(Arg),
+ [erl_syntax:tuple([erl_syntax:abstract(FA),
+ erl_syntax:list(DefinitionClauses)])];
+type_attribute_arguments(Node, AttrName) when AttrName =:= opaque;
+ AttrName =:= type ->
+ [Arg] = erl_syntax:attribute_arguments(Node),
+ {TypeName, Definition, TypeArgs} = erl_syntax:concrete(Arg),
+ [erl_syntax:tuple([erl_syntax:abstract(TypeName),
+ Definition,
+ erl_syntax:list(TypeArgs)])].
+
+
+
 %% attempting a rescue operation on a token sequence for a single form
 %% if it could not be parsed after the normal treatment
 

apps/els_lsp/src/els_parser.erl

@@ -65,15 +65,15 @@ parse_form(IoDevice, StartLocation, Parser, _Options) ->
  {eof, _EndLocation} = Eof -> Eof
  end.
 
-%% @Doc Find POIs in attributes additionally using tokens to add location info
+%% @doc Find POIs in attributes additionally using tokens to add location info
 %% missing from the syntax tree. Other attributes which don't need tokens are
 %% processed in `attribute/1'.
 -spec find_attribute_pois(erl_syntax:syntaxTree(), [erl_scan:token()]) ->
  [poi()].
 find_attribute_pois(Tree, Tokens) ->
  case erl_syntax:type(Tree) of
  attribute ->
- try erl_syntax_lib:analyze_attribute(Tree) of
+ try analyze_attribute(Tree) of
  {export, Exports} ->
  %% The first atom is the attribute name, so we skip it.
  [_|Atoms] = [T || {atom, _, _} = T <- Tokens],
@@ -111,6 +111,37 @@ find_attribute_pois(Tree, Tokens) ->
  []
  end.
 
+%% @doc Analyze an attribute node with special handling for type attributes.
+%%
+%% `erl_syntax_lib:analyze_attribute` can't handle macros in wild attribute
+%% arguments. It also handles `callback', `spec', `type' and `opaque' as wild
+%% attributes. Therefore `els_dodger' has to handle these forms specially and
+%% here we have to adopt to the different output of `els_dodger'.
+%%
+%% @see els_dodger:subtrees/1
+-spec analyze_attribute(tree()) -> {atom(), term()} | preprocessor.
+analyze_attribute(Tree) ->
+ case attribute_name_atom(Tree) of
+ AttrName when AttrName =:= callback;
+ AttrName =:= spec ->
+ [ArgTuple] = erl_syntax:attribute_arguments(Tree),
+ [FATree | _] = erl_syntax:tuple_elements(ArgTuple),
+ Definition = [], %% ignore definition
+ %% concrete will throw an error if `FATRee' contains any macro
+ {AttrName, {AttrName, {erl_syntax:concrete(FATree), Definition}}};
+ AttrName when AttrName =:= opaque;
+ AttrName =:= type ->
+ [ArgTuple] = erl_syntax:attribute_arguments(Tree),
+ [TypeTree, _, ArgsListTree] = erl_syntax:tuple_elements(ArgTuple),
+ Definition = [], %% ignore definition
+ %% concrete will throw an error if `TyperTree' is a macro
+ {AttrName, {AttrName, {erl_syntax:concrete(TypeTree),
+ Definition,
+ erl_syntax:list_elements(ArgsListTree)}}};
+ _ ->
+ erl_syntax_lib:analyze_attribute(Tree)
+ end.
+
 -spec find_compile_options_pois([any()] | tuple(), [erl_scan:token()]) ->
  [poi()].
 find_compile_options_pois(CompileOpts, Tokens) when is_tuple(CompileOpts) ->
@@ -234,7 +265,7 @@ application_with_variable(Operator, A) ->
 -spec attribute(tree()) -> [poi()].
 attribute(Tree) ->
  Pos = erl_syntax:get_pos(Tree),
- try erl_syntax_lib:analyze_attribute(Tree) of
+ try analyze_attribute(Tree) of
  %% Yes, Erlang allows both British and American spellings for
  %% keywords.
  {behavior, {behavior, Behaviour}} ->
@@ -590,11 +621,7 @@ subtrees(Tree, type_application) ->
  , erl_syntax:type_application_arguments(Tree)
  ];
 subtrees(Tree, attribute) ->
- NameNode = erl_syntax:attribute_name(Tree),
- AttrName = case erl_syntax:type(NameNode) of
- atom -> erl_syntax:atom_value(NameNode);
- _ -> NameNode
- end,
+ AttrName = attribute_name_atom(Tree),
  Args = case erl_syntax:attribute_arguments(Tree) of
  none -> [];
  Args0 -> Args0
@@ -603,13 +630,16 @@ subtrees(Tree, attribute) ->
 subtrees(Tree, _) ->
  erl_syntax:subtrees(Tree).
 
-%% Note: In erl_parse AST the arguments of a wild attribute are represented as
-%% plain terms. To make response consistent `attribute_arguments/1' returns the
-%% abstract format of those terms. However `erl_syntax' doesn't know that
-%% `callback', `spec', `type' and `opaque' are not wild attributes and have
-%% their arguments in abstract format already. So `erl_syntax' returns the AST
-%% of the AST for these attributes. To fix this we need to convert them back
-%% with `concrete/1' to be able to properly traverse them.
+-spec attribute_name_atom(tree()) -> atom() | tree().
+attribute_name_atom(Tree) ->
+ NameNode = erl_syntax:attribute_name(Tree),
+ case erl_syntax:type(NameNode) of
+ atom ->
+ erl_syntax:atom_value(NameNode);
+ _ ->
+ Tree
+ end.
+
 -spec attribute_subtrees(atom() | tree(), [tree()]) -> [[tree()]].
 attribute_subtrees(AttrName, [Mod])
  when AttrName =:= module;
@@ -629,19 +659,8 @@ attribute_subtrees(AttrName, _)
  when AttrName =:= include;
  AttrName =:= include_lib ->
  [];
-attribute_subtrees(AttrName, [Arg])
- when AttrName =:= callback;
- AttrName =:= spec ->
- {_FA, DefinitionClauses} = erl_syntax:concrete(Arg),
- [DefinitionClauses];
-attribute_subtrees(AttrName, [Arg])
- when AttrName =:= type;
- AttrName =:= opaque ->
- {_TypeName, Definition, TypeArgs} = erl_syntax:concrete(Arg),
- [TypeArgs, [Definition]];
 attribute_subtrees(AttrName, Args)
  when is_atom(AttrName) ->
- %% compile, export, export_type and wild attributes
  [Args];
 attribute_subtrees(AttrName, Args) ->
  %% Attribute name not an atom, probably a macro

apps/els_lsp/test/els_parser_SUITE.erl

@@ -20,6 +20,10 @@
  , types_recursive/1
  , opaque_recursive/1
  , record_def_recursive/1
+ , callback_macro/1
+ , spec_macro/1
+ , type_macro/1
+ , opaque_macro/1
  ]).
 
 %%==============================================================================
@@ -174,6 +178,34 @@ assert_recursive_types(Text) ->
  parse_find_pois(Text, type_application)),
  ok.
 
+-spec callback_macro(config()) -> ok.
+callback_macro(_Config) ->
+ Text = "-callback foo() -> ?M().",
+ ?assertMatch([_], parse_find_pois(Text, callback, {foo, 0})),
+ ?assertMatch([_], parse_find_pois(Text, macro, 'M')),
+ ok.
+
+-spec spec_macro(config()) -> ok.
+spec_macro(_Config) ->
+ Text = "-spec foo() -> ?M().",
+ ?assertMatch([_], parse_find_pois(Text, spec, {foo, 0})),
+ ?assertMatch([_], parse_find_pois(Text, macro, 'M')),
+ ok.
+
+-spec type_macro(config()) -> ok.
+type_macro(_Config) ->
+ Text = "-type t() :: ?M(a, b).",
+ ?assertMatch([_], parse_find_pois(Text, type_definition, {t, 0})),
+ ?assertMatch([_], parse_find_pois(Text, macro, 'M')),
+ ok.
+
+-spec opaque_macro(config()) -> ok.
+opaque_macro(_Config) ->
+ Text = "-opaque o() :: ?M(a, b).",
+ ?assertMatch([_], parse_find_pois(Text, type_definition, {o, 0})),
+ ?assertMatch([_], parse_find_pois(Text, macro, 'M')),
+ ok.
+
 %%==============================================================================
 %% Helper functions
 %%==============================================================================