Skip to content

Oddities related to quoted imports #13878

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

Closed
lukaszsamson opened this issue Oct 3, 2024 · 11 comments
Closed

Oddities related to quoted imports #13878

lukaszsamson opened this issue Oct 3, 2024 · 11 comments

Comments

@lukaszsamson
Copy link
Contributor

lukaszsamson commented Oct 3, 2024

Elixir and Erlang/OTP versions

Erlang/OTP 26 [erts-14.2.5.3] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]

Elixir 1.17.3 (compiled with Erlang/OTP 26)

Operating system

any

Current behavior

I noticed some strange behavior related to quoted imports. Given this compilation tracer

defmodule MyTracer do
  def trace(event, _env) do
    dbg(event)
    :ok
  end
end
Code.put_compiler_option(:tracers, [MyTracer])

when I compile this code

defmodule MyModule do
      def aaa, do: :ok
      defmacro bbb, do: :ok
      defmacro foo do
          quote do
              aaa()
              &aaa/0
              bbb()
              &bbb/0
              inspect(1)
              &inspect/1
              Node.list()
              &Node.list/0
          end
      end
end

The emitted traces are

[iex:7: MyTracer.trace/2]
event #=> :defmodule

[iex:7: MyTracer.trace/2]
event #=> {:imported_macro, [line: 2, column: 3], Kernel, :defmacro, 2}

[iex:7: MyTracer.trace/2]
event #=> {:remote_function, [line: 2], :elixir_def, :store_definition, 3}

[iex:7: MyTracer.trace/2]
event #=> {:remote_function, [line: 1], :elixir_utils, :noop, 0}

[iex:7: MyTracer.trace/2]
event #=> {:imported_function, [line: 5, column: 7], Kernel, :inspect, 1}

[iex:7: MyTracer.trace/2]
event #=> {:imported_quoted, [line: 5, column: 15], Kernel, :/, [2]}

[iex:7: MyTracer.trace/2]
event #=> {:imported_quoted, [line: 5, column: 8], Kernel, :inspect, [1, 2]}

[iex:7: MyTracer.trace/2]
event #=> {:imported_quoted, [line: 4, column: 7], Kernel, :inspect, [1, 2]}

[iex:7: MyTracer.trace/2]
event #=> {:on_module,
 <<70, 79, 82, 49, 0, 0, 6, 16, 66, 69, 65, 77, 65, 116, 85, 56, 0, 0, 0, 176,
   0, 0, 0, 17, 15, 69, 108, 105, 120, 105, 114, 46, 78, 121, 77, 111, 100, 117,
   108, 101, 8, 95, 95, 105, 110, 102, 111, 95, ...>>, :none}

[iex:7: MyTracer.trace/2]
event #=> :stop
  1. There are undocumented trace events :imported_quoted
  2. Only &inspect/1 results in a trace event. No similar events for remote and local captures
  3. No events for calls

Next, when I eval the macro, it outputs this AST

apply(MyModule, :"MACRO-foo", [__ENV__])
{:__block__, [],
 [
   {:aaa, [], []},
   {:&, [],
    [
      {:/, [context: MyModule, imports: [{2, Kernel}]],
       [{:aaa, [], MyModule}, 0]}
    ]},
   {:bbb, [], []},
   {:&, [],
    [
      {:/, [context: MyModule, imports: [{2, Kernel}]],
       [{:bbb, [], MyModule}, 0]}
    ]},
   {:inspect, [context: MyModule, imports: [{1, Kernel}, {2, Kernel}]], [1]},
   {:&, [imports: [{1, Kernel}], context: MyModule],
    [
      {:/, [context: MyModule, imports: [{2, Kernel}]],
       [
         {:inspect, [context: MyModule, imports: [{1, Kernel}, {2, Kernel}]],
          MyModule},
         1
       ]}
    ]},
   {{:., [], [{:__aliases__, [alias: false], [:Node]}, :list]}, [], []},
   {:&, [],
    [
      {:/, [context: MyModule, imports: [{2, Kernel}]],
       [
         {{:., [], [{:__aliases__, [alias: false], [:Node]}, :list]},
          [no_parens: true], []},
         1
       ]}
    ]}
 ]}
  1. Why is Kernel added twice in meta imports: [{1, Kernel}, {2, Kernel}]?

Expected behavior

  1. Trace event should be documented. Code documentation does not mention any private events and suggests the list given there is exhaustive

I'm not sure if emitting {:imported_quoted, [line: 5, column: 15], Kernel, :/, [2]} should happen

  1. Either all captures should emit events, none should emit events, or if import is special then it should be documented
  2. Call probably should emit events

Alternatively it's possible that events in quoted are not needed at all

  1. This looks strange but I don't understand the code in :elixir_quote so it may be correct
@sabiwara
Copy link
Contributor

sabiwara commented Oct 4, 2024

For reference, the context for the change introducing :imported_quoted: #11651

@josevalim
Copy link
Member

We only emit import quoted if the quote is potentially capturing an import, which is effectively when we may have a dependency. Everything else gets resolved when the result of the quote is expanded instead. We will document it and I will double check if imported_quoted is required for /, but most likely yes, as the trace implies it may be used, not that it will.

@lukaszsamson
Copy link
Contributor Author

I did one more experiment. This time I compared events emitted by expanding a macro vs events emitted by the exact same code not quoted.

defmodule MyModule do
          def aaa, do: :ok
          defmacro bbb, do: :ok
          defmacro foo do
            quote do
              aaa()
              &aaa/0
              bbb()
              &bbb/0
              inspect(1)
              &inspect/1
              Node.list()
              &Node.list/0
            end
          end

          def go do
            foo()
          end

          def bar do
            aaa()
            &aaa/0
            bbb()
            &bbb/0
            inspect(1)
            &inspect/1
            Node.list()
            &Node.list/0
          end
        end

Grouped for readability and omitted not interesting bits

Expanding foo macro in go function body

[iex:3: MyTracer.trace/2]
event #=> {:local_macro, [line: 18, column: 13], :foo, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_function, [line: 18], :aaa, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_function, [line: 18, counter: {MyModule, 6}], :aaa, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_macro, [line: 18], :bbb, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_macro, [line: 18, counter: {MyModule, 6}], :bbb, 0}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function,
 [line: 18, context: MyModule, imports: [{1, Kernel}, {2, Kernel}]], Kernel,
 :inspect, 1}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function,
 [
   line: 18,
   counter: {MyModule, 6},
   context: MyModule,
   imports: [{1, Kernel}, {2, Kernel}]
 ], Kernel, :inspect, 1}

[iex:3: MyTracer.trace/2]
event #=> {:alias_reference, [line: 18, counter: {MyModule, 6}, alias: false], Node}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function, [line: 18], Node, :list, 0}

[iex:3: MyTracer.trace/2]
event #=> {:alias_reference, [line: 18, counter: {MyModule, 6}, alias: false], Node}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function, [line: 18, no_parens: true], Node, :list, 0}

Directly calling the same code in bar body:

[iex:3: MyTracer.trace/2]
event #=> {:local_function, [line: 22, column: 13], :aaa, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_function, [line: 23, column: 14], :aaa, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_macro, [line: 24, column: 13], :bbb, 0}

[iex:3: MyTracer.trace/2]
event #=> {:local_macro, [line: 25, column: 14], :bbb, 0}

[iex:3: MyTracer.trace/2]
event #=> {:imported_function, [line: 26, column: 13], Kernel, :inspect, 1}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function, [line: 26, column: 13], Kernel, :inspect, 1}

[iex:3: MyTracer.trace/2]
event #=> {:imported_function, [line: 27, column: 14], Kernel, :inspect, 1}

[iex:3: MyTracer.trace/2]
event #=> {:alias_reference, [line: 28, column: 13], Node}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function, [line: 28, column: 18], Node, :list, 0}

[iex:3: MyTracer.trace/2]
event #=> {:alias_reference, [line: 29, column: 14], Node}

[iex:3: MyTracer.trace/2]
event #=> {:remote_function, [no_parens: true, line: 29, column: 19], Node, :list, 0}

Notice no imported_function emitted from macro expanded code for bot capture and call.

Additionally, I'd say that no_parens: true is unexpected on capture

@josevalim
Copy link
Member

Notice no imported_function emitted from macro expanded code for bot capture and call.

Exactly. The import is expanded on the module/function that does the quote, so where the quote is expanded, it can only be a remote call (or a local call if the function is locally overridden). 👍

Additionally, I'd say that no_parens: true is unexpected on capture

Remember that Elixir AST is generic. &Node.list/0 is not different from +Some.mod/0 (besides operator precedence). So if Some.mod gets no_parens, Node.list should get it too.

@josevalim
Copy link
Member

josevalim commented Oct 6, 2024

There are undocumented trace events :imported_quoted

Fixed in main.

Only &inspect/1 results in a trace event. No similar events for remote and local captures

We expand only imports and aliases inside quote, because that's the information we need to send to the expansion. The alias is a regular alias event, however. The import one is special because we need to gather all arities.

No events for calls

Per above, expected.

Why is Kernel added twice in meta imports: [{1, Kernel}, {2, Kernel}]?

We store all arity-module pairs for a given name. In this case, both arities come from Kernel, but they could come from different functions.


Thank you for the excellent questions and helping us document this more clearly.

@lukaszsamson
Copy link
Contributor Author

Thanks for the detailed answers @josevalim. Now I understand more on how macro hygiene works.

@lukaszsamson
Copy link
Contributor Author

@josevalim I revisited this and I'm convinced there is an issue with quoted import calls. Consider this simple example:
As you described, the quote generates only imported_quoted event

Code.compile_string("defmodule QuotedCalls do; defmacro foo do; quote do; inspect(1); end; end; end")
[iex:12: MyTracer.trace/2]
event #=> {:imported_quoted, [line: 1, column: 54], Kernel, :inspect, [1, 2]}

Yet when I expand this macro I don't get any trace events

Code.compile_string("defmodule MyModule do; require QuotedCalls, as: Q; def aasa, do: Q.foo(); end")
[iex:12: MyTracer.trace/2]
event #=> {:remote_macro, [line: 1, column: 68], QuotedCalls, :foo, 0}

Per your comment "so where the quote is expanded, it can only be a remote call" I'd expect to get remote_function event.

I think the issue is somewhere in do_expand_import branch handing {import, Receiver} in

{import, Receiver} ->
. Trace event is only emitted in expand_require for remote_macro. For function {function, Receiver, Name} is returned and dispatch callback is executed without emitting anything.

@lukaszsamson
Copy link
Contributor Author

Nice. I still have two questions:

  1. I'm curious why there is no elixir_import:record({Name, Arity}, Receiver, Module, ?key(E, function)) call in {import, Receiver} case
  2. Under what conditions is the quoted import call expanded to a local?

The import is expanded on the module/function that does the quote, so where the quote is expanded, it can only be a remote call (or a local call if the function is locally overridden)

@josevalim
Copy link
Member

  1. elixir_import is there to detect conflicts when an import Foo defined in the module itself conflicts with local but in this case the import was resolved externally. I think the fact we didn't (and don't) need this call was the cause for the bug in the first place :)

  2. It can only happen if the import was not expanded. The "precedence" of the quoted sentence should be read like this:

(The import is expanded on the module/function that does the quote, so where the quote is expanded, it can only be a remote call) or (a local call if the function is locally overridden)

@lukaszsamson
Copy link
Contributor Author

The "precedence" of the quoted sentence should be read like this...

The meaning of overridden was not clear. Do I get it right that this means the import is not resolved at quote time and resolved to a local on expand like in this example

defmodule Kernel.LexicalTrackerTest.QuotedFun do
   defmacro parse_root() do
      quote do
          parse("/")
      end
   end
end

defmodule Kernel.LexicalTrackerTest.UsingQuotedFun do
  require Kernel.LexicalTrackerTest.QuotedFun, as: QF

  def parse(_), do: :bar

  def foo, do: QF.parse_root()
end

@josevalim
Copy link
Member

josevalim commented Apr 8, 2025

Your example above is resolved locally. However, if parse/1 inside the quote came from an import, then it won't be resolved locally. This is to reduce coupling. Otherwise functions defined locally would override quoted expressions by accident.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants