ASSERT-KTH/sorald

Bug in S1155

Closed this issue · 6 comments

Zuplyx commented

The processor for rule S1155 only checks for the presence of the == operator when deciding whether to negate the call to Collections.isEmpty(). This leads to wrong code being generated for statements like collection.size() < 1:

Repair generated by Sorald:

- collection.size() < 1
+ !collection.isEmpty()

Correct repair:

- collection.size() < 1
+ collection.isEmpty()
algomaster99 commented

Hi @Zuplyx ! Thanks for the bug report, and I agree we should also check for collection.size() < 1 because it is completely inline with their definition:

The time complexity of any isEmpty() method implementation should be O(1) whereas some implementations of size() can be O(n).

I will work on the fix, meanwhile could you report this to SonarSource as well? They tend to respond quickly. See https://community.sonarsource.com/t/s1481-unused-local-variables-should-be-removed-is-wrongly-documented/89427.

Zuplyx commented

Hi @algomaster99, I am unsure what to report to SonarSource, as collection.size() < 1 is already being detected as a violation by SonarJava. IMHO the issue lies entirely within Sorald, because it's repair wrongly generates a negated call (!collection.isEmpty()) when it should be just collection.isEmpty().

algomaster99 commented

IMHO the issue lies entirely within Sorald

Oh right, if we are repairing it, SonarSource must have reported the warning. I agree there is no need. I will produce a fix.

Zuplyx commented

Thank you.

algomaster99 commented

This issue has been fixed in https://github.com/ASSERT-KTH/sorald/releases/tag/sorald-0.8.3. Thank you for the report!

Zuplyx commented

Thanks for the quick fix.

  • 👍1