Add local_for_callback option to Macro.Env.expand_import

[lukaszsamson]

This PR adds a new option :local_for_callback to Macro.Env.expand_import in order to allow hooking into local macro resolution and providing a custom resolver.

Rationale:
In ElixirSense minicompiler I need to resolve local macros using the metadata extracted from the AST. The current mechanism relying on :elixir_def.local_for is not going to work correctly when there is no real compilation in process. The current hacky code using extra filled with __info__(:macros) only returns public macros and with that only those already compiled and loadable. The result is improper tracking of references in traversed code.

Example code using the new option:

case Macro.Env.expand_import(env, meta, fun, arity,
  trace: true,
  allow_locals: allow_locals,
  check_deprecations: false,
  local_for_callback: fn meta, name, arity, kinds, e ->
    # a custom local resolver that uses mods_funs_to_positions def dictionary
    case state.mods_funs_to_positions[{e.module, name, arity}] do
      nil ->
        # no info found - treat as reference to a not existing local
        false

      %ModFunInfo{} = info ->
        category = ModFunInfo.get_category(info)
        definition_line = info.positions |> List.first() |> elem(0)
        usage_line = meta |> Keyword.get(:line)

        if ModFunInfo.get_def_kind(info) in kinds and
            (category != :macro or usage_line >= definition_line) do
          # found a local function or macro that is visible in the context
          if macro_exported?(e.module, name, arity) do
            # public macro found - return the expander
            proper_name = :"MACRO-#{name}"
            proper_arity = arity + 1
            Function.capture(e.module, proper_name, proper_arity)
          else
            # return a fake macro expander
            true
          end
        else
          # local not found - return false
          false
        end
    end
  end
) do
  {:macro, module, callback} ->
    expand_macro(meta, module, fun, args, callback, state, env)
  _ ->
    # omitted
end

The PR is based on the current code used in ElixirSense. I'm opening it as a draft. If there is will to merge this I'll work on tests.

Related PR hooking into define_import:
#13628

[josevalim]

Instead of adding a new option, we can augment allow_locals. If it is false, it is disabled, if it is true, it is the default implementation (elixir_locals), otherwise it is a custom function.

[lukaszsamson]

Done on the erlang side. Do you also want that on the elixir API side?

[josevalim]

I think this could be moved inside the local_for_callback function, threfore we don't need to pass either extra or allow_locals as argument to elixir_dispatch.expand_import.

[lukaszsamson]

Would you like me to remove extra argument from the dispatch call and instead pass a resolver callback using module.__info__(:macros) or macro_exported??

[josevalim]

If it is possible, yes, it would be perfect.

[josevalim]

I think we should enforce it to either be Fun or false. What is the use case where you need it to be a separate value?

[lukaszsamson]

I needed a way of distinguishing between
false - local function or not found
function - expandable macro
truthy value - fake macro

The code in :elixir_dispatch relies on the result being truthy for local macros. Originally I wanted to return a dummy macro from my callback but that is not easy to do or at least I couldn't find an easy way. I would have to generate a function of a particular arity in runtime or pregenerate multiple arity variants at compile time and keep them in dictionary.

[josevalim]

What would you return in those cases? Could you instead return false and match on the result of the expand_import function returning not_found?

[lukaszsamson]

I mean that returning false changes the behavior of the caller code in

elixir/lib/elixir/src/elixir_dispatch.erl

Lines 77 to 84 in b29c83a

	case (Function /= nil) andalso (Function /= Tuple) andalso
	elixir_def:local_for(Meta, Name, Arity, [defmacro, defmacrop], E) of
	false ->
	elixir_env:trace({local_function, Meta, Name, Arity}, E),
	{local, Name, Arity};
	_ ->
	false
	end

the emitted trace is then local_function instead of local_macro
and in

elixir/lib/elixir/src/elixir_dispatch.erl

Lines 175 to 191 in b29c83a

	Local = AllowLocals andalso elixir_def:local_for(Meta, Name, Arity, [defmacro, defmacrop], E),
	case Dispatch of
	%% There is a local and an import. This is a conflict unless
	%% the receiver is the same as module (happens on bootstrap).
	{_, Receiver} when Local /= false, Receiver /= Module ->
	{conflict, Receiver};
	%% There is no local. Dispatch the import.
	_ when Local == false ->
	do_expand_import(Dispatch, Meta, Name, Arity, Module, E, Trace);
	%% Dispatch to the local.
	_ ->
	Trace andalso elixir_env:trace({local_macro, Meta, Name, Arity}, E),
	{macro, Module, expander_macro_fun(Meta, Local, Module, Name, E)}
	end

local_macro trace is not emitted and the code calls into do_expand_import

[josevalim]

I see. You can do this for now:

for arity <- 0..255 do
  defp ok_fun(unquote(arity)), do: fn unquote_splicing(Macro.generate_arguments(arity, __MODULE__)) -> :ok end
end

And that will generate the functions that you need. We use a similar trick in Nx, perhaps we should add something to the stdlib specifically for this.

[lukaszsamson]

OK, that hack works 👍

[josevalim]

We can definitely get this in. I have dropped some comments, some of them are about refactoring the current code, but I can also do that after merging.

[josevalim]

Let's not add a new option, let's augment allow_locals to be a function? Then we don't need to worry about how these two different APIs compose.

[lukaszsamson]

@josevalim I removed the Extra and added a resolver but with that I started to run into bootstrap issues and strange test failures like:

     error: unknown bitstring specifier: signed_16()
     │
 303 │       byte_size(sec_data)::size(1)-signed_16(),
     │                          ^
     │
     └─ test/elixir/kernel/binary_test.exs:303:26: Kernel.BinaryTest."test bitsyntax macro"/1

     error: unknown bitstring specifier: refb_spec()
     │
 301 │       byte_size(refb)::refb_spec(),
     │                      ^
     │
     └─ test/elixir/kernel/binary_test.exs:301:22: Kernel.BinaryTest."test bitsyntax macro"/1

[josevalim]

@lukaszsamson so please keep the extra for now. we can merge it and i will investigate it later :)