Merge pull request #9 from elixir-lang/doorgan/fix_completions_for_shared_dependencies

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

Author: doorgan

apps/engine/lib/engine/engine.ex

@@ -5,12 +5,13 @@ defmodule Engine do
   context of the remote VM.
   """
 
-  alias Forge.Project
-
   alias Engine.Api.Proxy
   alias Engine.CodeAction
   alias Engine.CodeIntelligence
   alias Engine.ProjectNode
+  alias Forge.Project
+
+  alias Mix.Tasks.Namespace
 
   require Logger
 
@@ -64,6 +65,12 @@ defmodule Engine do
 
   defdelegate workspace_symbols(query), to: CodeIntelligence.Symbols, as: :for_workspace
 
+  def list_apps do
+    for {app, _, _} <- :application.loaded_applications(),
+        not Namespace.Module.prefixed?(app),
+        do: app
+  end
+
   def start_link(%Project{} = project) do
     :ok = ensure_epmd_started()
     start_net_kernel(project)

apps/engine/lib/engine/engine/api.ex

@@ -1,13 +1,12 @@
 defmodule Engine.Api do
+  alias Engine.CodeIntelligence
   alias Forge.Ast.Analysis
   alias Forge.Ast.Env
   alias Forge.Document
   alias Forge.Document.Position
   alias Forge.Document.Range
   alias Forge.Project
 
-  alias Engine.CodeIntelligence
-
   require Logger
 
   def schedule_compile(%Project{} = project, force?) do
@@ -35,6 +34,10 @@ defmodule Engine.Api do
     Engine.call(project, Engine, :list_modules)
   end
 
+  def project_apps(%Project{} = project) do
+    Engine.call(project, Engine, :list_apps)
+  end
+
   def format(%Project{} = project, %Document{} = document) do
     Engine.call(project, Engine, :format, [document])
   end

apps/engine/lib/mix/tasks/namespace.ex

@@ -15,6 +15,9 @@ defmodule Mix.Tasks.Namespace do
   use Mix.Task
 
   @dev_deps [:patch]
+  # Unless explicitly added, nimble_parsec won't show up as a loaded app
+  # and will therefore not be namespaced.
+  @no_app_deps [:nimble_parsec]
 
   # These app names and root modules are strings to avoid them being namespaced
   # by this task. Plugin discovery uses this task, which happens after
@@ -27,15 +30,13 @@ defmodule Mix.Tasks.Namespace do
     "forge" => "Forge"
   }
 
-  @deps_apps Engine.MixProject.project()
-             |> Keyword.get(:deps)
-             |> Enum.map(&elem(&1, 0))
-             |> then(fn dep_names -> dep_names -- @dev_deps end)
-             |> Enum.map(&to_string/1)
-
   require Logger
 
   def run([base_directory]) do
+    # Ensure we cache the loaded apps at the time of namespacing
+    # Otherwise only the @extra_apps will be cached
+    init()
+
     Transform.Apps.apply_to_all(base_directory)
     Transform.Beams.apply_to_all(base_directory)
     Transform.Scripts.apply_to_all(base_directory)
@@ -115,10 +116,31 @@ defmodule Mix.Tasks.Namespace do
   end
 
   defp init do
-    @deps_apps
-    |> Enum.map(&String.to_atom/1)
+    discover_deps_apps()
+    |> Enum.concat(@no_app_deps)
+    |> then(&(&1 -- @dev_deps))
     |> root_modules_for_apps()
     |> Map.merge(extra_apps())
     |> register_mappings()
   end
+
+  defp discover_deps_apps do
+    cwd = File.cwd!()
+
+    :application.loaded_applications()
+    |> Enum.flat_map(fn {app_name, _description, _version} ->
+      try do
+        app_dir = Application.app_dir(app_name)
+
+        if String.starts_with?(app_dir, cwd) do
+          [app_name]
+        else
+          []
+        end
+      rescue
+        _ -> []
+      end
+    end)
+    |> Enum.sort()
+  end
 end

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

@@ -16,7 +16,7 @@ defmodule Project.MixProject do
   # Run "mix help compile.app" to learn about applications.
   def application do
     [
-      extra_applications: [:logger]
+      extra_applications: [:logger, :erts]
     ]
   end
 

apps/expert/lib/expert/code_intelligence/completion.ex

@@ -17,10 +17,9 @@ defmodule Expert.CodeIntelligence.Completion do
 
   require Logger
 
+  @build_env Mix.env()
   @expert_deps Enum.map([:expert | Mix.Project.deps_apps()], &Atom.to_string/1)
 
-  @expert_dep_modules Enum.map(@expert_deps, &Macro.camelize/1)
-
   def trigger_characters do
     [".", "@", "&", "%", "^", ":", "!", "-", "~"]
   end
@@ -166,9 +165,10 @@ defmodule Expert.CodeIntelligence.Completion do
          %CompletionContext{} = context
        ) do
     debug_local_completions(local_completions)
+    project_apps = Engine.Api.project_apps(project)
 
     for result <- local_completions,
-        displayable?(project, result),
+        displayable?(project, project_apps, result),
         applies_to_context?(project, result, context),
         applies_to_env?(env, result),
         %CompletionItem{} = item <- to_completion_item(result, env) do
@@ -206,7 +206,7 @@ defmodule Expert.CodeIntelligence.Completion do
     |> List.wrap()
   end
 
-  defp displayable?(%Project{} = project, result) do
+  defp displayable?(%Project{} = project, project_apps, result) do
     suggested_module =
       case result do
         %_{full_name: full_name} when is_binary(full_name) -> full_name
@@ -223,16 +223,65 @@ defmodule Expert.CodeIntelligence.Completion do
         true
 
       true ->
-        Enum.reduce_while(@expert_dep_modules, true, fn module, _ ->
-          if String.starts_with?(suggested_module, module) do
-            {:halt, false}
-          else
-            {:cont, true}
-          end
-        end)
+        project_module?(project, project_apps, suggested_module, result)
+    end
+  end
+
+  defp project_module?(_, _, "", _), do: true
+
+  # credo:disable-for-next-line Credo.Check.Refactor.CyclomaticComplexity
+  defp project_module?(%Project{} = project, project_apps, suggested_module, result) do
+    module = module_string_to_atom(suggested_module)
+    module_app = Application.get_application(module)
+    project_app = Application.get_application(project.project_module)
+
+    metadata = Map.get(result, :metadata)
+
+    result_app = metadata[:app]
+
+    cond do
+      module_app in project_apps ->
+        true
+
+      # This is useful for some struct field completions, where
+      # a suggested module is not always part of the result struct,
+      # but the application is.
+      # If no application is set though, it's usually part of a result
+      # that is not part of any application yet.
+      result_app in project_apps or is_nil(metadata) ->
+        true
+
+      not is_nil(module_app) and module_app == project_app ->
+        true
+
+      is_nil(module_app) and not is_nil(project.project_module) and
+          module == project.project_module ->
+        true
+
+      true ->
+        # The following cases happen on test cases, due to the application
+        # controller not always recognizing project fixture modules as part
+        # of any application.
+        test_env?() and is_nil(module_app) and is_nil(project.project_module)
     end
   end
 
+  # Because the build env is fixed at compile time, dialyzer knows that
+  # in :dev and :prod environments, this function will always return false,
+  # so it produces a warning.
+  @dialyzer {:nowarn_function, test_env?: 0}
+  defp test_env?, do: @build_env == :test
+
+  defp module_string_to_atom(""), do: nil
+
+  defp module_string_to_atom(module_string) do
+    Forge.Ast.Module.to_atom(module_string)
+  rescue
+    _e in ArgumentError ->
+      # Return nil if we can't safely convert the module string to an atom
+      nil
+  end
+
   defp applies_to_env?(%Env{} = env, %struct_module{} = result) do
     cond do
       Env.in_context?(env, :struct_reference) ->

apps/expert/test/expert/code_intelligence/completion/translations/module_or_behaviour_test.exs

@@ -3,6 +3,7 @@ defmodule Expert.CodeIntelligence.Completion.Translations.ModuleOrBehaviourTest
   alias GenLSP.Enumerations.InsertTextFormat
 
   use Expert.Test.Expert.CompletionCase
+  use Patch
 
   describe "module completions" do
     test "modules should emit a completion for stdlib modules", %{project: project} do
@@ -262,6 +263,8 @@ defmodule Expert.CodeIntelligence.Completion.Translations.ModuleOrBehaviourTest
          %{
            project: project
          } do
+      patch(Engine.Api, :project_apps, [:project, :ex_unit, :stream_data])
+
       source = ~q[
         use En|
       ]

apps/expert/test/expert/code_intelligence/completion_test.exs

@@ -9,41 +9,62 @@ defmodule Expert.CodeIntelligence.CompletionTest do
   use Expert.Test.Expert.CompletionCase
   use Patch
 
+  setup %{project: project} do
+    project = %{project | project_module: Project}
+    {:ok, project: project}
+  end
+
   describe "excluding modules from expert dependencies" do
     test "expert modules are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Expert.CodeIntelligence|")
     end
 
     test "Expert submodules are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Engin|e")
       assert [] = complete(project, "Forg|e")
     end
 
     test "Expert functions are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Engine.|")
     end
 
     test "Dependency modules are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "ElixirSense|")
     end
 
     test "Dependency functions are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Jason.encod|")
     end
 
     test "Dependency protocols are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Jason.Encode|")
     end
 
     test "Dependency structs are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Jason.Fragment|")
     end
 
     test "Dependency exceptions are removed", %{project: project} do
+      patch(Engine.Api, :project_apps, [:project, :sourceror])
       assert [] = complete(project, "Jason.DecodeErro|")
     end
   end
 
+  test "includes modules from dependencies shared by the project and Expert", %{project: project} do
+    patch(Engine.Api, :project_apps, [:project, :sourceror])
+    assert [sourceror_module] = complete(project, "Sourcer|")
+
+    assert sourceror_module.kind == CompletionItemKind.module()
+    assert sourceror_module.label == "Sourceror"
+  end
+
   test "ensure completion works for project", %{project: project} do
     refute [] == complete(project, "Project.|")
   end
@@ -169,7 +190,7 @@ defmodule Expert.CodeIntelligence.CompletionTest do
 
   def with_all_completion_candidates(_) do
     name = "Foo"
-    full_name = "A.B.Foo"
+    full_name = "Project"
 
     all_completions = [
       %Candidate.Behaviour{name: "#{name}-behaviour", full_name: full_name},

apps/forge/lib/forge/ast/module.ex

@@ -76,6 +76,10 @@ defmodule Forge.Ast.Module do
   def safe_split(module, opts) when is_atom(module) do
     string_name = Atom.to_string(module)
 
+    do_safe_split(string_name, opts)
+  end
+
+  defp do_safe_split(string_name, opts \\ []) do
     {type, split_module} =
       case String.split(string_name, ".") do
         ["Elixir" | rest] ->
@@ -96,4 +100,20 @@ defmodule Forge.Ast.Module do
 
     {type, split_module}
   end
+
+  @spec to_atom(module() | String.t()) :: module()
+  def to_atom(module) when is_atom(module) do
+    module
+  end
+
+  def to_atom(":" <> module_string) do
+    String.to_existing_atom(module_string)
+  end
+
+  def to_atom(module_string) when is_binary(module_string) do
+    case do_safe_split("Elixir." <> module_string) do
+      {:erlang, [module]} -> String.to_existing_atom(module)
+      {:elixir, parts} -> Module.concat(parts)
+    end
+  end
 end