Support a configurable language module#21
Conversation
Erlang has a constraint that ensures there is a mapping from filename to module name: ``` foo.erl => -module(foo) bar.erl => -module(bar) ``` This makes looking up a module based on a path name deterministic. This is not the case in other BEAM languages such as Elixir. In Elixir, there is no mapping between the file name and the module name. It is also possible for an Elixir file to contain multiple modules. In order to allow edb to be used in Elixir, we need a way to configure the mapping from path to module. However, instead of baking in support just for Elixir, it would be ideal to have this be generic. This commit introduces a configurable module which can be provided as start options for edb. This is implemented with a behaviour, and a default erlang implementation is provided which is just the code that previously existed. The behaviour implements `init/0` and `source_to_modules/3`. To allow passing this option through, the application config is required to be set before starting edb. This is should be handled by the caller. The intention here, is for other languages to wrap edb with their own escript which provides a language implementation. For example, an Elixir wrapper can use: ``` defmodule EdbElixir.CLI do @moduledoc false def main(args) do Application.put_env(:edb, :dap_language, EdbElixir.DapLanguage) :edb_main.main(args) end end ```
jcpetruzza
left a comment
jcpetruzza
left a comment
There was a problem hiding this comment.
Sorry it too me a bit long to look into this. It looks good. If we can move the language-callback handling outside of the main server, I think it would be much cleaner. Wdyt?
|
|
||
| -type state() :: dynamic(). | ||
|
|
||
| -callback init() -> dynamic(). |
There was a problem hiding this comment.
return type should be state()
| Path :: binary(), | ||
| Lines :: [edb:line()], | ||
| Modules :: [module()], | ||
| State :: dynamic(). |
| -type state() :: dynamic(). | ||
|
|
||
| -callback init() -> dynamic(). | ||
| -callback source_to_modules(Path, Lines, State) -> {Modules, State} when |
There was a problem hiding this comment.
Even though the types are clear, could we add short docs about what this function is supposed to return?
| -type state() :: dynamic(). | ||
|
|
||
| -callback init() -> dynamic(). | ||
| -callback source_to_modules(Path, Lines, State) -> {Modules, State} when |
There was a problem hiding this comment.
In case of errors, the callback can only ever return [], and we can't distinguish things like "the callback thinks this source doesn't correspond to any elixir module" from "there is something misconfigured and we couldn't check".
Also, on an [] response, the DAP server can only that no breakpoint was set, but can't tell the user why.
All this to say, maybe the return type should be something like {ok,[Module]} | {error, Reason} , with Reason a binary(), or maybe a term() plus a format_error() callback
| -behaviour(edb_dap_language). | ||
|
|
||
| -export([init/0, source_to_modules/3]). | ||
|
|
There was a problem hiding this comment.
nit: define a state() type and use that in specs instead of #{}
| Path :: binary(), | ||
| Lines :: [edb:line()], | ||
| Modules :: [module()], | ||
| State :: dynamic(). |
There was a problem hiding this comment.
Thinking out loud: could we define also an implementation for each callback, that uses application:get_env() to do the dispatching? Something like
-spec init() -> state().
init() ->
Impl = impl(),
Imp:init().
...
-spec impl() -> module().
impl() ->
application:get_env(edb, dap_language, edb_dap_language_erlang).
Then we no longer need to store the module in the dap state, and we can just write:
{Modules, LangState1} = edb_dap_language:source_to_modules(Source, LangState0),
which means eqwalizer will still type-check the calls (instead of using dynamic() for everything), we can still use code-navigation to find the callback defs, etc.
Actually, if we make edb_dap_language a gen_server, we can also store the language state in the gen_server state and avoid having to thread it through the dap server state, which will make the code more readable, I think
| edb:set_breakpoints(Module, Lines); | ||
| set_breakpoints_in_modules(Modules, Lines) -> | ||
| ResultsByModule = [edb:set_breakpoints(Module, Lines) || Module <- Modules], | ||
| [{Line, merge_line_results(Line, ResultsByModule)} || Line <- Lines]. |
There was a problem hiding this comment.
This looks quadratic? Could we instead convert each result-by-module into a map #{Line => Result}, then use maps:merge_with() to accumulate, and finally flatten or something like that?
2 participants
Erlang has a constraint that ensures there is a mapping from filename to module name:
This makes looking up a module based on a path name deterministic. This is not the case in other BEAM languages such as Elixir. In Elixir, there is no mapping between the file name and the module name. It is also possible for an Elixir file to contain multiple modules.
In order to allow edb to be used in Elixir, we need a way to configure the mapping from path to module. However, instead of baking in support just for Elixir, it would be ideal to have this be generic.
This commit introduces a configurable module which can be provided as start options for edb. This is implemented with a behaviour, and a default erlang implementation is provided which is just the code that previously existed. The behaviour implements
init/0andsource_to_modules/3.To allow passing this option through, the application config is required to be set before starting edb. This is should be handled by the caller.
The intention here, is for other languages to wrap edb with their own escript which provides a language implementation.
For example, an Elixir wrapper can use: