crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.45k stars 1.62k forks source link

Combination of `responds_to?` and `forward_missing_to` #4490

Open jbomanson opened 7 years ago

jbomanson commented 7 years ago

I am considering the use of responds_to? in combination with forward_missing_to. However, responds_to? is prone to returning false negatives when used in such a way. The following is an example of this.

class A
  def one
  end

  def two
  end
end

class B
  def initialize(@value : A)
  end

  forward_missing_to @value
end

x = B.new(A.new)
x.one

pp x.responds_to? :one   # true
pp x.responds_to? :two   # false (This should be true!)
pp x.responds_to? :three # false

I would expect that x responds to the first two methods, and not only the first one. The key difference between the methods one and two appears to be the x.one call. If that call is removed, all of the outputs are false. If x.two is called, the respective output becomes true.

The problem persists even if the following code is appended to the above.

if x.responds_to? :two
  x.two
end

That is, in order for responds_to? to detect that a method created with forward_missing_to exists, the method must be called and that call apparently must not rely on the responds_to? call.

I am running Crystal 0.22.0 [3c71228] (2017-04-20) LLVM 3.5.0.

asterite commented 7 years ago

I'm more in favor of removing method missing

faultyserver commented 7 years ago

I'm also in favor of removing method_missing. In a dynamic, interpreted language, I think it makes sense to allow dynamic delegation of calls to other objects (method_missing in Ruby, __getattr__ in Python, send in any message-passing language), but in a static, compiled language, it seems less beneficial, if not harmful.

I'd much rather see instances of method_missing replaced with delegate, to: to specify an explicit set of calls that are passed to an object. More than a few times, I've implemented method_missing in Ruby and accidentally hidden some errors, because the result of method_missing was close enough to my expected result that I didn't notice I hadn't implemented the method directly on an object. Using delegate makes that (almost) impossible, since only a fixed set of calls will be handled, and any others will properly raise an error at compile time.

The primary use case I see for method_missing in Ruby is to create DSLs like FactoryGirl or other generic configuration systems, but this is pretty easily possible with macros, though the resulting language may not be exactly as clean.

Additionally, while method_missing is a macro, most of the name resolution that it generally gets used for in languages like Ruby would still be evaluated at run-time, meaning a lot of compiler optimizations for method calls can't happen, so most uses of it will likely incur a hefty performance hit as a trade off for allowing developers to be a bit lazier.

tl;dr: method_missing is a nice feature for rapid-prototyping in dynamic languages. Crystal already has delegate to cover most use cases, and method_missing encourages laziness/bad-practice for the sake of simplicity for the user.

ysbaddaden commented 7 years ago

I believe the only case I used #method_missing is for transparent calls from a relation back to class methods (like calling missing methods on an ActiveRecord::Relation actually calls methods defined on the model). I think it can be solved with specific relation types and delegations (that can be automatically defined at compile time) or even a redesign to have and extend a MyModel::Relation type.

Either way, I can do without #method_missing.

jbomanson commented 7 years ago

I have no major objections to removing method_missing.

However, I see some benefits to keeping it.

class WithSavedSize(E, T)
  include Enumerable(T)

  getter saved_size : Int32

  def self.new(e : Enumerable(T)) forall T
    new(e, T)
  end

  protected def initialize(@value : E, @dummy : T.class)
    @saved_size = value.size
  end

  forward_missing_to @value
end

module Enumerable
  def with_saved_size
    WithSavedSize.new(self)
  end
end

a = [1, 2, 3].with_saved_size
a.saved_size # 3
a << 4       # [1, 2, 3, 4]
a.saved_size # 3

PS. Perhaps another issue should be made on removing method_missing, so that it is more prominently displayed and that any people who rely on it will notice it.

asterite commented 7 years ago

I think the most common use case is to build delegators. In Java it's a pain, and in every other statically typed language too. In Go you can embed a struct or interface inside another type and methods are automatically forwarded (and you can forward to multiple expressions). In D they have alias this. I'd rather have that specific functionality in the language that a half-baked method_missing that interacts badly with responds_to? and that can't accept a block.

bew commented 7 years ago

Another use-case of method_missing would be to compose a method name on usage. Transform a call like Model.new.get_keys_by_length, to a DB request for example

paulcsmith commented 7 years ago

Here is an example where I think method_missing is useful https://play.crystal-lang.org/#/r/25sn. I plan to make it so it works with has... methods as well. This example isn't the best use case, but I'm planning to use it for things like page objects so I can do something like this:

flow = UserFlow.new

flow.visit_user_index

flow.should_have_users_header
# or reverse it without having to define a new method
flow.should_not_have_users_header
# Instead of this:
flow.has_users_header?.should be_true

Maybe not a HUGE improvement, but I it makes me happy :) So my vote is to keep it. I also really like forward_missing_to as an easy way to do decorators.

straight-shoota commented 7 years ago

Why don't you just define some custom expectations?

flow.should have_users_header

That's consistent with the style in stdlib and doesn't need to rely on method_missing. I find it more clearly stated as well.

straight-shoota commented 7 years ago

For simple excpectations I don't think there is anything wrong with flow.admin?.should be_true. This again is more consistent and it's easier to understand what property is being tested.

paulcsmith commented 7 years ago

@straight-shoota That works, but it's hard to share matchers across specs because if you include them they are included globally. Maybe not a big deal, but sometimes expectations are similarly named. But for now, I think I'll take that approach and hope nothing clashes.

I could also solve it if I could do a private include, e.g. private include UserFlow::Matchers, but that doesn't seem to be supported/on the roadmap.

I still think method_missing/forward_missing_to is really useful for decorators. For example here's something I'm using for testing: https://gist.github.com/paulcsmith/dd21b628c92160960e2efb2638c66433#file-actually_working_box-cr-L9

It makes using it a lot nicer because I don't have to call @record.whatever all the time, and I don't have to manually delegate all the fields. Maybe this can be done some other way with the macro system though? I admit my solution may not be ideal. I'm still new to Crystal so I'm experimenting a lot :D

straight-shoota commented 7 years ago

I interpreted your example here, that have_users_header would be a more complex check but from the playground example I can see, that it only substitutes methods should_be_X with X.should eq true (better would be: be_true). For these simple things it obviously makes no sense to create a custom expectation.

There have been a few comments in favor of file-private includes, so I'd expect this to come one day in some way... ;)

If you need a dynamic delegator, you can create it without method_missing (code example):

class FooWrapper
  def initialize(@foo : Foo)
  end

  macro finished
    {% methods = Foo.methods.reject{ |method| method.name == "initialize" } %}
    delegate {% for method in methods %}:{{ method.name.id }}, {% end %} to: @foo
  end
end
paulcsmith commented 7 years ago

@straight-shoota I tried to simplify the example and removed superfluous code. In reality it would be a more complicated matcher. But I think you're right it's best to just include a module in those cases, and if private includes come, even better!

Thanks for that example. That pretty much covers my use cases for method_missing so I guess I don't need it after all :D

HertzDevil commented 3 years ago

If method_missing is defined, then one of two things will happen:

Therefore, theoretically every instance of B will respond to any call signature on it, even #three (it is A that doesn't respond to it) and missing overloads such as #one(Int). This is probably why Ruby's respond_to? ignores method_missing, in which case the false positive here is responds_to?(:one). To do that we just need to add an extra semantic property to Crystal::Def to indicate whether it comes from a method_missing expansion.

An edge case is if any of the named parameters is the empty string:

class Foo
  macro method_missing(call)
  end
end

Foo.new.foo("": 1, x: 2)
# Error: no overload matches 'Foo#foo', : Int32, x: Int32
#
# Overloads are:
#  - Foo#foo(*, x)