[Fix #12988] Add new Style/AmbiguousEndlessMethodDefinition cop

[dvandersluis]

Adds a new Style/AmbiguousEndlessMethodDefinition cop to look for endless methods that are wrapped in and, or, if, unless, while or until nodes but likely cause confusion due to operator precedence.

For example:

def foo = true if bar

may appear to be def foo = (true if bar) but is actually parsed as (def foo = true) if bar. The cop corrects by wrapping the clause in parentheses to make the former explicit.

Autocorrection is not safe because the implicit precedence may actually be desired.

---

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.

[koic]

Question: should this be in the Lint namespace instead, to match Lint/AmbiguousOperatorPrecedence? Should the name be changed to match?

As ongoing in #11111, this should be categorized under Style department, not Lint department, at the least. This is just a note, TBH, I have doubts about whether it’s worth providing this as a cop. It seems like the type of issue that should be addressed by LSP's Inlay Hints.

[koic]

I feel uncomfortable with presenting something that is not compatible with the bad example as good. This could be misleading for developers who properly understand the precedence of operators.

[dvandersluis]

Yes this is a good point that the good case has different behaviour than the bad case. I was thinking that the parens in the good case would be what was actually intended for the majority of cases like this, but making the parens explicit for the actual precedence is possible too.

I am not sure that it is very likely that someone would write code along these lines and actually mean for the and/or/if etc. to modify the def rather than the def body, but I could be wrong. Maybe @phene or @vlad-pisanov can weigh in given their participation in the issue.

[koic]

It doesn't seem appropriate to use endless method definitions in cases where they can cause confusion. Methods that can lead to ambiguity without specifying operator precedence are likely not suited for endless method definitions in the first place.
In such cases, I think it's better to suggest using the regular method definition style. For example, the following would be bad and good examples:

      #   # bad
      #   def foo = true and false
      #
      #   # good - When the method body is the intended focus. This is incompatible behavior.
      #   def foo
      #     true and false
      #   end
      #
      #   # good - When the intended meaning is clear. This is compatible behavior.
      #   def foo
      #     true
      #   end and false

Also, because the user intent can't be made completely clear, I think autocorrect should not be supported.

[dvandersluis]

@koic thank you for your input. What do you think about having the autocorrect change to your "compatible" form? In that case the ambiguity is resolved in a way that does not change behaviour.

[dvandersluis]

I have updated the cop examples as per @koic's suggestion, and changed autocorrect to replace with a normal method definition instead of an endless method, in order to reduce ambiguity but retain the behaviour as is. @koic: I think this autocorrect is appropriate now, but if you still feel strongly about not having autocorrect I can remove it!

[bbatsov]

I'm OK with the proposed code, but I'm not sure about the proposed name. To me something like AmbiguousEndlessMethodDefinition makes a bit more sense. Might also be a good idea to add a style guide entry about this.

@rubocop/rubocop-core Any other suggestions are welcome!

[dvandersluis]

@bbatsov thank you for the review! I've updated the name as you suggested.

[dvandersluis]

@bbatsov I've opened a ruby-style-guide PR here: rubocop/ruby-style-guide#949

[dvandersluis]

Updated to include link to the styleguide.