crystal-lang / crystal

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

Continue with anonymous block arguments #8764

Open straight-shoota opened 4 years ago

straight-shoota commented 4 years ago

Follows up on #8117. This PR added syntax for anonymous block arguments def foo(&). The idea is to use this syntax exclusively for yielding methods. Currently it's valid to completely omit the block argument when there's yield in the body. This hides the yielding nature of the method from the signature and API docs. Such an essential property should be made visible.

After the initial syntax was added, we switched usage in stdlib in #8394.

The next step would be to deprecate some of the currently valid method signatures. The idea as proposed in #8117:

This would be a breaking change for yielding methods with a named block argument or no block argument at all, and also for methods with a named block argument that's never used in the literal scope of its body. But it improves clarity and helps to better identify the behaviour of block arguments.

Part of #7157

RX14 commented 4 years ago

This hides the yielding nature of the method from the signature and API docs.

No, it shows up in the API docs if you use yield. We shouldn't force people to annotate their method with an unneccesary & if they use yield but don't want to annotate the arg types. The rest of the changes are great.

straight-shoota commented 4 years ago

Oh my bad. The API docs already represent this correctly.

I'd still prefer always specifying & on yielding methods (types can be omitted obviously). Even if it's technically unnecessary, it still improves readability of the source code. We already put & in the signature in the API docs, but reading code is almost as important. I acknowledge that this doesn't need to be enforced by the compiler, though. It could just be a style guide. Maybe consider it for the formatter?

HertzDevil commented 2 years ago

It seems Ruby eventually wants to require & in methods that accept a block too.

For context, Ruby 3.1 allows a bare & to forward the anonymous block argument, whether it is captured or not (see #12110). Block forwarding requires a corresponding bare & parameter:

def foo1(&)
  yield
end

def bar1(&)
  foo1(&)
end

bar1 { 1 } # => 1

def foo2(&block)
  block.call
end

def bar2(&)
  proc(&) # captures anonymous block
  foo2(&)
end

bar2 { 1 } # => 1

I don't know whether anonymous block forwarding is exactly equivalent to a named one which would capture the block. It is definitely not a syntactic transformation though.

Also of note is that Def#block_arg returns a Nop unless the block parameter is used in the method body: (#5334)

module Foo
  def a1(&);                       end
  def b1(&block);                  end
  def c1(& : ->);                  end
  def d1(&block : ->);             end
  def a2(&);           yield;      end
  def b2(&block);      yield;      end
  def c2(& : ->);      yield;      end
  def d2(&block : ->); yield;      end
  def b3(&block);      block.call; end
  def d3(&block : ->); block.call; end
end

{% Foo.methods.map(&.block_arg) %}                           # => [, , , , , , , , block, block : (->)]
{% Foo.methods.select(&.block_arg.is_a?(Arg)).map(&.name) %} # => [b3, d3]

Likewise, when the Def nodes are printed only b3 and d3 retain their block parameters. I think this should change too, even if it means some of the Args will have an empty name.

zw963 commented 1 year ago

No, it shows up in the API docs if you use yield. We shouldn't force people to annotate their method with an unneccesary & if they use yield but don't want to annotate the arg types. The rest of the changes are great.

i don't agree with this idea, if yield exists in method body, block must be provided when invoke this method, add a extra & in method parameter is more clear way reveal this.

Also see following code for a really use case need to improve in std-lib.

https://github.com/crystal-lang/crystal/blob/771bddce1b39830316f03136a1cf8245aa976523/src/socket/server.cr#L48-L55

See discuss here, A extra & in the pamameter will make issue like this more clearly, user don't need search yield in (probalby a big) method body too find out whether this method need a block.