macro lookup conflicts with method lookup when including into toplevel

[jhass]

Now that we can include macros into the toplevel, it actually conflicts with method lookup:

class Foo
  def foo
    bar
  end

  private def bar
    p "method"
  end
end

module Bar
  macro bar
    p "macro"
  end
end

include Bar

Foo.new.foo

This prints macro. One could argue that it's correct behavior, although it also happens with using extend, where I'm more unsure if we want that behavior.

[asterite]

Macro lookup always conflicts with method lookup. A solution would be to have a different syntax for macro invocation (like Rust), but I'm not sure I like that.

Right now the macro/method lookup works like this:

1. Macros with that name are looked up in the call scope, going up the hierarchy. If a macro is found we are done.
2. Else, we try to type the call arguments and, once all types are known, methods matching those types are looked up.

The problem is, we don't want to type macro arguments because that would give false errors (property foo would say "undefined method foo").

One way to solve this would be to check wether there's a method with that name in the current scope, going up the hierarchy, regardless of types. If so it should resolve to a call, not to a macro.

I think this change would make sense because you'd want methods to be found before macros if a definition for them exists. What do you think?

I just tried it and it seems to work without a performance penalty.

[asterite]

@jhass by the way, why do you need to have a method and a macro with the same name? And why include that module with that macro at the top level?

[jhass]

It happened to me with the raise_error macro in my testing framework experiment. I tried moving it into a module and including this as needed, ideally only having it to include into Context and not into the toplevel. However for the moment I include it into the toplevel too so that it can be found in some of your helper methods who are defined at the toplevel. This then conflicted with the method of the same name in the Json::PullParser.

I guess preferring the method call make sense to reduce the damage one can make with such an action, even though we could escape the issue with calling self.bar and there's no such workaround to call the macro if the method wins, but I would expect that case to be a lot rarer.

[asterite]

What we can do for now is to change PullParser#raise_error to PullParser#parse_exception. If you run into this issue again we can rethink the problem.

By the way, the rescue e : T is really clever.

[jhass]

I had to work a bit with Java generics, you quickly realize that the type alias can be useful in more places than variable declarations ;)

[jhass]

Looking back at this, I think the real issue is that macro and method lookup are not interleaved. If we can make it so that the lookup is combined, in the example first looking for a macro bar inside Foo, then for a method bar inside Foo, then for a macro bar in the first ancestor, then for a method bar in the first ancestor, then for a macro bar in the second ancestor, then for a method bar int the second ancestor and so on all the way up, and then for a macro bar at the toplevel and then a method bar at the toplevel, it would result in the expected behaviour in at least 90% of all cases.

[sdogruyol]

What's the status on this?

[straight-shoota]

I first posted this as #4431 but @bcardiff referenced this issue which I had missed before. I'll just summarize some yet unmentioned aspects.

It is highly unexpected that global and class macro methods are preferred over instance methods. Shorter example code (carc.in):

macro render(foo)
  {{ foo.downcase }}
end

class Foo  
  def render
     render(nil)
  end
  
  def render(env)
  end
end

Foo.new.render

It is probably never really intended to invoke a macro if you try to call an instance method from within the scope of the same object. Especially when using libraries, they could not possibly take into account any macro that might be defined by any application. It would also suck to have to make self the explicit receiver of every method call.

A solution would be to have a different syntax for macro invocation

I'd question if macro methods should be directly visible inside instance methods at all. It's easy to confuse them with instance methods. I would suggest their visibility to be equal to class methods. To call a class macro from inside an instance method, it should be prefixed with the class scope. That requires no different syntax, just a more restrictive scoping. For class macros this is what I would have expected in the first place, analogous to class methods. Global macros are a bit different, since global methods are also visible inside instance methods, and for the limited set in Stdlib this is okay. It would probably be best to just not use them in applications...

A practical example of this isssue is described in kemalcr/kemal#359 . As already mentioned there, this can be worked around by adding an explicit scope: self.render(nil) in the sample code. The example for liquid.cr from @icyleaf is different, since tpl.render(ctx) has already an explicit scope but still invokes the global macro. I can't figure out why exactly this is failing but it also seems to be a naming conflict between instance method and macro method.

[bcardiff]

Some thought regarding this topic,

Interleaving the method lookup as jhass said sounds like the ideal thing to do. That is, when a foo call is found in an instance method, search if it is an instance method or a macro defined in the class, if not go to the top level name space and see if it is a macro or method there, after discarding it is not defined nearer.

The problem with that approach is that things like method_missing or method overridden down the hierarchy could be misleading.

One option (1) is to have a different syntax for macro calls and method calls. That will be more explicit, but I wouldn't like to loose the clean calls in the class body like property.

Another (2) would be that the macros visible should be defined/included in the lexical scope were it is invoked. But this does not follow the design of global namespace and could force the user requires as a prefix of the file to declare that a macro is wanted/preferred. This could solve the accidental hide of a method due to macros, but does not feels consistent with the rest of the language.

(3) What @straight-shoota mention in the quote: required class prefix on macro calls inside methods sound a good idea. It will be more explicit on some macro calls. But top level macro calls will still have more precedence (not solving method missing / method overridden dilema). Macro calls are expanded before the code is typed / compiled, so each call needs to be analyzed where it is a macro and changes de code to compile (the macro can get manipulate a block, introduce variables, etc). So the render issue exposed will still be present. So it is not a solution actually since it force you to do self. to avoid matching a macro. One could say let's prefix the intended top level macro call with ::, but that would look ugly for example in all the expect_raises or other dsl.

In general polluting the top level namespace with macros is dangerous since it can affects everything. The affected frameworks / apps I think are the ones that does not scope their code in class or modules. In those cases you can opt in which macros to use:

module DSL
  macro render
    puts "DSL.render"
  end
end

class Foo
  include DSL

  def render
    puts "Foo#render"
  end

  def do
    render
  end
end

class Bar
  def render
    puts "Bar#render"
  end

  def do
    render
  end
end

Foo.new.do # => DSL.render
Bar.new.do # => Bar#render

module DSL
  macro render
    puts "DSL.render"
  end
end

module Foo
  include DSL

  def self.render
    puts "Foo#render"
  end

  def self.do
    render
  end
end

module Bar
  def self.render
    puts "Bar#render"
  end

  def self.do
    render
  end
end

Foo.do # => DSL.render
Bar.do # => Bar#render

[asterite]

I fixed this. Now when macros are looked up and not found, but a method with the same name in the same scope is found, search doesn't continue up in the scope hierarchy. This should be much more intuitive and robust to program with.

[straight-shoota]

Great to see this resolved! 👍

[bcardiff]

@asterite, c87fc6d does not work with method missing and have some glitches with type hierarchy.

macro render
  puts "macro render"
end

class Foo
  def do
    render
  end

  macro method_missing(call)
    puts "method missing {{call.name.id}}"
  end
end

Foo.new.do # => macro render

class Bar < Foo
  def render
    puts "Bar#render"
  end
end

Bar.new.do                   # => Bar#render
[Bar.new, Foo.new].each &.do # => macro render, macro render
Foo.new.render               # => method missing render

The last should be Bar#render, macro render.
It would be ideal that all the printed macro render would be method missing render.

[asterite]

@bcardiff Old behaviour:

macro render
macro render
macro render
macro render
method missing render

Behavior after the commit:

macro render
Bar#render
macro render
macro render
method missing render

That means that I fixed the second case, which was the problematic one. All of the others were already broken before this commit.

I'd personally remove method_missing, it's a hack, not a good feature (I would consider type embedding like in Go for this)