Force calls with blocks to have their arguments parenthesised

[RX14]

This eliminates the subtle difference in associativity of do/end and {/} by enforcing calls which pass a block to have parentheses around their arguments to disambiguate. This is first done by changing the formatter to enforce this rule, formatting, then changing the compiler.

We should land the compiler change commit in next_release + 1 to give people a formatter tool which they can use to get their projects up to date, but it's included in this PR for criticism.

The largest hit is the spec DSL, every it "foo" do gets turned into it("foo") do, which is definitely uglier, but not impossible to live with I think. We could possibly also introduce a rule that this rule doesn't have to be followed if there's no block inside the block arguments, or we could unify both syntaxes to act like do/end, or we could leave this as-is (but I think the current state is confusing). So please start the discussion below.

[faustinoaq]

enforcing calls which pass a block to have parentheses around their arguments to disambiguate

I agree blocks {} and do/end should have the same behavior

[drhuffman12]

Maybe go ahead with adding this to the formatter, but as an optional param? Then, after the compiler changes [and other applicable code mods] are ready, this feature could be made default? I am guessing that is basically what @RX14 intended?

[ysbaddaden]

The largest hit is the spec DSL, every it "foo" do gets turned into it("foo") do, which is definitely uglier.

Perfect example to set my reaction to a definitive "hell no" :(

[ysbaddaden]

Was this change discussed anywhere? what benefit does it bring? What impact, except for the incredibly big patch to specs (for every projects)?

[konovod]

There was a discussion in #4504 and (reverted) #4517 .
But forcing a parentheses wasn't mentioned there and imho isn't a good solution.
I have nothing against a current state, but unification of both looks good too.

[asterite]

Even though I agree that do/end vs. { } can be confusing regarding precedence, I think it's not that terrible, and in Crystal it's less worse because you can't accidentally pass a block to a method that doesn't accept one. For example this code:

def foo(something)
end

foo File.open("foo.cr") do
end

In Ruby it works, but in Crystal you get an error:

Error in foo.cr:4: 'foo' is not expected to be invoked with a block, but a block was given

foo File.open("foo.cr") do
^~~

[bcardiff]

I fail to find some article/text explaining the motivation behind the different precedence of do/end and { }. All that I was able to find is how they work but not why it is a good design decision to have different precedence for the same thing. Maybe the motivation is to allow dsl with blocks passed to arguments with no parens. Like the one found in http://guides.rubyonrails.org/routing.html#redirection

get '/stories/:name', to: redirect { |path_params, req| "/articles/#{path_params[:name].pluralize}" }

Having the option allows the programmer to choose which one to use and avoid parenthesis.
I found it clear which one I prefer if parenthesis are avoided. Like the example above, using do/end will require extra parent and it will make uglier the code.

In situations where both options (do/end and { }) match in semantics, one can argue which should be used and it is more subject to opinions and harder to agree.

I get that the ambiguity can be a bit disrupting but I think is a case where it is a language feature that might not be that straight forward to grasp even if it is used a lot.

I think that been able to avoiding parenthesis in method calls is a key aspect for cleaner code. So I found it really hard to approve any change that goes in direction of mandatory parenthesis in the code. (Yet, I always liked them in method definition).

As @asterite point out, the compiler helps a bit to discover wrong invocation earlier.

[RX14]

First of all, I agree this is a bad implementation of disambiguating block syntax, because it ruins DSLs such as record and spec.

Being able to merge #4517 is the main motivation for merging this, I don't see any sane way to implement #4517 without making the two block syntaxes entirely interchangeable. And I think merging #4517 or similar has huge benefits (see #4504 for arguments). This PR was my first attempt at achieving this (I learnt a lot about the parser at least). I probably shouldn't have submitted it (it was 1AM and I didn't think much further than PR then sleep), and I should have thought about a better way to do this change some more.

I think a better approach would be to change {/} to have the precedence of do/end because the change would break far less. My initial reason for doing this PR instead of my previous suggestion is that it would catch all the edge cases whereas there could concievably be an edge case where changing {/} to do/end associativity would result in code which still compiled, yet would have a logic error. However, balancing against the diff size in this PR, I think it's a better route to go (I didn't think about spec it and co when I started this change).

Thoughts?

[asterite]

I'm thinking that maybe we can implement #4517 by being careful about the last argument to a call with a block. If the call has another call as its last argument, then { can't be changed to do. For example:

# Can be changed, `map` doesn't have arguments
[1, 2, 3].map { |x|
}

def method
end

# Can't be changed: last argument is a method,
# so changing '{' to 'do' would associated it to 'foo'
foo method {
}

# Can be changed
foo method, 1 {
}

Actually, after writing all of this, maybe we can make parentheses mandatory for calls that have a call with a {...} block as arguments? With that rule, the before-last example won't compile anymore and one would have to write:

# Error: `method { }` is an argument to 'foo`, so `foo` must use parentheses
foo method {
}

# OK
foo(method {
})

I actually find the second one less confusing.

Or, well, I think just implementing #4517 with the "last argument is a call" in mind would be good-enough already.

[RX14]

@asterite I thought up of a variation on your second idea (which replaces this PR) but I thought it might be a bit hard to do.

I don't like the making the formatter format differently on fairly arbitrary rules. It makes the formatter unpredictable and annoying.