search

openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

Prefer String::equalsToIgnoreCase - "foo".equals("Foo".toLowercase()) -> "foo".equalsToIgnoreCase("Foo") #274

Open timo-abele opened 3 months ago

timo-abele commented 3 months ago

What problem are you trying to solve?

Just came across this finding from IntelliJ:

String upperFoo = "FoO"
"foo".equals(upperFoo.toLowercase())

can be

String upperFoo = "FoO"
"foo".equalsToIgnoreCase(upperFoo)

Condition: the first string is all lowercase

Would be nice to have that automated. I've considered posting this in rewrite-migrate-java, but as there is no "since" in the API I guess it has been around forever and was never a new language feature.

knutwannheden commented 3 months ago

Correct. There is in fact a StringRules recipe. This was intended as a demo for Refaster recipes, but there is a recipe in there which does almost what you want. In fact that recipe could be simplified.

I suspect a similar Refaster rule also exists in Errorprone Support, which is available via rewrite-third-party.

Stephan202 commented 3 months ago

I suspect a similar Refaster rule also exists in Errorprone Support, which is available via rewrite-third-party.

I had a look; we don't have such a rule. I'm also not sure we would accept such a contribution, as we instead prefer that a toLowerCase() is rewritten to accept a locale. The StringCaseLocaleUsage check provided by Error Prone was in fact contributed by Picnic. See also this SO thread about caveats around the use of String#equalsToIgnoreCase; I'd generally try to stay away from this method.

knutwannheden commented 3 months ago

@Stephan202 Interesting. I was not aware of this issue.

timo-abele commented 3 months ago

See also this SO thread about caveats around the use of String#equalsToIgnoreCase; I'd generally try to stay away from this method.

Interesting. I ended up not following the IntelliJ suggestion either. I'm not mad if the issue is rejected.

knutwannheden commented 3 months ago

As implemented in the demo recipe it should be safe: https://github.com/openrewrite/rewrite-migrate-java/blob/582216517bdbc601ec0203f87bbbbb8e8a424f22/src/main/java/org/openrewrite/java/migrate/lang/StringRules.java#L77

knutwannheden commented 3 months ago

... or maybe not. If the case conversions are known to use certain locales like ROOT I suppose it would be a safe operation.