squid:S1157 incorrectly suggesting equalsIgnoreCase when toLowerCase(Locale) is being called

java

(Trejkaz) #1

We have code comparing strings like this:

    str1.toLowerCase(locale).equals(str2.toLowerCase(locale));

This triggers the squid:S1157 warning which suggests switching to equalsIgnoreCase - but surely that isn’t safe? My experience in this situation is that equalsIgnoreCase can’t be trusted because it produces unexpected results for some strings in some locales.


(Nicolas Peru) #2

Reading equalsIgnoreCase javadoc https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#equalsIgnoreCase(java.lang.String)

it is not equivalent with your code but it will work for what you seem to be aiming to achieve.


(Trejkaz) #3

If the user is in Turkish locale, it gives the wrong result by returning true when comparing “i” and “I”, when it should return false.


(Nicolas Peru) #4

I struggle to understand where things are not working as you expect : could you try to write a small reproducer with the results produced by the call (true/false) and where the warning won’t be correct ? thanks for your help.


(Trejkaz) #5
import java.util.Locale;

public class UtilityClass {
    public static boolean faultyCompare(String a, String b) {
        // Fair to warn about this one.
        return a.toLowerCase().equals(b.toLowerCase());
    }

    public static boolean localisedCompare(String a, String b, Locale locale) {
        // This one is explicitly passing the locale, so the warning was questionable.
        return a.toLowerCase(locale).equals(b.toLowerCase(locale));
    }

    public static boolean sonarSuggestedCompare(String a, String b) {
        // Exists to test the suggested replacement.
        return a.equalsIgnoreCase(b);
    }
}

Test:

import java.util.Locale;
import org.junit.Test;

import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;

public class UtilityClassTest {
    @Test
    public void testFaultyCompare() {
        // Not appropriate to do this in a test, but illustrates the issue.
        Locale.setDefault(new Locale("tr", "TR"));

        // Will pass because we just set the locale, but would normally fail.
        // Relying on default locale in backend code is bad news, though, so warning on usages of toLowerCase()
        // with no Locale parameter is fair enough.
        assertThat(UtilityClass.faultyCompare("i", "I"), is(false));
    }

    @Test
    public void testLocalisedCompare() {
        assertThat(UtilityClass.localisedCompare("i", "I", new Locale("tr", "TR")), is(false));
        assertThat(UtilityClass.localisedCompare("i", "I", new Locale("en", "AU")), is(true));
    }

    @Test
    public void testSonarSuggestedCompare() {
        assertThat(UtilityClass.sonarSuggestedCompare("i", "I"), is(false));
    }
}

Results: testSonarSuggestedCompare fails, testLocalisedCompare passes. testFaultyCompare also passes, but only if your machine is set to the right locale to make it pass, which is definitely bad news.

So I think it can be summed up as:

  • toLowerCase() is no good and warning on that is fine. We actually make compile fail if someone uses it.
  • toLowerCase(Locale) is passing the locale, so maybe it’s fine, but I’m not 100% confident that the code is correct in all locales either, because a Collator is usually better
  • The suggested replacement, equalsIgnoreCase(), produces the wrong result as expected by the user, but is at least consistent, which might be preferable for some strings if the string is not going to be presented to a user anyway. Maybe @Nls / @NonNls annotations could help distinguish the two cases.