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

Fix a crash in Lint/EmptyConditionalBody #11712

Merged

Conversation

gsamokovarov
Copy link
Contributor

var =
  unless object.action value:, other:
    condition || other_condition # This is the value of `other:`, like so:
                                 # `other: condition || other_condition`
  end

The code above is tricky, because we may think condition || other_condition is the body of the condition, however the code is a valid case for the Lint/EmptyConditionalBody rule. It is equivalent to:

var =
  unless object.action value:, other: condition || other_condition
  end

However, the variable assignment is tricking CommentsHelp#find_end_line method into calling an #end method of Parser::Source::Map::Variable object. Parser::Source::Map::Variable does not respond to it. Other Parser::Source::Map subclasses also do not respond to #end methods so we need to check for its existing. Parser::Source::Map::RescueBody can be a parent node too.

```ruby
var =
  unless object.action value:, other:
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid `unless` branches without a body.
    condition || other_condition # This is the value of `other:`, like so:
                                 # `other: condition || other_condition`
  end
```

In the condition above, the variable assignment is tricking the
`CommentsHelp#find_end_line` method into calling an end method of
`Parser::Source::Map::Variable`. Other `Parser::Source::Map` subclasses
do not respond to respond to the `#end` method as well, won't hurt to
check.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 19, 2023

Looks good. I really hope we already has some cop that advises against splitting maps in such a manner over multiple lines, as such code looks horrible.

@bbatsov bbatsov merged commit adffde1 into rubocop:master Mar 19, 2023
@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Mar 20, 2023

This wasn't the intend of this code... The devs wanted:

var =
  unless object.action(value:, other:)
    condition || other_condition
  end

But since we're forcing them to omit the parentheses in our company, rubocop nicked them, and we ended up with the empty branch. To be honest, our devs are quite confused with this syntax and how we can omit the parentheses with it. We can do it only for the returning-value expression and the however-many-edge-cases there are. It's some tricky syntax to grok.

@gsamokovarov gsamokovarov deleted the lint-empty-branch-value-omission branch March 20, 2023 14:56
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

2 participants