Warn on expressions that contain logical operators but no braces

[m-o-e]

A pretty mean (and probably common) trap:

$ cat >test.cr <<EOC
foo = ["batz"]

if foo.includes? "bar" || foo.includes? "batz"
  puts "this code is bug-free"
end
EOC

$ crystal test.cr
(no output)

$ ameba test.cr

Inspecting 1 file

.

Finished in 2.21 milliseconds
1 inspected, 0 failures

Can easily go unnoticed even with test-coverage.
I think it would be great if ameba would warn about it.

[straight-shoota]

The proposed implementation in #510 catches a lot of issues which could be considered false positives and be more of a nuisance than helpful.

For example, the following expressions seem very clear without parantheses. Yet this rule complains about them.

puts message || "no message"

printf "%s, %d\n", message || "no message", 1

Part of the reason is of course that puts and printf are well-known methods which return only Nil. So it wouldn't make sense to "or" their return values.
But in the second call there is simply no possibility for misinterpretation because there is another argument after the binary op.

I wouldn't consider it completely inadequate to require adding parentheses in these cases. But I'm not convinced it should be necessary to do so.

The example from the OP of course would certainly benefit from a complaint about missing parenthesis.
Maybe we just need a more fine-grained set of conditions? I'm not sure if there's any feasible way to discern this though.

[nobodywasishere]

That's valid, thank you for the feedback. Maybe it can be narrowed down to a Call for each args where if any arg is BinaryOp, and the right of that is another Call with a BinaryOp. A little specific, but maybe that'd be fine?

[Sija]

I agree with @straight-shoota. We can start with a narrow scope and expand it later if needed.