Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parenthesized forwarded args in Style/MethodCallWithArgsParentheses omit_parentheses #9637

Merged

Conversation

gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Mar 25, 2021

The new-style arguments delegation included in Ruby 2.7 and expanded in
Ruby 3.0 is required to be parenthesized, since the syntax is clashing with the
endless start... range, which can be started with start ... as well. Currently,
the most iffy cop of them all is issuing offenses for:

def delegated_call(...)
  @proxy.call(...)
             ^^^^^ Omit parentheses for method calls with arguments.
end

Removing the parens will start an endless range with the beginning being the
result of the method call without arguments. See the parse trees below for more
details:

-> ruby-parse -e "def foo(...); bar(...); end"
(def :foo
  (args
    (forward-arg))
  (send nil :bar
    (forwarded-args)))

-> ruby-parse -e "def foo(...); bar ...; end"
(def :foo
  (args
    (forward-arg))
  (erange
    (send nil :bar) nil))

-> ruby-parse -e "def foo(...); bar...; end"
(def :foo
  (args
    (forward-arg))
  (erange
    (send nil :bar) nil))

We don't want that, so allow the parentheses and leave this gotcha to
the poor developer that's gonna run onto it.

@gsamokovarov gsamokovarov force-pushed the omit-parentheses-forwarded-args branch from d53ac45 to 05ea477 Compare March 25, 2021 12:31
@gsamokovarov gsamokovarov force-pushed the omit-parentheses-forwarded-args branch 3 times, most recently from 86bd5ea to e01b512 Compare March 25, 2021 16:24
The new-style arguments delegation included in Ruby 2.7 and expanded in
Ruby 3.0 is required to be parenthesized, since the syntax is clashing
with the endless `start...` range, which can be started with `start ...`
as well. Currently, the most iffy cop of them all is issuing offenses
for:

```ruby
def delegated_call(...)
  @proxy.call(...)
             ^^^^^ Omit parentheses for method calls with arguments.
end
```

Removing the parens will start an endless range with the beginning
being the result of the method call without arguments. See the parse
trees below for more details:

```bash
-> ruby-parse -e "def foo(...); bar(...); end"
(def :foo
  (args
    (forward-arg))
  (send nil :bar
    (forwarded-args)))

-> ruby-parse -e "def foo(...); bar ...; end"
(def :foo
  (args
    (forward-arg))
  (erange
    (send nil :bar) nil))

-> ruby-parse -e "def foo(...); bar...; end"
(def :foo
  (args
    (forward-arg))
  (erange
    (send nil :bar) nil))
```

We don't want that so allow the parens and leave this gotcha to the poor
developer that's gonna run into it.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 28, 2021

The new-style arguments delegation included in Ruby 2.7 and expanded in
Ruby 3.0 is required to be parenthesized, since the syntax is clashing with the
endless start... range, which can be started with start ... as well.

I'd add something along those lines to the cop's documentation.

the most iffy cop of them all

Well said! This has to be the cop with the most special cases and bugs in the history of RuboCop! 😆

@gsamokovarov gsamokovarov force-pushed the omit-parentheses-forwarded-args branch from e01b512 to 8eed46d Compare March 28, 2021 14:43
@gsamokovarov
Copy link
Contributor Author

But boy, do we love this style in Dext! Maybe there is a better approach to tackle the parentheses omission as in this form we have to enumerate the cases where omitting them leads to ambiguous or invalid code. 😅

I have updated the docs to hint that there are other cases we allow parentheses even if we are enforcing the omit_parentheses style.

@bbatsov bbatsov merged commit 7f45422 into rubocop:master Apr 4, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants