Allow block after bracket call syntax

[homonoidian]

Fixes #13975. I've been hitting this issue very much lately, I'm using self.[] in many of my projects for "smart constructors" and they sometimes require blocks. There's also a bug in the rough vicinity, foo[&.bar] and foo[1, &.bar] etc. causing the formatter to crash, maybe I'll try to fix it sometime later.

[HertzDevil]

This PR does not actually add support for foo[&.bar] at the moment, right?

[homonoidian]

If by "add support" you mean parsing it's already parsed correctly, and works perfectly fine, and was working perfectly fine prior to this PR.

The problem is with the formatter. Some of my code in particular uses foo[&.bar] and so on (and it's quite expected for someone using x[...] { ... } to try using x[..., &...] at some point). This and variations on this break the formatter.

I tried looking into it, but the formatter code is too... complicated for me as an outsider, I guess. What I saw (or what I believe I saw) is that x[] and x[...] are handled separately from normal calls (which is expected, I guess...) and it seems that someone didn't anticipate a block getting in there, maybe?.. Maybe not?.. So perhaps someone else more familiar with the formatter could pick this up? It's a separate issue in the topic/tag sense, but it's related to this one, that's why I've mentioned it in the first place. So maybe it'd be better if I or someone else creates a separate issue for this?

Anyway. Sorry for the long response, I couldn't resist :)

[oprypin]

It's also possible that foo[&.bar] works only by accident. I couldn't find any occurrences of it at all.

Though in Ruby this syntax works

[oprypin]

OK let's clarify this:

Block case

class C
  def [](n, &f)
    n.times do
      f.call
    end
  end
end

c = C.new
c[1] { puts "OK" }

• Ruby: OK

• Crystal before:

   10 | c[1] { puts "OK" }
           ^
  Error: unexpected token: "{"
  

• Crystal after: OK

• Crystal formatter before:

  syntax error: unexpected token: "{"
  

• Crystal formatter after: Deletes { puts "OK" } and succeeds -- not good

Block arg case

class C
  def [](&f)
    f.call(X.new)
  end
end

class X
  def test
    puts "OK"
  end
end

c = C.new
c[&.test]

• Ruby: N/A

• Crystal before:

   14 | c[&.test]
       ^
  Error: wrong number of block parameters (given 1, expected 0)
  

• Crystal after: Same syntax error as before

• Crystal formatter before: crash

  expecting ], not `&, `, at :14:3 (Exception)
  

• Crystal formatter after: Same crash as before

---

With this in mind:

• @homonoidian, could you clarify why you say that foo[&.bar] already works? As far as I see, it doesn't work.

• Additionally, I'm thinking maybe it shouldn't work??

• This PR sadly leaves the formatter in a dangerous state (syntax error before, bad behavior after)

[homonoidian]

Gosh, it removes it?! Okay. Let's see. This is getting more complicated than I thought :) As it always does...

1. So regarding the block arg case, what you call "syntax error" is correct (in that it should be an error) in both "before" and "after", it's due to f.call(arg) while the block is expecting no arguments (as is stated in the method signature def [](&f) aka def [](&f : ->)). Changing it to def [](&f : X ->) makes the example work. Or using yield instead of f.call makes it work in any case, because yield doesn't seem to take the signature into account (for whatever reason). And the crash? That's the crash I was talking about, yes. Bad bad bad.
2. That the formatter removes the block... Well, I suppose someone (that someone being me!) didn't even try to run it... That's human factor in play here. As I've said my assumption is that the formatter doesn't know about the block, it doesn't even try to render it. Therefore, it doesn't render it and that's what we see as "removal".
3. So I guess it's the same issue. Just two different kinds of blocks. In one case it's emitting junk because the block is part of "arguments" and it's at least trying to do something about it. The other case (do ... end or { }) the formatter simply isn't aware of. The block property is (after this PR) set by the parser but it is not read by the formatter.
4. So I do agree that this PR probably shouldn't be merged as is, without the formatter working as it should. I should probably look into the code. Maybe it isn't that hard after all to fix it.
5. I think it should work, but if and only if all the tools support it. It's better to have a syntax error guard than a portal to bug land. But I'm kind of going against myself here, as I'm already using it in quite a lot of code, and up to this point it only crashed the formatter, never removed anything. Gosh.

[straight-shoota]

I think this PR is good. We'll need to bring the formatter along to support the syntax. But that shouldn't be hard. I can take a look at it.

@oprypin Why do you think the short block syntax foo[&.bar] should perhaps not be valid syntax? IMO it makes a lot of sense... With this syntax of #[] methods, we're basically just using square bracket instead of parenthesis for method calls.

Anyway, it's probably OK to leave the short block syntax for a follow-up. If we need more discussion about whether it should be supported, we can do that in a dedicated issue.

[homonoidian]

Okay, so I looked into the code. The fix for x[1, 2, &foo] is really really straightforward. Because &foo is a block arg, and in the formatter format_args simply wasn't given the argument for the block argument. So it's a few characters kind of change right here. What's worse is &.foo. It's "rewritten" into a block in the parser (so &.foo becomes do |__arg0| __arg0.foo end). And there's some dark arts if block = node.block code that's dealing with this with the help of the lexer.

I did succeed in fixing the formatter issue, mostly. By refactoring the body of this condition into a separate method, let's say format_call_block, and then calling it roughly before the closing bracket is written, under a similar condition if block = node.block. That's roughly the fix. This results in more code than I'm comfortable adding to this PR. So I do think it's better to have a separate issue/PR for this, and I can upload most of the fix now but not all of it. "Not all of it" means there are a few failing tests out of those that I've added, and I couldn't find a way to fix them yet. Additionally, I've found a bug where formatting

foo(
  1,
  # 2,
  # 3,
  &.foo
)

... fails in a very very ugly way:

foo(
  1 # 2,
# 3,
,
  &.foo
)

Additionally the following:

foo(
  # Comment
  &.z
)

... formats to this:

foo(
  # Comment
&.z)

I suspect there are more issues like these ones, maybe some of them are already known. And my fix inherits them, causing a few failing tests. I've tested the above in 1.11.2, same kind of failure, so it's not me. Formatting foo[&...]? and foo[...]? { } is also mostly uncharted territory in my fix, more tests are needed. Formatting foo[] {} and foo[] do ... end works with the fix, as far as I can tell. And I suppose more tests are needed for calls too as the existing ones didn't catch the above. Without fixing calls [] won't format properly also (I mean, it will format properly in most common cases, it already does; but not in all cases).

And in the end, I'm lost! :rofl:

[straight-shoota]

Do you have a branch with your changes available?

[homonoidian]

Here it is: https://github.com/homonoidian/crystal/tree/brackets-block-formatting

[homonoidian]

It's working on my projects, I'm already using it and I don't think it broke any one of my files. But formatting bugs begin when indentation and multiline appear. I think I remember writing column where I presumably shouldn't... Probably responsible for some of the bugs. After months of writing 2-3 line methods my head is spinning. Everything is so convoluted there and my additions make it worse, as I don't have the bigger picture. Nah. Anyway.

[HertzDevil]

Block arg case in Ruby:

class C
  def [](&f)
    f.call(X.new)
  end

  def []=(*args, **opts, &f)
    f.call(X.new, args, opts)
  end
end

class X
  def test
    puts "OK"
  end

  def test2(args, opts)
    puts "#{args}, #{opts}"
  end
end

c = C.new
c[&:test]                        # "OK"
c[1, 2, x: 3, y: 4, &:test2] = 5 # [1, 2, {:x=>3, :y=>4}, 5], {}

---

This error:

 14 | c[&.test]
     ^
Error: wrong number of block parameters (given 1, expected 0)

is a semantic one, not a syntactic one, because a captured &f without a restriction is understood to accept no arguments. Doing yield X.new instead of f.call(X.new) will work as intended.

[homonoidian]

@HertzDevil The formatter also inserts a bad comma whenever one introduces a newline. E.g. "x[\n&.a\n]" (crash) or "foo[\n 1, 2, &.x]" which doesn't crash but results in:

foo[
  1, 2,
, &.x]

[stephannv]

It would be awesome to have this merged. My use case: I want to replace Foo.new(...).call by Foo[...], but it works only if the #call doesn't accept a block, if I do:

Foo[...] do 
  # do something
end

I get # Error: expecting token 'EOF', not 'do'.