We’re still on an old SonarQube, so maybe you’ve addressed this already. But I checked over at Java static code analysis | Code Smell: Methods returns should not be invariant which I assume is kept up to date, and I don’t see anything about an exception. So if you’ve addressed this and I just need to upgrade, you still should change your rule description.
Basically, rule java:S1132 says if foo is a string, replace foo.equals("bar") with "bar".equals(foo) to preclude NPE problems. In one issue flagged in our code, we are checking for possible “blank” cases with something like
if (foo == null || foo.trim().equals("")) {...
The NPE checker java:S2259 doesn’t complain about foo.trim(). That rule’s flow checker is able to see that the null check with short circuit logic prevents a method call on a null object. So the question is, why is it getting dinged for S1132?
So a rigid application of S1132 would mean changing the right half to something like "".equals(foo.trim())... Now this is one of those cases that could go either way. Some might argue that you should ALWAYS follow S1132 to get into the habit. But others would argue that the original form is more readable, because it’s parallel construction (foo is first in both clauses, so obviously foo is what you’re interested in).
If you take the latter view, then this application of S1132 is clearly FP. You already have logic to check nulls and filter out cases with proper null-check predicates. So you just need to have S1132 tie into that logic.
BTW when I saw my formatted post, the link to your other page shows “Method returns should not be invariant” – yet the link takes you to the right page (S1132) so something is wrong here?
The second non-compliant example is similar to the example I posted, but simpler (no call to .trim() to complicate things). You might add a note to the effect that even though the second line is properly null-checked, the compliant replacement is still preferable since when you invert it like that, you get the null check “for free”. (For the benefit of people who aren’t Java gurus.)
Note that that url is rules DOT sonarsource DOT com/java/type/Code%20Smell/RSPEC-1132 (need to jump through hoops to keep your system from turning it into a link).
Now if I go to the parent (i.e., take off the RSPEC-1132 part) and then type “left” in the search bar, the third one on the search hit list looks like the right rule. And when I click on it, I get
which looks right. But the URL looks the same! And if I then copy-paste the URL from that new page into a DIFFERENT browser (so not a caching issue), I get something like the original screenshot (where the title says “Strings literals” but the text talks about returning invariant values).
(And shouldn’t that be “String literals” rather than “Strings literals” ? )
Oh, I should also point out that even if we didn’t have the null check, SQ’s recommendation of replacing foo.trim().equals(“”) with “”.equals(foo.trim()) wouldn’t save us, due to the extra call to trim(). If foo is null, then trim() will still produce an NPE. And if foo is NOT null, then trim() cannot produce a null – it only returns a valid (albeit possibly empty) String.
Of course, if we didn’t have the null check then hopefully java:S2259, the NPE checker, would be flagged.
Regarding the FP, I understand your point, and it seems valid, but after some discussions and thinking, it is decided this is out of the scope of this rule.
In case you posted: if (foo == null || foo.trim().equals("")) {... foo.trim() won’t produce NPE, but there are many cases where it could produce it.
We may have something to cover cases like this in the future, but not for now.
Not sure if my point was clear. What I was saying is that if you don’t null-check first, then SQ’s recommendation to change foo.trim().equals(“foo”) to “foo”.equals(foo.trim()) won’t save you one bit.
Yes, your point was clear and I understand your opinion.
I find this problem out of the scope of this rule. The rule only checks if the String literal is on the left side of the expression.
One more point from my side. When I suggested you use NEXT rule page rather than https://rules.sonarsource.com/, I meant only as a temporary solution until we fix the rules page.
