Remove recursion with method_added

[bew]

This code used to work (I don't remember the version and I didn't managed to bisect that properly with versions before 0.21.0):

module ModuleName
  macro method_added(meth)
    {% puts "Method #{meth.name} added" %}
    {{meth}}
  end

  # triggers the hook
  def normal_method
  end
end

It used to be called once, display the proper message, and defines the method.
(In fact this is a bad example, see #5066 (comment) for a better one!)

Now I get:

"Method normal_method added"
"Method normal_method added"
"Method normal_method added"
"Method normal_method added"
...

Followed by a stackoverflow error...

This is because the macro is expanded to:

 
def normal_method
end

and during the interpretation of the macro expansion, the macro method_added is called (again) for the generated method.

I think that methods defined by a method_added expansion should not trigger method_added again, but methods defined in other places (normal code or other macros) should trigger it.

[oprypin]

have you tried different versions on https://carc.in ?

[bew]

@oprypin good idea! And the result is weird:

For 0.18.0 -> 0.21.1: it recurses, the compiler crash, and the output is visible on carc.in.
For 0.22.0 -> 0.23.1: it surely recurses then crash, but carc.in crashes too (error 504), and the view is back to the editor (not showing the result).

I keep a repository with cool crystal snippets, and I added this one 5 weeks ago (29 aug. 2017). The thing is I'm almost sure it worked when I added it, as I almost never add non-working snippets to the repo..

Either way, I think it should not recurse like this for methods defined by the expansion of the method_added macro. WDYT?

[Sija]

Isn't the {{meth}} at the end of the macro method_added causing the recursion?

def foo; end # => method_added # => def foo; end # => method_added # ...

[bew]

@Sija yes it is. The (small) purpose of this snippet is to write a message, and then actually define the method, so it can be called later.

I've update the original post, hopefully its more clear

[straight-shoota]

Wow I didn't know about method_added... there are to many awesome but undocumented features in Crystal =)

But why would you define the method again inside the macro? As far as I understand the code and examples, method_added is invoked after the method has been defined. So your {{ meth }} just defines the method again (and again and again ...). You just don't need it. And I doubt this has ever worked without recursion judging by the introductory code of this feature from #1090.

[Sija]

@straight-shoota yep, that was exactly my point ;) TIL the same 🎉

[bew]

yeah I know this example is dumb as hell, maybe a more useful one would be:

module Tracer
  macro included
    macro method_added(def_node)
      def \{{def_node.name}}(\{{def_node.args.splat}})
        puts "Entering method \{{def_node.name}}"

        previous_def

        puts "Leaving method \{{def_node.name}}"
      end
    end
  end
end

class MyClass
  include Tracer

  def foo
    puts "Hello from foo"
  end

  def bar(*args)
    puts "There are #{args.size} args!"
  end
end


MyClass.new.foo
puts
MyClass.new.bar 1, 2, 3

Should output:

Entering method foo
Hello from foo
Leaving method foo

Entering method bar
There are 3 args!
Leaving method bar

Here, each method definition in MyClass will call the method_added hook (coming from the inclusion of Tracer), and after the hook ran, the generated def will also call method_added hook (which is clearly not needed!). This is what I want to avoid.

[straight-shoota]

Yes, I figured there would be a more reasonable example.
Just a note: You could probably use previous_def instead of expanding the body in the macro.

I think it's safe to assume that methods defined in the method_added macro should never trigger that hook itself.

[bew]

You could probably use previous_def instead of expanding the body in the macro.

Oh yes, you're right!! I always thought the method wasn't defined and it was the role of the macro to define it... (weird thinking, I agree..) silly me, I should have read the specs!
(I updated my example)

[straight-shoota]

Well, the specs actually neither show nor test that. At least it's not obvious to me. They seem to just spec that the macro is invoked, not that the method has been defined at that point.

[bew]

The specs probably don't test it because it was obvious IMO. Nevermind, now I know 😄

[makenowjust]

@straight-shoota Sorry I missed to write document for this feature.

@bew

When I implement this feature, I thought recursion with method_added is no problem because we can mutate some values on compile-time, for example:

module Tracer
  macro included
    WRAPPED = [] of String

    macro method_added(def_node)
      \{% if !WRAPPED.includes? def_node.name %}
        \{% WRAPPED.push def_node.name %}

        def \{{def_node.name}}(\{{def_node.args.splat}})
          puts "Entering method \{{def_node.name}}"

          previous_def

          puts "Leaving method \{{def_node.name}}"
        end
      \{% end %}
    end
  end
end

class MyClass
  include Tracer

  def foo
    puts "Hello from foo"
  end

  def bar(*args)
    puts "There are #{args.size} args!"
  end
end


MyClass.new.foo
puts
MyClass.new.bar 1, 2, 3

Above example works intentionally (MyClass.new.bar 1, 2, 3 does not work, but I think it is another issue). However, I think now this recursion has no usecase, so I'd fix not to invoke method_added when method is defined inside method_added.

[bew]

When I implement this feature, I thought recursion with method_added is no problem because we can mutate some values on compile-time

Yes I figured this out while trying to make it work! (I just pushed the whole node into the array, not just its name ˆˆ)

Thank you for looking into that @makenowjust!

[straight-shoota]

@makenowjust don't feel sorry. It after all was (and probably still is?) an experimental feature. And your PR got accepted without a proper documentation or spec, so not your fault if it is anyone's ;)