crystal-lang / crystal

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

Make `responds_to?` match all arguments #2549

Open asterite opened 8 years ago

asterite commented 8 years ago

Consider this code:

class Foo
  def bar(x, y)
    x + y
  end
end

foo = Foo.new
if foo.responds_to?(:bar) # => true
  foo.bar(1) # Compile-time error!
end

# However...
foo.responds_to?(:bar) # => true

responds_to?, in its current form, is pretty weak. It only allows checking whether an object responds to a method with a name, but doesn't consider the arguments or its types.

We should maybe make responds_to? receive, in addition to the method name, the arguments and even a block. With that, we could check if a method can be invoked with those arguments and block, before actually performing the call (which will now never give a compile error).

foo = Foo.new
if foo.responds_to?(:bar, 1) # => false
  foo.bar(1) # never analyzed by the compiler
end

if foo.responds_to?(:bar, 1, 2) # => true
  foo.bar(1, 2)
end

This will also cover #2391, or at least part of it, because you could check that a method doesn't respond to a method with a given set of arguments (for example, I can test that a method should accept a string argument but not an int).

With this change, a call to .responds_to?(:foo) will check that there's an argless method foo. We should maybe have a way to check that there's any method foo, regardless of the arity, though I'm not sure it would be very useful (you usually use responds_to? before invoking a method, and you always have the arguments to it).

Thoughts?

ozra commented 8 years ago

I would very much like to still see a clean way of just checking for the method name, since in many cases that will probably suffice, and the code will then be less cluttered. Possibly as another pseudo-method? .has_method?(:foo) perhaps?

asterite commented 8 years ago

Or the same name but like responds_to?(:bar, *) (because responds_to? is already parsed in a special way, so...)

kumpelblase2 commented 8 years ago

I would prefer being able to specify a type instead of actual values. So for example instead of the example you made foo.responds_to?(:bar, 1, 2) I would prefer to have the following:

class Bar
    def baz(foo)
        foo.bytesize
    end
end

bar = Bar.new

bar.responds_to?(:baz, String) # => true
bar.responds_to?(:baz, Int) #=> false
luislavena commented 8 years ago

I was actually bitten by this a few times. responds_to? was working but then the method signature I was trying didn't match.

In my particular case, I need to compute several possible values for that method, so I think will be better if I can match the method signature's types instead. That way I avoid having the pre-calculated values for the method signature check when the compiler have already discarded that code block.

straight-shoota commented 7 years ago

As stated in #1551, responds_to? should also reflect if the specified method (or method overload) is callable from the current context.

asterite commented 5 years ago

I think having responds_to? with types is better. One could also specify named arguments and so on. So for example:

class Bar
  def foo(x)
  end

  def bar(x : Int32)
  end

  def baz(x : Int32, y : String)
  end
end

bar = Bar.new
bar.responds_to?(:foo) # => true
bar.responds_to?(foo()) # => false
bar.responds_to?(bar(Object)) # => true
bar.responds_to?(bar(Int32)) # => true
bar.responds_to?(bar(String)) # => false
bar.responds_to?(baz(Int32, y: String)) # => true
bar.responds_to?(baz(x: Int32, y: String)) # => true

So if [...] is specified you just simulate a call but instead of passing arguments you pass types, and you can use named arguments.

Maybe in some cases you already have a variable that you'll pass to the method so it's easier to check it with the type of that:

x = 1
bar.responds_to?(bar(typeof(x))) # => true

What do you think? I can try to implement it.

vladfaust commented 5 years ago

Wouldn't

bar.responds_to?(:foo) # => true
bar.responds_to?(:foo, nil) # => false # Maybe use something special like Void or NoReturn
bar.responds_to?(:bar, Object) # => true
bar.responds_to?(:baz, Int32, y: String) # => true

syntax be better?

Otherwise I feel like calling responds_to?(bar(Object)) actually has some side effects because of parentheses...

bew commented 5 years ago

@asterite I like your proposal, the only thing that's not clear enough i think when I read obj.responds_to?(bar(Int32)) is whether bar(Int32) will do a method call (with the actual class Int32 as param), or is just there as an 'AST' node..

One idea to make it more obvious that bar(Int32) won't be really called could be: obj.responds_to?(.bar(Int32)) (with a dot before the method to check)

asterite commented 5 years ago

@bew I really like that idea. I wanted to make it look like a call because that's what you are testing, whether it responds to a given call, even specifying a block (I didn't think of a syntax for that yet). The dot makes it clearer.

vladfaust commented 5 years ago

obj.responds_to?(.bar(Int32)) looks very unnatural IMO. It's not clear that you're actually calling a responds_to? method. I'd stick with symbols. Also dot usually means class method call, and IIRC responds_to? checks for instance methods. Please consider my proposal instead.

asterite commented 5 years ago

How do you distinguish between "responds to a method regardless of the arguments" and "responds to a method with the given arguments" in your proposal?

vladfaust commented 5 years ago

responds to a method regardless of the arguments

responds_to?(:foo)

responds to a method with the given arguments

responds_to?(:foo, Int32, String)
responds_to?(:foo, Int32, y: String)
responds_to?(:foo, x: Int32, y: String)

The only issue is with "responds to a method explicitly without arguments". I'm thinking of

responds_to?(:foo, nil)

or

responds_to?(:foo, Void)
asterite commented 5 years ago

Well, that special case right there is why I prefer my proposal: if it's a symbol then it's the first form, if it's a call then it's the second form. It's better to always try to avoid special cases.

straight-shoota commented 5 years ago

responds_to?(.bar(*args, **kwargs)) would also be an alternative for matching "any arguments". Or maybe just responds_to?(.bar(*)). This would allow to completely remove the symbol syntax, having one less special case ;)

Regarding type arguments, I think it can easily become very cluttered, when you have union types and either need to declare them explicitly or wrap them in typeof everywhere. I liked that in the original post. Being able to use explicit types is also great.

Maybe we could make typeof implicit if the expression is not a type? This would make it easy to ask if an object responds to a method with a specific argument without needing to care about the specific type of that argument:

if foo.responds_to?(.bar(x))
  foo.bar(x)
end

Otherwise we might consider to implement that specific use case in a method Object#call_if_responds_to.

Sija commented 5 years ago

This would allow to completely remove the symbol syntax, having one less special case ;)

@straight-shoota That would break BC and lot of code out there.

asterite commented 5 years ago

The only problem with passing variables like that is when the argument is an actual type (like one you would pass to a be_a spec matcher). Then it's not clear whether you want to pass a type or specify a type to match. I do agree that it would be more convenient, and probably more useful.

straight-shoota commented 5 years ago

@Sija True. The question is, will this variant still be used a lot or more and more replaced by explicitly typed responds_to? checks?

@asterite Yes, that's a downside because it can lead to slightly unexpected behaviour. But it might be excusable for avoiding tons of typeof. And it shouldn't cause real problems, it's just that you need to use typeof explicitly in these cases. (Which you would need to do anyway if the implicit typeof didn't exist).

vladfaust commented 5 years ago

@straight-shoota

This would allow to completely remove the symbol syntax

Is this the goal? Symbol syntax would need to stay for autocasting enum arguments and may be implementing compile-time checks in cases.

Symbols is what everyone in Crystal and Ruby got used to. It feels immutable and baked into the code. It matches perfectly with defined method existence check.

Dot syntax looks awful. Having an orphan dot (responds_to?(.foo) -- what am I, calling a greater void for foo?) is the last thing I wanted in my code.

@asterite

It's better to always try to avoid special cases.

False. Edge cases are everywhere. Everything should be as simple as it can be, but not simpler -- over-abstraction is bad. And in this particular case expressiveness and beauty are at risk.

straight-shoota commented 5 years ago

@vladfaust I'm only referring to symbol literals used with responds_to?. This is orthogonal to the discussion about the future of the symbol type.

The leading dot syntax is similar to what we already have with when matchers. And I think it is pretty good at describing it's purpose.

jhass commented 5 years ago

Maybe we should just do away with the "method call" appearance of this keyword now and do something like

if object responds_to? some_method(String)

so akin to for example Java's instanceof. Or extend the concept we're using for typeof and ask whether the expression we intend to call would work:

if callable?(object.some_method(some_string))
Sija commented 5 years ago

How about using block argument prefix?

foo.responds_to?(&.bar(1, :dos, "tres"))
jhass commented 5 years ago

My point is that this doesn't align with any of the semantics the proposed constructs currently have. These points have largely been made, but to summarize:

That's why I'm proposing to either introduce an entirely new syntax construct that doesn't really exist like this in the grammar yet, or at least fit it into the existing duality we already have in the call syntax. That is foo(expr) is either a call or a keyword evaluation like in typeof(expr).

I'm not a big fan of this duality existing in the first place (and one could argue we already have this duality in another place, through obj.is_a?*), but as I see it, it would fit into the second case, the one I called "keyword evaluation", of the call syntax already.

* I prefer reusing the typeof duality because the is_a? duality is a lot closer to just being a convenience tool vs. an actual functional difference, in the sense that it looks like a regular method call which the compiler is just smart about and doing some casting for us.

bew commented 5 years ago

With all the suggested syntaxes, you can't check if a prefix calls like ~obj would work on obj.

Also I think that the prefix dot-notation cannot be used with a parentheses-less responds_to? without being ambiguous for the reader: responds_to? .foo vs responds_to?(.foo)..

Another suggestion:

What if we do:

obj.responds_to?(~_)
obj.responds_to?(_.foo)
obj.responds_to?(_ + 2) # maybe not this... 
obj.responds_to? _.write(Slice)
obj.responds_to? _.bar(*)

Where _ would be replaced by obj, and the expression would be checked (eg: can obj responds to write with a Slice?)

Since it's known that in Crystal _ can't be used as a var name, I think it'll be easy to learn this syntax

jhass commented 5 years ago

With all the suggested syntaxes, you can't check if a prefix calls like ~obj would work on obj.

My suggestion solves this easily and naturally, without any new concept to learn:

if callable?(~obj)
straight-shoota commented 5 years ago

@bew Prefix operators are just normal methods without arguments. Using obj.responds_to?(:~) etc. should just work fine.

bew commented 5 years ago

@jhass oh right, I didn't see it in your summary post, that's interesting too!

oprypin commented 5 years ago

With the callable? suggestion, might as well go fully general (accept any expression) and name it compiles?

bew commented 5 years ago

The only thing I don't like with callable? (or compiles?) is that you'll have to write the code twice (of I understand correctly):

if callable?(obj.foo)
  obj.foo
end

Other option, a special method / keyword: (note: naming is hard)

do_if_compiles? do
  obj.foo
end
oprypin commented 5 years ago

{% try %} 😂

straight-shoota commented 3 years ago

In most use cases you have a particularly call that is optional (i.e. only call if it's a valid call). E.g. a.b(c) is only going to happen if a responds to b with argument c. So you really just want to know if a.b(c) compiles. Not if a.b can be called with a type C as first argument (type C would typically just be typeof(c)).

Is there any benefit of specifying typeof(c) instead of c (i.e. a.b(C))? I can't see any. It would just add unneccessary boilerplate or cause bugs when the argument type is hard coded but the type of the actual argument changes.

Per arguments in https://github.com/crystal-lang/crystal/issues/2549#issuecomment-454029138 I agree that a top-level method-like syntax is better than the current Object#responds_to? mimicking a method call on the receiver. This makes it really easy to just use exactly the same syntax as in the actual code. In the same way as the argument to typeof() is just regular code.

Block arguments haven't been mentioned yet at all, and I suppose they would be particularly difficult with type arguments. But callable?(a.b(c, &.d)) just works (I'm using short syntax for brevity, but a full block works as well - and that's really the point).

As suggested in https://github.com/crystal-lang/crystal/issues/2549#issuecomment-490673409 it seems very reasonable to have a generic compiles? feature. I don't think compilation check needs to be limited to just a single call. Implementation effort shouldn't be much different and it offers more versatility.

Generalization could even go a step further: This is very much like a nilable typeof. typeof raises a compiler error if the expression doesn't compile. typeof? could return nil instead and thus you can handle it (for example by not generating the actual code). compiles?(a.b(c)) would be equivalent to !typeof?(a.b(c)).nil?. It's the same concept, so we don't need a new name (just a variation on an existing one).

Because typical use cases revolve around checking if a call is possible and then making that call happen, there could be a convenient solution for that.

if compiles?(a.b(c))
  a.b(c)
end

Nobody likes to write a.b(c) twice (especially if it's a more complex call). A simple macro could be used for that:

macro try(call)
  if compiles?({{ call }})
    {{ call }}
  end
end

try a.b(c)

I'm using compiles? in the examples, but it could as well just be !typeof?(a.b(c)).nil? directly if we want to go that way; there would be no need for a separate compiles? feature, but it could be added as a convenience. The first step should be to start with compiler support for either compiles? or typeof? (or callable? in case I'm missing a huge argument why this should not be generalized). Then we can focus on convenience features in stdlib.

straight-shoota commented 3 years ago

Hm, I guess with a more generalized implementation it would be quite complex to apply filters to guard the actual code.

HertzDevil commented 2 years ago

The problem with a general typeof? or compiles? is that a corresponding type filter might act on multiple variables simultaneously in a non-trivial manner (i.e. not a conjunction of multiple single-variable filters). For example:

x = 1 || 'a'
y = 1 || 'a'
if compiles?(x < y)
  # (x.is_a?(Int32) && y.is_a?(Int32)) || (x.is_a?(Char) && y.is_a?(Char))
end

The equivalent is_a? expression above does not restrict any types; if we cannot make it work, we cannot make compiles?(x < y) work in the form above, or we need a completely new mechanism distinct from type filters for this. With the augmented responds_to? we have:

x = 1 || 'a'
y = 1 || 'a'
if x.responds_to?(&.<(y))
  # neither `Int32#<(Int32 | Char)` nor `Char#<(Int32 | Char)`
  # compiles, so this branch is unreachable
end

To me this seems more reasonable because the filter can only act on its receiver (x).

There is also a difference between overload matching and successful compilation, because the former does not actually have to type the method bodies and the latter must:

(..).responds_to?(&.each) # => true
compiles?((..).each)      # => false

A minor note is that some type names can be used even if the augmented responds_to? takes expressions instead of argument types:

x = uninitialized Int32
1.responds_to?(&.+(x)) # => true
asterite commented 2 years ago

To add a bit more to the discussion, instead of compiles? it might be better to have a compiler_error or compile_error method/macro. That would return nil if there's no error, and a String with the error otherwise. That way you could also check that the error message is what you expect it to be. This would be specially useful in cases you {% raise %}

HertzDevil commented 2 years ago

Something not mentioned is that passing arguments themselves instead of their types means autocasting applies to a bigger set of expressions:

enum Bar
  X
end

class Foo
  def foo(x : Int16); end
  def bar(x : Bar); end
end

x = Foo.new
v = 0_i8
s = :x

# arguments
x.responds_to?(&.foo(0))       # => true
x.responds_to?(&.foo(0_i8))    # => true
x.responds_to?(&.foo(1234567)) # => false
x.responds_to?(&.foo(v))       # depends on `-Dno_number_autocast`
x.responds_to?(&.bar(:x))      # => true
x.responds_to?(&.bar(s))       # => false

# argument types
x.responds_to?(&.foo(Int8))   # depends on `-Dno_number_autocast`
x.responds_to?(&.foo(v))      # not allowed
x.responds_to?(&.bar(Symbol)) # => false
x.responds_to?(&.bar(:x))     # not allowed
x.responds_to?(&.bar(s))      # not allowed
straight-shoota commented 2 years ago

Just to be clear: Shouldn't x.responds_to?(&.foo(Int8)) work as well (with argument types)? Autocasting can't take a concrete value into account (as with a literal). But autocasting from Int8 to Int16 should always work and could technically be applied with argument types.

That said, I would prefer value based argument matching anyways.

HertzDevil commented 2 years ago

Fixed and added examples for enums (which only work on literals). However, general Int8 to Int16 autocasting requires an argument node to be associated with a Crystal::NumberAutocastType, and there is no argument there, so that might be a bit difficult to do.

Also, to clarify, all the calls of the second kind are also valid calls of the first kind, passing metaclasses and looking for methods that take e.g. an Int8.class.