Ban and/or on conditionals only

[vrthra]

I was wondering if there would be interest for a restricted AndOr cop that checked for and/or keyword use only within conditionals. This leaves the keyword operators free to be used as control flow operators. I am aware of the style guide opinion, but having a choice between complete ban and a partial ban would be nice. I have a prototype here which I can submit if the idea is worthwhile.

[bbatsov]

This was how this cop originally worked (as the first draft of the style guide promoted such style), but the problem is that not every use of and and or outside conditionals is for flow-of-control and there's no way to differentiate between them. Consider this:

some_result = some_bool or some_other_bool

x = some_value or some_other_value

While those cases might not pop very often they are quite real. That said, if @yujinakayama and @jonas054 are open to making the cop configurable I'll support this.

[jonas054]

I quite like constructs like

do_something or return

so I'd love to see a change in this cop. A configuration option is the way to go.

[adrienthebo]

To elaborate on this, there are a number of cases where using boolean operators for flow control display some unusual behavior:

Return:

[1] pry(main)> def flow_control
[1] pry(main)*   false or return :x
[1] pry(main)* end  
=> :flow_control
[2] pry(main)> flow_control
=> :x
[3] pry(main)> def boolean_op
[3] pry(main)*   false || return :x
[3] pry(main)* end  
SyntaxError: unexpected tSYMBEG, expecting keyword_end
  false || return :x
                   ^
[3] pry(main)> def boolean_op
[3] pry(main)*   false || (return :x)
[3] pry(main)* end  
=> :boolean_op
[4] pry(main)> boolean_op
=> :x

Raise:

[5] pry(main)> false or raise "boom!"
RuntmeError: boom!
from (pry):9:in `__pry__'
[6] pry(main)> false || raise "boom!"
SyntaxError: unexpected tSTRING_BEG, expecting keyword_do or '{' or '('
false || raise "boom!"
                ^
[6] pry(main)> false || (raise "boom!")
RuntimeError: boom!
from (pry):10:in `__pry__'

To me this seems like an unfortunate quirk of the Ruby language, but I think that this behavior actually degrades the readability rather than improves it.

It might be good to have a catchall And/Or cop in addition to a cop for checking for And/Or in conditionals so that people can incrementally remove logical composition operators without having to unravel the odd behaviors around raise and return at the same time.

I have slightly longer comment on this at puppetlabs/puppet#2900 (comment).

[bbatsov]

Btw, you could also write return(x) or raise(x). You should also keep in mind that we don't register offenses when changing the code you result in a modified AST.

[mikegee]

You should also keep in mind that we don't register offenses when changing the code you result in a modified AST.

I'm having trouble parsing this, but it sounds very interesting. Could you elaborate?

[bbatsov]

I'm having trouble parsing this, but it sounds very interesting. Could you elaborate?

x = y and z and x = y && z would produce different ASTs:

x = y and z

(and
  (lvasgn :x
    (send nil :y))
  (send nil :z))

x = y && z

(lvasgn :x
  (and
    (send nil :y)
    (send nil :z)))

Obviously such code cannot be auto-corrected. I think we actually register offenses in such scenarios, but don't autocorrect them. Maybe there should be an option not to register offenses at all.