RefactoringMiner icon indicating copy to clipboard operation
RefactoringMiner copied to clipboard

Incorrect mapping leads to Invert Condition False Positive

Open victorgveloso opened this issue 2 years ago • 4 comments

https://github.com/apache/hbase/commit/29cf51d8391099f5628a52d2c3306932758d4108#diff-d827cf8ba45db10748de68b5b3d90987bb09b2bac680030080a940173f8f45b7L148-R203

The statements involved in the detected Invert Condition Refactoring are unrelated. Notably, while (!isUpdated) is in line 148 and if (isUpdated) is in line 202.

victorgveloso avatar Dec 15 '23 20:12 victorgveloso

Based on the diff, I think testUtil.waitFor(60000, 250, new ExplainingPredicate<Exception>() {

is now executing the same code in a loop, so the while loop is not needed any more.

So, there is a new if added at the end of the code with inverted condition from !isUpdated -> isUpdated

if (isUpdated) {
    return true;
}

RefactoringMiner allows the matching of different control statements, and in this case we have a control flow restructuring, where the original while is eliminated and at the end an if is added with an inverted condition. So, these two control statements are somehow related.

Screenshot from 2023-12-17 00-01-27 Note how the statement quotaCache.triggerCacheRefresh(); which was the first statement inside the body of the deleted while is also moved right after the newly added if This confirms that these two control statements are related.

Screenshot from 2023-12-17 00-10-17

tsantalis avatar Dec 17 '23 05:12 tsantalis

@victorgveloso Based on the analysis I did above, I think the Invert Condition is not a False positive.

tsantalis avatar Dec 17 '23 05:12 tsantalis

@pouryafard75 What is your opinion about our diff for this commit?

tsantalis avatar Dec 17 '23 13:12 tsantalis

@tsantalis @victorgveloso I prefer not to have a mapping for isUpdated.
However, there are more apparent inaccuracies in this diff such as:

  • L166 -> R157 , R167, R179. isUpdated = false;
  • L167 -> R158 , R168, R180. break;

Also, FP for:

  • L154 -> R150 boolean isBypass = true; -> boolean isUserBypass = quotaCache.getUserLimiter(User.getCurrent().getUGI(), table).isBypass(); if (isUserBypass != bypass) These statements are irrelevant.

Probably after fixing these inaccuracies, we can discuss the main issue again which was isUpdated but at the moment, I prefer not to have them mapped.

pouryafard75 avatar Dec 17 '23 18:12 pouryafard75