Macro lookup from included module broken

[straight-shoota]

In 0.23 the following code does no longer compile, in 0.22 it did. It might been caused by #236

module Include
  macro include_macro
  end
end
class Foo
  include Include

  include_macro                   # => undefined local variable or method 'include_macro'
end

I am not sure if this behaviour is intended, at least it should be mentioned in the release notes as breaking change.

This makes macro lookup behave like class method lookup in the same context of an included, but different from a parent class:

module Include
  def self.include_method
  end
  macro include_macro
  end
end
class Parent
  def self.parent_method
  end
  macro parent_macro
  end
end

class Foo < Parent
  include Include
  
  parent_method                   # => ok
  parent_macro                    # => ok
  include_method                  # => undefined local variable or method 'include_method'
  include_macro                   # => undefined local variable or method 'include_macro'
end

I would actually prefer to be able to call macros defined in modules from the including class scope and don't think they should be different from macros defined in a parent class. I think it is a common use case to define helper macros in a module and call these macros from class scope where this module is included. Now you need the extra verbosity of Include.include_macro and it is not possible to refer to the including type context, because with the explicit receiver, @type points to the module, not the including class. So I would consider this a major breaking change - and probably a bug?

[mverzilli]

Good catch, we need to investigate this soon. So the first suspect is this: c87fc6d

[matiasgarciaisaia]

Fixed via #4649 - thanks @asterite 👍

0.23.1 is coming (?)

[eliasjpr]

Just wanted to state that Amber Framework is also breaking because of this behavior

[straight-shoota]

Unfortunately, this is not entirely resolved. For some reason the method lookup seems to be confused when an instance method of the same name is defined by a macro from an included module and the macro has already been called from the parent class. The following code results in undefined local variable or method 'foo' in the second-to-last line.

module Include
  macro foo
    def foo 
    end
  end
end

class Parent
  include Include

  foo 
end

class Foo < Parent
  foo
end

Seems a bit strange, but it fails under exactly these conditions. Calling foo macro twice inside the same class works fine.

It works if Include is included directly to Foo.

/cc @asterite @matiasgarciaisaia

[matiasgarciaisaia]

@straight-shoota I'm not really into the details of this, but may it have to do something with foo being both a macro and the method being defined?

This other program works:

module Include
  macro foo
    def bar
      puts "Hi there!"
    end
  end
end

class Parent
  include Include

  foo 
end

class Foo < Parent
  foo
end

Foo.new.bar

[straight-shoota]

Yes, this is a precondition for the improved macro lookup to fail. And it only happens if the macro has been invoked in a superclass (remove foo from Parent in my example and it works). I have no clue why.

Btw. The failure is not depending on class hierarchy, it also errors if Parent is a module. But this would probably be kind of expectable...

[matiasgarciaisaia]

Sorry - I've forgot you explicitly stated the precondition on your message 👍

I think we don't care about this case that much. I don't see why you'd want to have a macro and a method with the same name.

If you think this is really an issue, I think the best way would be to open a new issue and state some real use case for that, and we can consider it for a later version. For now, I think the issue is solved enough.

[straight-shoota]

I don't think this is kind of a feature request where you would describe a use case to advocate the issue, but it is clearly a bug where otherwise valid behavior breaks under very specific conditions. If macros and instance methods generally can share the same names without collision (which is the current state and makes absolute sense because they have very different scopes) this should work in any case and don't suddenly fail on arbitrary preconditions that might not even be too rare.

A use case is a macro that acts as a generator for a synonymous method - it makes no sense to use different names. A real world example is this method (simplified from here):

macro getattr(*whitelist)
  def getattr(attr_name)
    {% for method in whitelist %}
      if {{ method.id.stringify }} == attr_name
        return self.{{ method.id }}
      end
    {% end %}
  end
end
# in a class that inherits this macro:
class UseCase
  getter foo, bar
  getattr foo, bar   # => defines instance method `getattr` to access `#foo` and `#bar`
end

If a another class inherits from UseCase it cannot call macro getattr to override UseCase#getattr.

[mverzilli]

@straight-shoota I understand that it may feel arbitrary, but there are probably more pressing needs now and with @asterite's fix this became just a corner case issue. We can leave this open to not lose track of the issue and revisit eventually, but we won't hold on 0.23.1 for this.

To me that code looks a bit confusing, but in any case you could rename the macro to def_get_attr and get the same things done (that's what @matiasgarciaisaia meant by "let's talk use cases").

class UseCase
  getter foo, bar
  def_get_attr foo, bar   # => defines instance method `get_attr` to access `#foo` and `#bar`
end

Btw, took the liberty to rename getattr to get_attr to make it honor Crystal conventions :).

[straight-shoota]

Sure, I didn't want to push this for 0.23.1 but keep it on the list ;) There are plenty of workarounds, easy alternatives without renaming anything are calling the macro directly as Include.foo or including Include explicit into the child class.
But it is still a bug ;) Thanks for reopening!

[straight-shoota]

This was closed when @matiasgarciaisaia merged 0.23.1 branch into master (#4791) but this issue was not entirely resolved, so it should stay open. I could open a new issue if this would be preferred, but it feels like the same problem and was only partially fixed.

[matiasgarciaisaia]

True! Thanks for pointing out! :)