AmbiguousBlockAssociation gives bad advice

[mvz]

The message printed for the AmbiguousBlockAssociation cop is confusing because it assumes an intent on the part of the developer that does not match the actual behavior of Ruby. I.e., the message only makes sense if the code in question is currently broken.

Here is an example:

foo bar { |val| baz val }

Here, the cop would print something like:

Parenthesize the param bar to make sure that the block will be associated with the foo method call.

However, the code as it stands associates the block with bar, and if the code is currently working, the programmer should definitely not parenthesize the parameter, because it would change the meaning of the code.

Expected behavior

I expect RuboCop to either make no suggestion on how to resolve the ambiguity, or to suggest something that does not change the semantics of the code as it stands.

Actual behavior

RuboCop suggests a change that always breaks the existing code.

Steps to reproduce the problem

• Create a file foo.rb, containing:

  foo bar { |val| baz val }
  

• Run rubocop foo.rb.

RuboCop version

0.48.0 (using Parser 2.4.0.0, running on ruby 2.4.0 x86_64-linux)

[bbatsov]

Well, the message of the cop could probably be better, but the advice is just what it's supposed to be. That's not a bug. Might not play well with some DSLs, but the cop works just as it's expected to. Such code is confusing and should ideally be avoided.

[mvz]

Huh? The point of this bug report is that the message could be better. I'm not saying this should not be flagged as ambiguous.

[stephengroat]

we just started getting this error message which makes no sense at all:

verify.rb:59:8: W: Parenthesize the param sort_by to make sure that the block will be associated with the != method call.
   error('section.yml is not alphabetized by name') \
       if sections != sections.sort_by { |section| section['id'].downcase }
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

sort_by is not a param, it's a method, correct?

[mvz]

@stephengroat that looks like a different issue, so I think you should file a new ticket for it.

[stephengroat]

@mvz thanks will do

[bbatsov]

Huh? The point of this bug report is that the message could be better. I'm not saying this should not be flagged as ambiguous.

Suggestions for improvements are welcome. I'm not super fond of the current message myself.

[smakagon]

@mvz Feel free to suggest any improvements for description for this cop. I can change it and create PR when we all ok with that.

[ashmaroli]

Given: assert @site.pages.find { |page| page.data["date"] == date }
When I run rubocop against above,
I would like the warning message to have been:

W: Parenthesize the param @site.pages.find { |page| page.data["date"] == date } to make sure
that the block will be associated with the find
method call.
      assert @site.pages.find { |page| page.data["date"] == date }
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[smakagon]

Hi guys, @mvz @ashmaroli @bbatsov
I've extended offense message of this cop in this PR to show more context for developer.

Now for this code:
Foo.some_method a { |el| puts el }

It will return this message:

Parenthesize the param a { |el| puts el } to make sure that the block will be associated with the a method call.

It should provide better guidance for developer on fixing this offense.
What do you think?

[ashmaroli]

@smakagon Yes, that's more understandable and in line with what I suggested in the comment above. 👍

[mvz]

Yes, that's much better!