Lint/AmbiguousBlockAssociation Pattern Matching Regression

[jcalvert]

I discovered what I believe to be a regression in the behavior of the Lint/AmbiguousBlockAssociation cop from v1.33.0 on. For a large code base, when upgrading RuboCop from 1.32 to the latest revision, I saw previously matching lines now fail.

When the commit was made deprecating IgnoredMethods for AllowedMethods and AllowedPatterns, the argument to ignored_method? was changed from node.last_argument.send_node.source to node.last_argument.method_name for matches_allowed_pattern?. This means that the string passed to the regular expression changed, so regular expressions that worked prior to 1.33.0 may not match. The output of the cop message still uses send_node.source for interpolation as well. This makes the message instructions difficult to mark for allowance because the output will include the whole method chain but only match on the last call.

Given the language of the output message it's possible the intention would be for this cop to only want the last method call in the chain to appear. The most common usage for the Allowed methods and patterns on this cop is for the rspec style - because of that I would provide color commentary that I think providing the whole method chain to match against the regular expression is more useful. Idiomatic rspec usage is typically in conjunction with other method calls like receive and with which help differentiate specs from legitimate uses where we would want the cop to trigger.

---

Expected behavior

for AllowedPatterns of [/receive\(.*?\)\.twice/]

expect the string

expect(order).to receive(:complete).twice { OrderCount.update! }

to have no offenses

Actual behavior

 expect(order).to receive(:complete).twice { OrderCount.update! }
       +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Parenthesize the param `receive(:complete).twice { OrderCount.update! }` to make sure that the block will be associated with the `receive(:complete).twice` method call.

Steps to reproduce the problem

Included above

RuboCop version

1.35.1 (using Parser 3.1.2.1, rubocop-ast 1.21.0, running on ruby 2.7.5 x86_64-linux)
  - rubocop-rails 2.15.2

[bbatsov]

@ydah, please look into this.

[ydah]

@jcalvert You are right, this problem is happening. I had missed it. Thank you.
Thanks also for sending us the PR. I will check that PR.

[ydah]

As I mentioned in #10970 (comment) , I think this is the same problem with AllowedMethods.

[koic]

AllowedMethods and AllowedPatterns have different uses, so AllowedMethods seems fine.

[bbatsov]

Agreed. AllowMethods should be doing only exact name matches.

[ydah]

IgnoredMethods may differ from the previous behavior strictly because if Regex is not specified, the comparison will be by equivalence operators like AllowedMethods is now. However, if the difference in behavior does not seem to be a problem, it seems to be a good.