Broken compilation

[florius0]

When including Sibyl into other __using__/1 macros, comilation breaks in several ways:

• inline default arguments result in error ...defines defaults multiple times. Elixir allows defaults to be declared once per definition
• defoverridable/1 stops working.

Example:

defmodule A do
  @callback f() :: any()

  defmacro __using__(_opts) do
    quote do
      use Sibyl
      @decorate_all trace()

      @behaviour unquote(__MODULE__)

      @impl unquote(__MODULE__)
      def f, do: 1

      defoverridable unquote(__MODULE__)
    end    
  end
end

defmodule B do
  use A
  
  # Will fail to compile
  # def g(_a \\ nil), do: nil

  def f, do: 2
end

B.f() # => 1 instead of 2

PS defoverridable/1 can be fixed if use Sibyl... is declared after it. Perhaps decoration can be done as the last step of module compilation?
PPS looks like it's Decorator bug, not Sibyl's

[vereis]

Yup definitely a bug in decorator. If you check out the main branch atm you should have access to an Sibyl.Experimental module that you can use.

This replaces decorator with something admittedly grosser, but it circumvents this error.

It works by overriding the def macro so that we can define functions thusly:

def my_fun do
  ... sibyl code to emit event start ...
  super() # actually whatever was passed into the `def` macro, but its the same idea
after
  ... sibyl error event emission ...
catch
  ... sibyl catch event emission ...
end

I admit that I'm unsure if this works OOTB with defoverridable though.

[vereis]

Although now that I think about it... Sibyl.Experimental could instead:

1. inject a defoverridable clause for all public functions in the module
2. for all public functions inject my code block as above

I believe if you have defoverridable multiple times in a module it works as expected, super() usage works as expected, etc.

Might be a less invasive approach?

But then I'm not sure how defoverridable works in tandem with functions with multiple clauses plus default args...

[florius0]

It works by overriding the def macro so that we can define functions thusly

I would say that is actually more sane than decorator's approach

Thx, I'll try it out shortly

[florius0]

I was able to fix defoverridable/1 by doing this:

defmodule Helpers.Sibyl do
  defmacro __using__(:decorate_all) do
    quote do
      @before_compile unquote(__MODULE__)
    end
  end

  defmacro __before_compile__(_env) do
    quote do
      use Sibyl
      @decorate_all {Sibyl.Decorator, :trace, []}
    end
  end
end

note the expanded trace/0 in the attribute.

It works by overriding the def macro so that we can define functions thusly

It would fail to decorate custom def's.

Sibyl.Experimental could instead:

1. inject a defoverridable clause for all public functions in the module
2. for all public functions inject my code block as above
   I believe if you have defoverridable multiple times in a module it works as expected, super() usage works as expected, etc.

Might be a less invasive approach?

This approach might be better, but it would restrict its use to def/2 only.

Two alternative approaches were revealed to me (in a dream):

1. Patch all functions in compiled .beams directly, the drawback would be that we would essentially be operating in erlang-land, so no macros and custom defs.
2. Manipulate the module's ast directly via custom compiler (FIY elixir gives you the ability to add your own compilers), tho I'm not sure on how exactly it would be better than Decorator's approach, but sure it would be

[vereis]

Could you elaborate on what you mean by "custom def"?

[florius0]

e.g. the module I have in my project for defining runtime-configurable defdelegates

defmodule DI do
  defmacro __using__(opts) do
    quote location: :keep, generated: true do
      import unquote(__MODULE__)

      @app unquote(opts)[:app] || Mix.Project.config()[:app]
      @path unquote(opts)[:path] || [__MODULE__, :impl]

      def impl, do: Utils.Application.fetch_env!(@app, @path)

      defoverridable impl: 0
    end
  end

  defmacro defdi({name, _meta, args}) do
    vars =
      Macro.prewalk(args, fn
        {:\\, _meta, [var, _default]} -> var
        ast -> ast
      end)

    quote do
      def unquote({name, [line: __ENV__.line], args})
      def unquote({name, [line: __ENV__.line], vars}), do: impl().unquote(name)(unquote_splicing(vars))
    end
  end
end

[florius0]

I was able to fix defoverridable/1 by doing this:

UPD: it didn't work

[florius0]

Current iteration of Sibyl.Experimental doesn't work when used inside a macro (weird, IMO it should).

I managed to fix all of my problems with decorator tho.

1. ...defines defaults multiple times. Elixir allows defaults to be declared once per definition – turns out, @on_definition is being called multiple times with the exact same args, so doing Enum.uniq/1 fixes it.
2. defoverridable/1 stops working - and this happens bcs of how decorator works. It thinks that overrided function is a clause of previously defined one. I had to make assumption that clauses are always together. Potential bug is if it would instantly override the function after its definition, that's what Decorator.Separator is for.

# lib/decorator/decorate.ex
  defmacro before_compile(env) do
    decorated =
      env.module
      |> Module.get_attribute(:decorated)
      |> Enum.uniq()
      |> Enum.reverse()

PS I just realised that my fix would break if function has it's caluse defined in separate places, e.g.

defmodule A do
  defmacro __using__(_opts) do
    quote do
      def f(0), do: :zero
    end
  end
end

defmodule B do
  use Sibyl
  @decorate_all trace()

  use A

  def f(x), do: x
end

B.f(0) #=> :zero

Could be fixed later with defoverridable/1, but would require one to look up original clauses.

I'm in favour of this behaviour compared to existing one, which couldn't be fixed in user code at all

[florius0]

IMO we should mention this problem in readme.

Unrelated, but it would be nice:

1. To add option to decorator to ignore certain function for when using @decorate_all I wouldn't want to trace smth like __schema__/*. And I suspect it has impact on performance
2. To somehow cache and ideally pre-fill Sibyl.Event.reflect/*.

Maybe I'll implement both today.

UPD

2. To somehow cache and ideally pre-fill Sibyl.Event.reflect/*

Is not so necessary, since all modules in releases are loaded at startup, thus reducing Sibyl.Event.reflect/* time considerably