Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prioritize Uris #648

Merged
merged 1 commit into from
May 10, 2020
Merged

Prioritize Uris #648

merged 1 commit into from
May 10, 2020

Conversation

dgud
Copy link
Contributor

@dgud dgud commented Apr 1, 2020

If a module have several Uris prioritize them.

Search for project related Uri first and prioritize modules in src dir.

When working OTP reposiory we get at least two uris per module, one
in src tree and one in otp release tree, when jumping to definition
we want to jump to the one in the src tree and not to the release tree.

Also some modules names are duplicated in tests dir, avoid these too.

Now that the indexing is parallel the order is random, so it's not
given where to go.

If a module have several Uris prioritize them.

Search for project related Uri first and prioritize modules in src dir.

When working OTP reposiory we get at least two uris per module, one
in src tree and one in otp release tree, when jumping to definition
we want to jump to the one in the src tree and not to the release tree.

Also some modules names are duplicated in tests dir, avoid these too.

Now that the indexing is parallel the order is random, so it's not
given where to go.
@robertoaloi
Copy link
Member

@dgud Can I ask you to share the erlang_ls.config file you're using for the OTP repo? I would like to start playing around with Erlang LS with that code base, too, and possibly add a reference config for OTP in the docs website.

@robertoaloi
Copy link
Member

Also, I am not sure we should take this decision for the user. For example, in the definition provider, the protocol allows multiple definitions to be sent to the client. I think the server should be as dumb as possible and return all of the possible ones. It should be the client to render them to the user (e.g. in a dropdown). Then, to solve the specific issue with the OTP code base, we may want to introduce a blacklist which would have to be configured via the config file.

@dgud
Copy link
Contributor Author

dgud commented Apr 6, 2020

I have not used any erlang_ls.config, IMHO most projects should work decently without one.
I merge all my PR's and use that currently.

You can see this as an intermediate patch, until you fix that all references are returned,
but even so I would like them to be prioritized.

I have an installed erlang dev system 22.3 and the otp git repo, so the erlang_ls node will pick up all
modules twice, and some more (mnesia) because they are in some test directories as well.
Please test, I'm sure you will see the same problems as I did and have tried to fix.

@robertoaloi
Copy link
Member

My idea is a bit different. The els_config module should be as dumb as possible (it contains too much magic even today). Agreed that most projects should work out-of-the-box, but this should be done by separate layers. For example, a rebar3 project type would contain all the logic required to run rebar3 projects (inferring those info from the rebar.config and mapping them to low-level instructions for the els_config module. Same for a otp project type (the OTP code base is so specific that it may require an actual project type). This would ensure us to have a stable common ground and to improve support for individual project types incrementally. Having all this logic in the els_config would force us to be really scared when touching it, since we could break things for a different project type.

@alanz
Copy link
Contributor

alanz commented Apr 8, 2020

@robertoaloi that is basically the approach followed for haskell by using hie-bios

@dgud
Copy link
Contributor Author

dgud commented May 6, 2020

See also #358
The protocol supports returning several references or ?

@robertoaloi
Copy link
Member

@dgud In general, yes (for example in code navigation or in the context of completions). Unfortunately, the find_module function is also used in contexts where exactly one result is expected (e.g. when showing the edoc on hover). The real bug here is #358, I don't have a problem in merging this PR since it will at least bring some well defined behaviour in case of multiple modules with the same name (which is already a corner case). @alanz ? @jfacorro ?

@alanz
Copy link
Contributor

alanz commented May 6, 2020

I agree with the logic around dealing with it, in a well-defined way.

This can hold the line until we have a build-server specific approach which should avoid the problem, and at the very least give us a point to tweak the priority algorithm if we find cases it does not handle properly.

@robertoaloi robertoaloi merged commit d7ef0f1 into erlang-ls:master May 10, 2020
the-mikedavis added a commit to the-mikedavis/erlang_ls that referenced this pull request Oct 24, 2022
…_paths/2`

erlang-ls#346 discarded any paths containing symlinks since
they broke assumptions about there being a single URI for each module
in calls to `els_utils:find_module/1`.

erlang-ls#648 added prioritization for cases when there are
multiple URIs in `els_utils:find_module/1`, so discarding paths with
symlinks is now no longer necessary.

Following symlinks is necessary when using rebar3 checkout dependencies
(erlang-ls#1046) or when using Bazel
(erlang-ls#1064) since Bazel places dependencies and compiled
files under its cache directory and symlinks the relevant directory in
the cache to the workspace directory.

This change removes the behavior of `els_utils:resolve_paths/2` that
discards paths containing symlinks.
robertoaloi pushed a commit that referenced this pull request Nov 8, 2022
#346 discarded any paths containing symlinks since
they broke assumptions about there being a single URI for each module
in calls to `els_utils:find_module/1`.

#648 added prioritization for cases when there are
multiple URIs in `els_utils:find_module/1`, so discarding paths with
symlinks is now no longer necessary.

Following symlinks is necessary when using rebar3 checkout dependencies
(#1046) or when using Bazel
(#1064) since Bazel places dependencies and compiled
files under its cache directory and symlinks the relevant directory in
the cache to the workspace directory.

This change removes the behavior of `els_utils:resolve_paths/2` that
discards paths containing symlinks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants