[fix] Determine if completion is displayable based on the project dependencies #9

doorgan · 2025-04-03T05:49:29Z

Fixes #8

This shifts the approach from "is this suggestion part of RemoteControl", which requires quite a lot of special casing, to "is this suggestion part of the project" instead.

The approach here is to:

Determine the application that defined the suggested module
Determine the dependency applications of the project
Check if the application from 1 is part of any dependency of the project, or any of the applications loaded by default by elixir

I think we should generally avoid doing string checks on module names to determine where they beling to, since a project may define modules in the same namespace as some other project. For example, if you are editing a different project/application that defines modules in the Lexical namespace: you'd want completions for the modules you define but not for the ones in Lexical.RemoteControl.

For 2, which is the hardest part here, I decided to parse the .app files in the build directory. The rationale here is that:

As soon as the project is compiled, which expert does as-you-type, the app files get updated
We can trust that it only contains applications related to the project and nothing else, it's not polluted by RemoteControl

What I experienced with other options was:

mix.lock files include dependencies, but not extra_applications or applications loaded by elixir by default
The result from Mix.Project.deps_apps() is sometimes [] or out of date, which seemed quite unreliable
The results from :application_controller.which_applications(), besides apparently being private API, also include RemoteControl dependencies, applications, and namespaced applications

The downside is that we need to parse the .app files a few times whenever we have a bunch of suggestions to check.

lukaszsamson · 2025-04-03T07:26:32Z

I think this approach misses extra_applications from project and deps, beam apps like stdlib that elixir auto loads or preloaded erts app.

Note 1 about application controller: elixir compiler does not update entries there on recompilation.
Note 2 about application controller: erts app is not loaded by default and yet modules from there are available

doorgan · 2025-04-03T18:11:58Z

@lukaszsamson another alternative I see is to read the .app files in the project's build directory (.lexical/build/erl-x/elixir-y/test/lib/<app>/ebin/<app>.app)
Those files include all the applications and optional applications. The downside of course is again having to read files from disk(many of them this time), but it does have a comprehensive and correct list of applications, and it does get udpated on recompilation.

29d61f0 implements the approach I mention here. I think we might also be able to cache those, but if we have as-you-time recompilation I'd have to measure to know if it's really worth it

doorgan · 2025-04-03T21:34:58Z

apps/remote_control/test/fixtures/project/mix.exs

    [
      # {:dep_from_hexpm, "~> 0.3.0"},
      # {:dep_from_git, git: "https://github.com/elixir-lang/my_dep.git", tag: "0.1.0"}
+      {:stream_data, "~> 1.2"}


There was a test that checked that a SreamData module was being completed. It worked before because we only checked if the module wasn't a Lexical one, but with the new approach that checks if module is part of the project, we actually need the dependency to be installed.

doorgan · 2025-04-04T20:03:31Z

I've been having quite a lot of bad luck with the CI on a flaky test here

doorgan · 2025-04-04T20:04:41Z

apps/server/lib/lexical/server/code_intelligence/completion.ex

+    # we consider it a "project module" if:
+    # 1. the module is in the project apps
+    # 2. there is no module app and there is not a project module
+    #    (this is the case for some test completions)


Alternatively we can make the tests correctly recognize all project defined modules as part of the project application, still getting familiarized with how this happens

doorgan · 2025-04-04T20:14:08Z

apps/server/lib/lexical/server/code_intelligence/completion.ex

+  defp get_project_apps(project) do
+    build_path = RemoteControl.Build.path(project)
+
+    for app <- File.ls!(Path.join([build_path, "test", "lib"])),


I'm assuming here that by the time we try to get completions, RemoteControl is already started and the project is already compiled. I haven't found issues using a build of this PR for a while, but it's still an assumption.

I think this function should be pushed into remote_control since it's in control of the project and what's going on. I think that doing operations inside of remote control will allow you to leverage the mix project.

Having this logic in the server app was, i think, a mistake.

I think that doing operations inside of remote control will allow you to leverage the mix project.

Yes and no; Mix does not always produce the list of project applications. I'm not sure why exactly, but I would often get an empty list of applications back from Mix. It would also, with current Mix.Project apis, miss both the project and the dependencies extra applications.

Loading the .app files doesn't require an rpc call to the remote control node, but I see the point of keeping project related logic in the remote control app, I can move this code to that app

scohen · 2025-04-08T21:20:59Z

apps/common/lib/lexical/ast/module.ex

+    module
+  end
+
+  def to_atom(module_string) when is_binary(module_string) do


is this safe? what happens if we pass in :some_module here?

scohen · 2025-04-08T21:21:54Z

apps/remote_control/lib/lexical/remote_control.ex

+  def project_apps(project) do
+    build_path = RemoteControl.Build.path(project)
+
+    for app <- File.ls!(Path.join([build_path, "test", "lib"])),


can we not use Mix.Project here to do this work?

consulting the filesystem on every module completion will cost a lot of performance

can we not use Mix.Project here to do this work?

I tried, it doesn't yield good results. As an example, I just tried this on an igniter clone and I get these results from the RemoteControl node:

Mix.Project.apps_paths: nil Mix.Project.deps_apps: []

Running completions a few times I eventually get this:

Mix.Project.apps_paths: nil Mix.Project.deps_apps: [:sourceror, :earmark_parser, :text_diff, :file_system, :deep_merge, :decimal, :spitfire, :eflame, :mime, :nimble_options, :nimble_parsec, :bunt, :telemetry, :ex_check, :jason, :doctor, :statistex, :mix_test_watch, :ham, :hpax, :mint, :yamerl, :yaml_elixir, :mix_audit, :glob_ex, :credo, :phx_new, :makeup, :makeup_elixir, :makeup_erlang, :ex_doc, :inflex, :erlex, :dialyxir, :nimble_pool, :finch, :req, :rewrite, :benchee, :owl, :mimic]

Which is close, but is missing :logger, :public_key, :ssl, :inets, some of which are loaded by elixir, some of which were defined as extra_applications in one of the mix files. As far as I can tell, there is no public Mix api to get those, so we need to resort to reading manifests in the filesystem to get everything(which is close to what mix does anyways after compilation).

Also, adding a dependency causes Mix.deps_apps to return [] again for a while until it catches up.

consulting the filesystem on every module completion will cost a lot of performance

I was mulling over this too, I ultimately decided to fetch the project applications after compilation, and keep them in memory for completions.

the missing apps are all in erlang though, and should be available for completions.

That's a good call, I added the builtin apps from both erlang and elixir in acc6565. Short of an api to list them, I had to hardcode them.

scohen · 2025-04-10T23:54:29Z

apps/server/lib/lexical/server/code_intelligence/completion.ex

+    module_app in project_apps or (is_nil(module_app) and is_nil(project.project_module)) or
+      (not is_nil(module_app) and module_app == project_app) or
+      (is_nil(module_app) and not is_nil(project.project_module) and
+         module == project.project_module) or
+      (is_nil(metadata) or result_app in project_apps)


would pushing this into the remote control app help things?

Also, i think a cond would be a lot more readable than this very nested statement.

would pushing this into the remote control app help things?

No, there is something in the test setup that is preventing the application controller from recognizing the Project fixture modules as part of the :project application, I'm having a bit of a hard time figuring out what is it exactly.

Also, i think a cond would be a lot more readable than this very nested statement.

Updated in 0ff39e9

scohen · 2025-04-28T15:45:17Z

apps/remote_control/lib/lexical/remote_control.ex


  defdelegate workspace_symbols(query), to: CodeIntelligence.Symbols, as: :for_workspace

+  defdelegate list_apps(), to: RemoteControl.Build.ApplicationCache, as: :list_apps


is the as: necessary here?

Trying without it seems to work, so no
This is an old commit though

…pendencies

doorgan · 2025-06-26T18:56:52Z

This has been working as expected in my testing for quite a while, and I need the changes to namespacing in this PR for the burrito packaging work, so I'm merging it

doorgan force-pushed the doorgan/fix_completions_for_shared_dependencies branch from 2b53730 to 5ef3103 Compare April 3, 2025 21:27

doorgan commented Apr 3, 2025

View reviewed changes

doorgan requested a review from scohen April 4, 2025 20:03

doorgan commented Apr 4, 2025

View reviewed changes

scohen reviewed Apr 10, 2025

View reviewed changes

doorgan force-pushed the doorgan/fix_completions_for_shared_dependencies branch 2 times, most recently from 4b94717 to 0ff39e9 Compare April 11, 2025 05:58

doorgan mentioned this pull request Apr 24, 2025

Rename project to Expert #16

Merged

scohen reviewed Apr 29, 2025

View reviewed changes

doorgan force-pushed the doorgan/fix_completions_for_shared_dependencies branch from 3f78e5a to 34e1a6d Compare May 15, 2025 21:07

doorgan requested a review from scohen May 27, 2025 17:34

doorgan force-pushed the doorgan/fix_completions_for_shared_dependencies branch from 484dc3d to a5d9989 Compare June 26, 2025 18:44

fix: Exclude expert dependencies from completions based on project de…

3a47058

…pendencies

doorgan force-pushed the doorgan/fix_completions_for_shared_dependencies branch from a5d9989 to 3a47058 Compare June 26, 2025 18:49

doorgan merged commit 4ab4fb4 into main Jun 26, 2025
12 checks passed

doorgan deleted the doorgan/fix_completions_for_shared_dependencies branch June 26, 2025 18:57


		defdelegate workspace_symbols(query), to: CodeIntelligence.Symbols, as: :for_workspace

		defdelegate list_apps(), to: RemoteControl.Build.ApplicationCache, as: :list_apps

Uh oh!

[fix] Determine if completion is displayable based on the project dependencies #9

[fix] Determine if completion is displayable based on the project dependencies #9

Uh oh!

Conversation

doorgan commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszsamson commented Apr 3, 2025

Uh oh!

doorgan commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doorgan commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doorgan Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doorgan Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doorgan commented Jun 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

doorgan commented Apr 3, 2025 •

edited

Loading

doorgan Apr 8, 2025 •

edited

Loading

doorgan Apr 11, 2025 •

edited

Loading