Skip to content

Add local_for_callback option to Macro.Env.expand_import #14620

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lukaszsamson
Copy link
Contributor

@lukaszsamson lukaszsamson commented Jul 3, 2025

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

end

case :elixir_dispatch.expand_import(meta, name, arity, env, extra, allow_locals, trace) do
case :elixir_dispatch.expand_import(meta, name, arity, env, extra, allow_locals, trace, local_for_callback) do
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@josevalim
Copy link
Member

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.

@lukaszsamson lukaszsamson force-pushed the ls-expand-import-option branch from a1349a8 to 997eb4c Compare July 4, 2025 12:12
@lukaszsamson
Copy link
Contributor Author

@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
Copy link
Member

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

@lukaszsamson lukaszsamson marked this pull request as ready for review July 4, 2025 16:17
- When `true`, uses a default resolver that looks for public macros in
the current module
- When a function, uses the function as a custom local resolver. The function
must have the signature: `(meta, name, arity, kinds, env) -> function() | false`
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need the kinds, do we? I think it can be hidden from the API so we keep this as a 4 arity function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could hide kinds and assume it's a macro resolver.

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 do that, since it is for expansion (so it has to be macros)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -96,7 +96,7 @@ defmodule Macro.Env do
]

@type expand_import_opts :: [
allow_locals: boolean(),
allow_locals: boolean() | (Macro.metadata(), atom(), arity(), [atom()], t() -> any()),
Copy link
Member

Choose a reason for hiding this comment

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

Do we return any or does it have to be AST?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the return type is false | function()

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so we should update it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants