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

Remote Intellisense #2217

Merged

Conversation

cristineguadelupe
Copy link
Contributor

Initial intellisense support for remote nodes
Remote modules with its docs and its exported functions with its docs

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Uffizzi Preview deployment-36494 was deleted.

@@ -19,11 +19,12 @@ defmodule Livebook.Intellisense.SignatureMatcher do
{:ok, list(signature_info()), active_argument :: non_neg_integer()} | :error
def get_matching_signatures(hint, intellisense_context) do
%{env: env} = intellisense_context
node = intellisense_context.node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass the node as an option here too. intellisense_context comes from the node that does evaluation, and for the remote cell that's not the node we want to use.

Comment on lines 50 to 57
File.rm_rf!(@tmp_dir)
File.mkdir_p!(@tmp_dir)
File.write!("#{@tmp_dir}/Elixir.RemoteModule.beam", bytecode)

true = :erpc.call(node, :code, :set_path, [:code.get_path() ++ [~c"#{@tmp_dir}"]])
{:ok, _} = :erpc.call(node, :application, :ensure_all_started, [:elixir])
{:module, RemoteModule} = :erpc.call(node, :code, :load_file, [:"Elixir.RemoteModule"])
:loaded = :erpc.call(node, :code, :module_status, [:"Elixir.RemoteModule"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify we could use Livebook.Runtime.ElixirStandalone. Start a runtime, evaluate code that defines the module, and then use runtime.node.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea but we can end-up accidentally polluting the node in tests and not find bugs. So I would try to keep the remote node as "clean" as possible, if that makes any sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other tests run in the context of Livebook itself, so there is definitely much more polluted in that sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? To be clear, what I mean is that, if the remote node has Livebook's runtime, then we can accidentally do :erpc.call(node(), SomeLivebookModule) which will not be available in actual remote nodes, only in tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

That when testing completion all the livebook modules could be listed, we just have the tests such that this is never the case.

then we can accidentally do :erpc.call(node(), SomeLivebookModule)

I see, we are clearly just fetching docs info so I'm not worried about that, but that's a fair point, we can keep the minimal version.

}
end
end

setup_all do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to the describe block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can have a setup_all inside the describe block. Therefore my proposal is to keep these tests in a separate file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do a separate module in the same file just so it's still grouped together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate module or separate file are both fine by me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it back to a separate file

members = MapSet.new(members)
kinds = opts[:kinds] || [:function, :macro, :type]

specs =
with true <- :function in kinds or :macro in kinds,
{:ok, specs} <- Code.Typespec.fetch_specs(module) do
{:ok, specs} <- :erpc.call(node, :"Elixir.Code.Typespec", :fetch_specs, [module]) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{:ok, specs} <- :erpc.call(node, :"Elixir.Code.Typespec", :fetch_specs, [module]) do
{:ok, specs} <- :erpc.call(node, Code.Typespec, :fetch_specs, [module]) do

case Code.fetch_docs(module) do
@spec get_module_documentation(module(), node()) :: documentation()
def get_module_documentation(module, node) do
case :erpc.call(node, :"Elixir.Code", :fetch_docs, [module]) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case :erpc.call(node, :"Elixir.Code", :fetch_docs, [module]) do
case :erpc.call(node, Code, :fetch_docs, [module]) do

Map.new(specs)
else
_ -> %{}
end

type_specs =
with true <- :type in kinds,
{:ok, types} <- Code.Typespec.fetch_types(module) do
{:ok, types} <- :erpc.call(node, :"Elixir.Code.Typespec", :fetch_types, [module]) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{:ok, types} <- :erpc.call(node, :"Elixir.Code.Typespec", :fetch_types, [module]) do
{:ok, types} <- :erpc.call(node, Code.Typespec, :fetch_types, [module]) do

@@ -99,7 +100,7 @@ defmodule Livebook.Intellisense.Docs do
_ -> %{}
end

case Code.fetch_docs(module) do
case :erpc.call(node, :"Elixir.Code", :fetch_docs, [module]) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case :erpc.call(node, :"Elixir.Code", :fetch_docs, [module]) do
case :erpc.call(node, Code, :fetch_docs, [module]) do

funs = funs || exports(mod)
node = ctx.intellisense_context.node

if :erpc.call(node, :"Elixir.Code", :ensure_loaded?, [mod]) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if :erpc.call(node, :"Elixir.Code", :ensure_loaded?, [mod]) do
if :erpc.call(node, Code, :ensure_loaded?, [mod]) do

Comment on lines 695 to 696
loaded = :erpc.call(node, :"Elixir.Code", :ensure_loaded?, [mod])
exported = :erpc.call(node, :"Elixir.Kernel", :function_exported?, [mod, :__info__, 1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
loaded = :erpc.call(node, :"Elixir.Code", :ensure_loaded?, [mod])
exported = :erpc.call(node, :"Elixir.Kernel", :function_exported?, [mod, :__info__, 1])
loaded = :erpc.call(node, Code, :ensure_loaded?, [mod])
exported = :erpc.call(node, :erlang, :function_exported, [mod, :__info__, 1])

File.mkdir_p!(@tmp_dir)
File.write!("#{@tmp_dir}/Elixir.RemoteModule.beam", bytecode)

true = :erpc.call(node, :code, :set_path, [:code.get_path() ++ [~c"#{@tmp_dir}"]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
true = :erpc.call(node, :code, :set_path, [:code.get_path() ++ [~c"#{@tmp_dir}"]])
true = :erpc.call(node, :code, :set_path, [[~c"#{@tmp_dir}" | :code.get_path()]])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let's prune this path a bit:

Suggested change
true = :erpc.call(node, :code, :set_path, [:code.get_path() ++ [~c"#{@tmp_dir}"]])
build_path = Mix.Project.build_path() |> String.to_charlist()
paths = Enum.reject(:code.get_path, &List.starts_with?(&1, build_path))
true = :erpc.call(node, :code, :set_path, [[~c"#{@tmp_dir}" | paths]])

This will make sure the node does not have any of our deps either!

@cristineguadelupe cristineguadelupe merged commit 76139d7 into livebook-dev:main Sep 21, 2023
7 checks passed
@cristineguadelupe cristineguadelupe deleted the cg-remote-intellisense branch September 21, 2023 17:05
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