diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java index 442f01bab4e..15ccb40dcbd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java @@ -19,6 +19,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Matchers.constructor; import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.staticMethod; import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; @@ -37,16 +38,25 @@ /** Flags unsafe usages of the {@link java.util.Locale} constructor and class methods. */ @BugPattern( - summary = "Possible unsafe operation related to the java.util.Locale library.", + summary = "Possible unsafe operation related to the java.util.Locale class.", severity = WARNING) public final class UnsafeLocaleUsage extends BugChecker implements MethodInvocationTreeMatcher, NewClassTreeMatcher { private static final Matcher LOCALE_TO_STRING = instanceMethod().onExactClass("java.util.Locale").named("toString"); + private static final Matcher LOCALE_OF = + staticMethod().onClass("java.util.Locale").named("of"); private static final Matcher LOCALE_CONSTRUCTOR = constructor().forClass("java.util.Locale"); + // Used for both Locale constructors and Locale.of static methods. + private static final String DESCRIPTION = + " They do not check their arguments for" + + " well-formedness. Prefer using Locale.forLanguageTag(String)" + + " (which takes in an IETF BCP 47-formatted string) or a Locale.Builder" + + " (which throws exceptions when the input is not well-formed)."; + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (LOCALE_TO_STRING.matches(tree, state)) { @@ -54,11 +64,21 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .setMessage( "Avoid using Locale.toString() since it produces a value that" + " misleadingly looks like a locale identifier. Prefer using" - + " Locale.toLanguageTag() since it produces an IETF BCP 47-formatted string that" - + " can be deserialized back into a Locale.") + + " Locale.toLanguageTag() since it produces an IETF BCP 47-formatted string" + + " that can be deserialized back into a Locale.") .addFix(SuggestedFixes.renameMethodInvocation(tree, "toLanguageTag", state)) .build(); } + if (LOCALE_OF.matches(tree, state)) { + Description.Builder descriptionBuilder = + buildDescription(tree) + .setMessage("Avoid using the Locale.of static methods." + DESCRIPTION); + + fixCallableWithArguments( + descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state); + + return descriptionBuilder.build(); + } return Description.NO_MATCH; } @@ -66,41 +86,37 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState public Description matchNewClass(NewClassTree tree, VisitorState state) { if (LOCALE_CONSTRUCTOR.matches(tree, state)) { Description.Builder descriptionBuilder = - buildDescription(tree) - .setMessage( - "Avoid using Locale constructors, and prefer using" - + " Locale.forLanguageTag(String) which takes in an IETF BCP 47-formatted" - + " string or a Locale Builder."); + buildDescription(tree).setMessage("Avoid using the Locale constructors." + DESCRIPTION); - // Only suggest a fix for constructor calls with one or two parameters since there's - // too much variance in multi-parameter calls to be able to make a confident suggestion - ImmutableList constructorArguments = - ImmutableList.copyOf(tree.getArguments()); - if (constructorArguments.size() == 1) { - // Locale.forLanguageTag() doesn't support underscores in language tags. We can replace this - // ourselves when the constructor arg is a string literal. Otherwise, we can only append a - // .replace() to it. - ExpressionTree arg = constructorArguments.get(0); - String replacementArg = - arg instanceof JCLiteral - ? String.format( - "\"%s\"", ASTHelpers.constValue(arg, String.class).replace('_', '-')) - : String.format( - "%s.replace('_', '-')", state.getSourceForNode(constructorArguments.get(0))); + fixCallableWithArguments( + descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state); - descriptionBuilder.addFix( - SuggestedFix.replace(tree, String.format("Locale.forLanguageTag(%s)", replacementArg))); - } else if (constructorArguments.size() == 2) { - descriptionBuilder.addFix( - SuggestedFix.replace( - tree, - String.format( - "new Locale.Builder().setLanguage(%s).setRegion(%s).build()", - state.getSourceForNode(constructorArguments.get(0)), - state.getSourceForNode(constructorArguments.get(1))))); - } return descriptionBuilder.build(); } return Description.NO_MATCH; } + + // Something that can be called with arguments, for example a method or constructor. + private static void fixCallableWithArguments( + Description.Builder descriptionBuilder, + ImmutableList arguments, + ExpressionTree tree, + VisitorState state) { + + // Only suggest a fix for constructor or Locale.of calls with one parameter since there's + // too much variance in multi-parameter calls to be able to make a confident suggestion + if (arguments.size() == 1) { + // Locale.forLanguageTag() doesn't support underscores in language tags. We can replace this + // ourselves when the constructor arg is a string literal. Otherwise, we can only append a + // .replace() to it. + ExpressionTree arg = arguments.get(0); + String replacementArg = + arg instanceof JCLiteral + ? String.format( // Something like `new Locale("en_US")` or `Locale.of("en_US")` + "\"%s\"", ASTHelpers.constValue(arg, String.class).replace('_', '-')) + : String.format("%s.replace('_', '-')", state.getSourceForNode(arguments.get(0))); + descriptionBuilder.addFix( + SuggestedFix.replace(tree, String.format("Locale.forLanguageTag(%s)", replacementArg))); + } + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsageTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsageTest.java index 759acb43314..f3d596b8106 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsageTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsageTest.java @@ -71,6 +71,46 @@ static class Inner { .doTest(); } + @Test + public void unsafeLocaleUsageCheck_localeOfUsageWithOneParam_shouldRefactorNonLiteralParam() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + static class Inner { + private Locale locale; + + Inner(String a) { + locale = Locale.of(a); + } + } + + private static final Test.Inner INNER_OBJ = new Inner("zh_hant_tw"); + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + static class Inner { + private Locale locale; + + Inner(String a) { + locale = Locale.forLanguageTag(a.replace('_', '-')); + } + } + + private static final Test.Inner INNER_OBJ = new Inner("zh_hant_tw"); + } + """) + .doTest(); + } + @Test public void unsafeLocaleUsageCheck_constructorUsageWithOneParam_shouldRefactorLiteralParam() { refactoringHelper @@ -95,6 +135,30 @@ class Test { .doTest(); } + @Test + public void unsafeLocaleUsageCheck_localeOfUsageWithOneParam_shouldRefactorLiteralParam() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + private static final Locale LOCALE = Locale.of("zh_hant_tw"); + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + private static final Locale LOCALE = Locale.forLanguageTag("zh-hant-tw"); + } + """) + .doTest(); + } + @Test public void unsafeLocaleUsageCheck_constructorUsageWithTwoParams_shouldRefactor() { refactoringHelper @@ -125,7 +189,49 @@ static class Inner { private Locale locale; Inner(String a, String b) { - locale = new Locale.Builder().setLanguage(a).setRegion(b).build(); + // BUG: Diagnostic contains: forLanguageTag(String) + locale = new Locale(a, b); + } + } + + private static final Test.Inner INNER_OBJ = new Inner("zh", "tw"); + } + """) + .doTest(); + } + + @Test + public void unsafeLocaleUsageCheck_localeOfUsageWithTwoParams_shouldRefactor() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + static class Inner { + private Locale locale; + + Inner(String a, String b) { + locale = Locale.of(a, b); + } + } + + private static final Test.Inner INNER_OBJ = new Inner("zh", "tw"); + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + static class Inner { + private Locale locale; + + Inner(String a, String b) { + // BUG: Diagnostic contains: forLanguageTag(String) + locale = Locale.of(a, b); } } @@ -159,6 +265,30 @@ static class Inner { .doTest(); } + @Test + public void unsafeLocaleUsageCheck_localeOfUsageWithThreeParams_shouldFlag() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + static class Inner { + private Locale locale; + + Inner(String a, String b, String c) { + // BUG: Diagnostic contains: forLanguageTag(String) + locale = Locale.of(a, b, c); + } + } + + private static final Test.Inner INNER_OBJ = new Inner("zh", "tw", "hant"); + } + """) + .doTest(); + } + @Test public void unsafeLocaleUsageCheck_toStringUsage_shouldRefactor() { refactoringHelper @@ -208,7 +338,7 @@ String getLocaleDisplayString() { } @Test - public void unsafeLocaleUsageCheck_multipleErrors_shouldFlag() { + public void unsafeLocaleUsageCheck_multipleErrorsWithNew_shouldFlag() { compilationHelper .addSourceLines( "Test.java", @@ -226,6 +356,25 @@ class Test { .doTest(); } + @Test + public void unsafeLocaleUsageCheck_multipleErrorsWithLocaleOf_shouldFlag() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Locale; + + class Test { + private static final Locale LOCALE = + // BUG: Diagnostic contains: forLanguageTag(String) + Locale.of( + // BUG: Diagnostic contains: toLanguageTag() + Locale.TAIWAN.toString()); + } + """) + .doTest(); + } + @Test public void unsafeLocaleUsageCheck_instanceMethodUsage_shouldNotFlag() { compilationHelper diff --git a/docs/bugpattern/UnsafeLocaleUsage.md b/docs/bugpattern/UnsafeLocaleUsage.md index 38be98d7b85..437dd0ada1b 100644 --- a/docs/bugpattern/UnsafeLocaleUsage.md +++ b/docs/bugpattern/UnsafeLocaleUsage.md @@ -4,26 +4,29 @@ examples of error prone issues below: #### Constructors The constructors don't validate the parameters at all, they just "trust" it -100%. +100%. \ +This is also true for the static method `Locale.of`, introduced in JDK 19. For example: -``` -Locale locale = new Locale("en_AU"); -toString() : "en_au" -getLanguage() : "en_au" -locale.getCountry : "" - -locale = new Locale("somethingBad#!34, too long, and clearly not a locale ID"); -toString() : "somethingbad#!34, too long, and clearly not a locale id" -getLanguage() : "somethingbad#!34, too long, and clearly not a locale id" -getCountry() : "" +```java +Locale locale = new Locale("en_AU"); // or Locale.of("en_AU") +locale.toString(); // "en_au" +locale.getLanguage(); // "en_au" +locale.getCountry(); // "" + +locale = new Locale("somethingBad#!34, long, and clearly not a locale ID"); +// or Locale.of("somethingBad#!34, long, and clearly not a locale ID") +locale.toString(); // "somethingbad#!34, long, and clearly not a locale id" +locale.getLanguage(); // "somethingbad#!34, long, and clearly not a locale id" +locale.getCountry(); // "" ``` As you can see, the full string is interpreted as language, and the country is empty. -For `new Locale("zh", "tw", "#Hant")` you get: +For `new Locale("zh", "tw", "#Hant")` and `Locale.of("zh", "tw", "#Hant")` +you get: ``` toString() : zh_TW_#Hant @@ -51,20 +54,70 @@ There's no reliable way of getting a correct result through a `Locale` constructor, so we should prefer using `Locale.forLanguageTag()` (and the IETF BCP 47 format) for correctness. -**Note:** You might see a `.replace("_", "-")` appended to a suggested fix for +**Note:** You might see a `.replace('_', '-')` appended to a suggested fix for the error prone checker for this bug pattern. This is sanitization measure to handle the fact that `Locale.forLanguageTag()` accepts the "minus form" of a tag (`en-US`) but not the "underscore form" (`en_US`). It will silently default to `Locale.ROOT` if the latter form is passed in. +**Note:** This error-prone rule cannot reliably fix constructors and static +method `Locale.of` with two or three parameters, because a proper fix requires +more context. + +If the initial code started with a `String` that was split at `'_'` or `'-'`, +just to be used for locale, the right fix is to use `toLanguageTag()`. + +```java +// Initial code +void someMethod(String localeId) { + String[] parts = localeId.split("_"); + Locale locale = switch (parts.size) { + case 1 -> new Locale(part[0]), // or Locale.of + case 2 -> new Locale(part[0], part[1]), // or Locale.of + case 3 -> new Locale(part[0], part[1], part[2]), // or Locale.of + } + // use the locale +} + +// Fixed code +void someMethod(String localeId) { + Locale locale = Locale.forLanguageTag.replace('_', '-'); + // use the locale +} +``` + +If the initial code started separate "pieces" (language, region, variant) +the right fix is to use a `Locale.Builder()`. + +```java +// Initial code +void someMethod(@NotNull String langId, String regionId) { + Locale locale (regionId == null) + ? new Locale(langId) // or Locale.of + : new Locale(langId, regionId); // or Locale.of + // use the locale +} + +// Fixed code +void someMethod(@NotNull String langId, String regionId) { + Locale.Builder builder = new Locale.Builder(); + builder.setLanguage(langId); + if (regionId == null) { + builder.setCountry(regionId); + } + Locale locale = builder.build(); + // use the locale +} +``` + #### toString() -This poses the inverse of the constructor problem +This poses the inverse of the constructor problem. -``` +```java Locale myLocale = Locale.forLanguageTag("zh-hant-tw") String myLocaleStr = myLocale.toString() // zh_TW_#Hant -Locale derivedLocale = ??? // Not clean way to get a correct locale from this string +Locale derivedLocale = ??? // Not clean way to get a correct locale from myLocaleStr ``` The `toString()` implementation for `Locale` isn't necessarily incorrect in