Style/UnneededCondition autocorrect changes semantics.

[jcoyne]

Similar to #6415, but that was syntax and this is semantics.

Expected behavior

Autocorrect should not change the behavior of the code.

Actual behavior

Autocorrect changed the behavior of the code.

Steps to reproduce the problem

Given this code:

     unless @members # if members have not been fetched and cached for this object yet, fetch them

        with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
          @members = @fetcher.get_collection(@druid) # cache members in an instance variable
        end
     else
       @members # return cached instance variable
     end

and I run rubocop -a

it transforms it into

     with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
       @members = @fetcher.get_collection(@druid) # cache members in an instance variable
     end || @members

which does not behave in the same way.

RuboCop version

$ rubocop -V
0.62.0 (using Parser 2.5.3.0, running on ruby 2.5.3 x86_64-darwin17)

[rrosenblum]

Given your example, I think we could catch this by checking if the conditions mutate the variable. In reality, I don't think we will be able to catch all instances of this problem.

If the modification of the variable happened in a different method, we wouldn't be able to easily identify it.

with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
  update_member
end

def update_member
  @members = @fetcher.get_collection(@druid) # cache members in an instance variable
end

Maybe we should mark this cop as unsafe auto-correction. I feel like as time goes on, we are going to find edge cases for nearly every auto-correction, and we are going to wind up marking most auto-correction as unsafe.

[mvz]

I don't see the autocorrection given above with RuboCop 0.63.0. Instead, I get the following, which seems just fine to me:

@members || with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
  @members = @fetcher.get_collection(@druid) # cache members in an instance variable
end

@jcoyne which version of RuboCop are you using and can you confirm the autocorrection you gave?

[jcoyne]

I posted the version in my original report. 0.62.0

[jcoyne]

It still seems to be a problem with 0.63.0

before:

# test.rb
def my_method
     unless @members # if members have not been fetched and cached for this object yet, fetch them

        with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
          @members = @fetcher.get_collection(@druid) # cache members in an instance variable
        end
     else
       @members # return cached instance variable
     end
end

rubocop -a --only Style/UnneededCondition test.rb
rubocop -v
0.63.0
cat test.rb
def my_method
     with_retries(max_tries: Dor::Config.release.max_tries, base_sleep_seconds: Dor::Config.release.base_sleep_seconds, max_sleep_seconds: Dor::Config.release.max_sleep_seconds) do |_attempt|
          @members = @fetcher.get_collection(@druid) # cache members in an instance variable
        end || @members
end

[mvz]

Ah, I see what happened when I tried it: I did a full rubocop -a, which first changed the unless ... else ... end into its equivalent if ... else ... end. After that the autocorrect for UnneededCondition did the right thing.

My conclusion: This is a simple bug and UnneededCondition just messes up for the unless case. Whether the conditions mutate the variable is not relevant in this case.

A simpler case:

unless bar
  foo
else
  bar
end

Should be corrected to:

bar || foo

But the actual result is:

foo || bar

[Drenmi]

Thanks for the investigation @mvz! 🙇 Would you be interested in opening a PR for this?

[mvz]

@Drenmi I'm trying to wrap my head around the logic of IfNode#if_branch, and I find it confusing. I see that it probably does the right thing for some cases, but I also see the following occur several times, which reverses the logic for unless nodes:

condition, if_branch, else_branch = *node

In the present cop, we have both the line above, and node.if_branch, so if_branch has two different meanings within the same cop!

TL;DR: I'm going to fix this bug by fixing just this cop, but I'd like to make this #if_branch method clearer and since you wrote it I'd like your input.

[mvz]

@Drenmi following up on my last comment, I would like replace #if_branch by #first_branch, and add #truthy_branch. For example, take this code:

unless foo
  bar
else
  baz
end

Here, #if_branch and its replacement #first_branch would return the node for bar, and #truthy_branch would return the node for baz (which is the result if foo is truthy).

Wdyt?

[rrosenblum]

I like the idea behind first_branch and truthy_branch. The current implementation of if_branch does seem a little weird knowing that the if and else branches are flipped for unless.

IfNode#node_parts is a good example of this

      def node_parts                                  
        if unless?                                                
          condition, false_branch, true_branch = *self          
        else                                                             
          condition, true_branch, false_branch = *self    
        end                           
                                  
        [condition, true_branch, false_branch]                                 
      end 

[Drenmi]

[...] following up on my last comment, I would like replace #if_branch by #first_branch, and add #truthy_branch.

This certainly seems like an improvement to how understandable the API is. I'm all for it. I'll let you go ahead and decide on the naming. 🙂