Skip to content
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

Instance methods should not be shadowed by macros on the same scope #4724

Open
straight-shoota opened this issue Jul 18, 2017 · 9 comments
Open

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Jul 18, 2017

This is a continuation of #236 which gave methods in the same scope as the caller precedence over macros higher up in the scope hierarchy. But it is still inconvenient that macros shadow instance methods on the same scope:

class Foo
  macro foo
    "macro"
  end
  def foo
    "method"
  end
  
  def call_foo
    foo
  end
  
  def call_self_foo
    self.foo
  end
  
  def call_macro_foo
    Foo.foo
  end
end

pp Foo.foo                # => macro
pp Foo.new.foo            # => method
pp Foo.new.call_foo       # => macro
pp Foo.new.call_self_foo  # => method
pp Foo.new.call_macro_foo # => macro

https://carc.in/#/r/2dsx
Ideally, only the first and last call should invoke the macro.

As I have expressed in #236 (comment):

It is highly unexpected that [...] class macro methods are preferred over instance methods.
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. [...] It would also suck to have to make self the explicit receiver of every method call.
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.

@bcardiff in #236 (comment):

(3) What @straight-shoota mentioned in the quote: required class prefix on macro calls inside methods sound a good idea. It will be more explicit on some macro calls

I would suggest to either
a) remove macro lookup from the implicit scope entirely which would make macro calls from within an instance method the same as class methods.
b) give instance methods of the same name precedence over the macro.

In both cases it would be necessary to be able to call macros explicitly from instance scope as Foo.my_macro/self.class.my_macro. The latter is actually not working, but it would be nice to have a way to explicitly call a macro on the current scope without naming the scope.

@ysbaddaden
Copy link
Contributor

Well, self.class.foo is a method call because self.class is a method call, and happens to return Foo at runtime (not compile time). This is different from Type.foo which may be a compile-time macro call or, in a second time, a runtime method call. This behavior is expected to me.

Same for calling a macro inside a method. It's expected for the macro to be expanded at compile time, not delayed to be a method call.

Having to specify the type for all macro calls, for the unlikely case of having conflicts in macro and method names (which is a bad design) would be a huge drawback. Shall we have to call Object.property, because macros may conflict with class methods?

@straight-shoota
Copy link
Member Author

Well, the macro property will certainly not be called from an instance scope, so there is no trouble in confusing it with a method.
Prefixing all macro calls with a static receiver would only be required for case a), which is probably not ideal.
In b) it would only be required to specify an explicit receiver if there is a local method with the same name. Just as it is currently required the other way around: foo calls the macro, self.foo the method. I'd prefer that foo calls the method.
I would not necessarily deem if a bad design if there are macros and methods with the same, as they can be designated to be called from different scopes: Instance methods are obvious called on instances, but macros can be called on the class scope.
When both macro and method with the same name exist, I would argue that in almost all cases it is intended to call the instance method from instance scope (in an instance method). Otherwise you're right about bad design.

An example from my current rework of Kemal to use encapsulation instead of global scope:

class MyApp < Kemal::Application
  get "/" do |env|
    "hello"
  end
  def initialize_instance_routes
    get "/foo" do |env|
      "bar"
    end
  def
end
app = MyApp.new
app.initialize_instance_routes
app.run

Line 2 should call the maro, line 6 the instance method.

It is never intended to call the macro from instance scope. I guess it would also be a possibility to give the macro a modifier that it should only be visible in class scope.

@RX14
Copy link
Contributor

RX14 commented Jul 18, 2017

Why don't we just disallow macros and methods of the same name in the same class? We've established that it's bad design and confusing, why not disallow it?

@straight-shoota
Copy link
Member Author

Where have we established that it's bad design? ;)
Are instance and class methods of the same also bad design? If not, why would macros and instance methods be bad design, when the macros are only intended for use on the class scope?

I support the idea to disallow macros and methods of the same name, if there was a way to designate a macro as being visible only from class scope, where there would be no collision with an instance method. That would actually be according to suggestion b).

What about scoping a macro with self.? As far as I can see, this currently has no meaning and it is analog to class methods. The only difference would be that "instance" macros without self. are also visible at class level. And there should probably not be both self.foo and foo macro on the same class.

class Foo
  macro self.foo
    "macro"
  end
  def foo
  end
end

@asterite
Copy link
Member

It's definitely a bad design, and we should disallow it. I absolutely agree with @RX14 . It's super confusing.

@straight-shoota
Copy link
Member Author

What do you think about the idea of (optionally) limiting macros to class scope just like class methods? It's clearly separated scope (class vs. instance) so no confusion. It's quite common to have class and instance methods of the same name, so why shouldn't it be possible to have a class macro instead of a class method?
It would allow a usecase as I have described with regard to Kemal, where it makes sense to have a get method on class instances and a get macro on class scope.

@RX14
Copy link
Contributor

RX14 commented Sep 10, 2017

@straight-shoota you mean have macros act exactly the same as methods regarding def foo and def self.foo scope and visibility? I think i'd be for that proposal.

@straight-shoota
Copy link
Member Author

Yes, exactly .macro self.foo would only be visible from class scope to avoid mixup with instance methods.

Though the current behaviour makes macro foo visible from both, class and instance scope. It would be a bit confusing that macro self.foo and def self.foo have the same meaning, whereas macro foo and def foo have different visibility. I'm not sure what to do about this but I think it makes sense to keep the current behaviour of macro foo.

@RX14
Copy link
Contributor

RX14 commented Sep 11, 2017

@straight-shoota i'd prefer it if it unified macro and def's behaviour entirely, including modifiers like private etc. Actually nvm thats a terrible idea.

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

No branches or pull requests

4 participants