erlang_ls icon indicating copy to clipboard operation
erlang_ls copied to clipboard

Go to definition/find references does not work from attributes referencing functions or via macros

Open dszoboszlay opened this issue 4 years ago • 10 comments

Describe the bug Consider the following module:

-module(test).
-export([foo/0, bar/0, baz/0]).
-deprecated([foo/0]).

-define(MYSELF, test).

foo() -> ?MODULE:bar().
bar() -> ?MYSELF:baz().
baz() -> ok.

I would expect go to definition/find references would work from every place foo, bar or baz is mentioned, but it does not.

To Reproduce Try to use go to definition on the function name in the following places:

  • -deprecated([foo/0]).
  • ?MYSELF:baz()

Or try to find references for the baz/0 function, and none will turn up.

Expected behavior Go to definition/find references would work from all places.

Actual behavior It does not work across module attributes (except for -export) or macros (except for ?MODULE).

Context

  • erlang_ls version (tag/sha): 5e754dba9
  • Editor used: Emacs
  • LSP client used: lsp-mode-20200115.950

dszoboszlay avatar Jan 30 '20 10:01 dszoboszlay

Thanks for the report @dszoboszlay , we will look into this.

robertoaloi avatar Jan 30 '20 14:01 robertoaloi

As a reference for other contributors, the deprecated attribute is used to inform xref (not the compiler) about deprecations:

https://erlang.org/doc/man/xref.html#deprecated_function

robertoaloi avatar Jan 30 '20 15:01 robertoaloi

Support for the deprecated attribute is something that is easy to add, so we can do that. I am not sure if we can generalize it for custom attributes (some information is lost for wild attributes, making it really hard to fetch position information), but I will check.

The second point requires more effort. We support ?MODULE because it's extremely widely spread, but processing generic macros is another story.

Contributions are also welcome :)

robertoaloi avatar Jan 30 '20 15:01 robertoaloi

IMHO we shouldn't process macros, that gets very complicated very quickly.

jfacorro avatar Jan 30 '20 15:01 jfacorro

I don't think you should try to find function names in arbitrary attributes. But there are a number of attributes defined in OTP that would make sense to parse (export, import, compile, deprecated and maybe some more).

Regarding processing macros: don't you already have to do that? Compiling the module would involve running the preprocessor too.

dszoboszlay avatar Jan 30 '20 22:01 dszoboszlay

Regarding processing macros: don't you already have to do that? Compiling the module would involve running the preprocessor too.

Compiling is different, since the compiler takes care of everything. We only use the compiler for the warning. The Erlang standard library doesn't expose an API to use just the pre-processor.

We are currently avoiding expanding macros through the usage of a fork of eep_dodger, which attempts to dodge macros by parsing and return them as a syntax tree.

jfacorro avatar Jan 31 '20 03:01 jfacorro

We can do things incrementally, starting from the simple ones. I will probably split this ticket into two or three, and we can address them separately.

robertoaloi avatar Jan 31 '20 09:01 robertoaloi

The Erlang standard library doesn't expose an API to use just the pre-processor.

The 'P' option for the compiler would "produce a listing of the parsed code, after preprocessing and parse transforms", which I think is what you need. Just take care of respecting the -file(FileName, LineNumber). attributes inserted by the compiler in order to map lines back to positions in the original sources.

dszoboszlay avatar Jan 31 '20 09:01 dszoboszlay

@michalmuskala will your new parser help here?

alanz avatar Feb 03 '20 09:02 alanz

Not directly, but it could make it simpler.

The parser tries to keep as much information as possible (e.g. retaining full AST for values of attributes instead of converting to forms), and makes macros first-class elements of the AST. With those changes, it's easier to process those elements retaining full information about positions, etc. But it would still require custom implementation for how to do it all.

michalmuskala avatar Feb 04 '20 16:02 michalmuskala