Make Style/RedundantParentheses allow parentheses in assignment

[koic]

Fixes #12511 (comment).

This PR makes Style/RedundantParentheses allow parentheses around conditional expressions in assignment to respect user's intentions for readability:

var = (foo || bar)

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

I think this should be some configuration option, not a default behavior.

[koic]

I think that by default user's intent can be respected with var = (a || b). If user don't like it, intentionally writing var = foo || bar will not register an offense.
How about starting with that approach, and if issues become noticeable, consider adding a configuration option?

[bbatsov]

My point is that if a cop is supposed to register redundant parentheses anything that's redundant, but someone might want to keep should be considered some special case. In general it's hard to know if some redundant parentheses where placed on purpose or accidentally.

[bbatsov]

Also - it seems me it'd be good to extend the example's in the cop's documentation to cover more cases of redundant parentheses (e.g. like the one we're discussing right now).

[koic]

That reasoning makes a lot of sense. OTOH, I apologize for sharing my personal opinion, but for instance, the operator precedence differs in the following example:

$ ruby-parse -e "var = foo || bar"
(lvasgn :var
  (or
    (send nil :foo)
    (send nil :bar)))

$ ruby-parse -e "var = foo or bar"
(or
  (lvasgn :var
    (send nil :foo))
  (send nil :bar))

I understand that allowing parentheses in these cases provides consistent readability. But, the current behavior forces parentheses to be removed when using ||. (which is, of course, expected behavior).
For a use case, see rubygems/rubygems@7cc647c#diff-6e1c7ff50326d1315e87ff73c66c05ba3897487bcbac0ed1290ad3a3bd889efc, though I doubt that this improved readability, from var ||= (foo || bar) to var ||= foo || bar.

So, I've added this special case to the documentation. How about starting with this?

[bbatsov]

So, I've added this special case to the documentation. How about starting with this?

Yeah, I agree the parentheses should be preserved when removing them would change the AST. I'd suggest the tests and the changes to focus on this.

[bbatsov]

Looking at the tests we've allowed them everywhere where there is assignment, and I don't think this always improves the readability.

I still think that by default they should allowed only where removing them would change the AST, and that there should be an option to allow them in assignments more broadly.

[bbatsov]

@rubocop/rubocop-core Anyone else has an opinion on how to proceed here?

[bquorning]

I would expect a Style/RedundantParentheses in its default configuration to flag not just some redundant parentheses but all redundant parentheses.

If that results in my code looking “inconsistent” because a = (b or c) has parens while a = b || c has not, it is fine. It actually underlines the difference between the two operators. (And in my personal opinion I think many people could use the reminder that || and or are not the same thing.)

[bbatsov]

@bquorning I totally agree with your perspective!