RSpec/ReceiveMessages is marked as a safe autocorrect, but is not

[john-h-k]

Aside from the fact that receive_messages and similar could be overridden (which in my mind means it should never be considered a safe cop), it is unsafe:

allow(Foo).to receive(:bar).and_return(1)
baz_double = instance_double(Baz)
allow(Foo).to receive(:cat).and_return(baz_double)

autocorrects to

allow(Foo).to receive_messages(bar: 1, cat: baz_double)
baz_double = instance_double(Baz)
# errors as the double is created after its use

[pirj]

This can be solved by autocorrected code to the last position of previous code, not first.

would you like to send a PR, @john-h-k ?

[ccutrer]

Just ran into the same issue:

     it "properly serializes a response with no recordings" do
-      allow(@bbb).to receive(:conference_key).and_return("12345")
+      allow(@bbb).to receive_messages(conference_key: "12345", send_request: response)
       response = { returncode: "SUCCESS",
                    recordings: "\n  ",
                    messageKey: "noRecordings",
                    message: "There are no recordings for the meeting(s)." }
-      allow(@bbb).to receive(:send_request).and_return(response)
       expect(@bbb.recordings).to eq []
     end

[ccutrer]

And another one, of this form:

allow(object).to receive(:message).and_return(something)
x = object.some_method
allow(object).to receive(:another_message).and_return(x)

In this case, it's impossible to to use receive_messages, since the first mock must be set up before the method is called that calculates the return value for the second mock.

[john-h-k]

I should be able to confidently run safe rubocop autocorrects without even checking them, and this doesn’t pass that test. Even with the change to reorder the lines better, this is still an inherently unsafe fix

[pirj]

I should be able to confidently run safe rubocop autocorrects without even checking them

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND

I don’t have much to add to that.

[john-h-k]

I don't mean in a legal sense. I mean that is the attitude taken by the rest of rubocop. Other "safe" autocorrects are ones which have zero effect on the actual behaviour of the program and instead just visual. This affects the behaviour

[ydah]

Consecutive allow(Foo).to receive(:bar).and_return(baz) limiting offenses to only should eliminate cases of unintended violations and automatic modifications, so it may be a good to change it that way. WDYT?

The following are offenses

allow(Foo).to receive(:bar).and_return(1)
allow(Foo).to receive(:cat).and_return(baz)

The following are not considered offenses

allow(Foo).to receive(:bar).and_return(1)
do_something
allow(Foo).to receive(:cat).and_return(baz)

[ccutrer]

I was thinking that for a bit, until I just ran into one of this form:

allow(object).to receive(:message1).and_return([{ some_complicated_config}])
allow(object).to receive(:message2).and_return(object.message1.first)

so... consecutive expectations and none of those expectations' return value makes a call to the receiving object? but what about if it calls some other method, which would then call the receiving object? I think it should just be marked as unsafe, with some of these examples in the docs for why it's unsafe.

[pirj]

I don’t mean in a legal way. I mean bugs and deficiencies take place, and “should confidently without checking” is an aspiration, but we almost never can make sure.
We do our best by running autocorrection on new/modified cops against our current project’s code and check if it works as expected.
But we don’t have the capacity to ensure it is so for e.g. doing this on all real-world-rspec repos.

in RuboCop, a cop is safe to auto-correct if it is “by design”, not “by implementation”. This is where bugs and design flaws manifest the difference between the expectation and reality.

[pirj]

Is the example with using the result of one call as the return value of another realistic?

[ccutrer]

I mean, I found it in an actual code base, and it caused my specs to fail until I figured out why.

As to if it’s well-written code… that could be up for debate.

[bquorning]

#1687 marks the cop’s autocorrection as unsafe. Reviews are welcome.

This can be solved by autocorrected code to the last position of previous code, not first.

Let’s try to do this in a later PR.