From 8da2d79708f00b754beb048c9f8d98909c1bee52 Mon Sep 17 00:00:00 2001 From: Mihai Nita Date: Sun, 19 Oct 2025 13:06:44 -0700 Subject: [PATCH 1/3] Update UnsafeLocaleUsage with detection for Locale.of --- .../bugpatterns/UnsafeLocaleUsage.java | 84 ++++++---- .../bugpatterns/UnsafeLocaleUsageTest.java | 153 +++++++++++++++++- docs/bugpattern/UnsafeLocaleUsage.md | 85 ++++++++-- 3 files changed, 273 insertions(+), 49 deletions(-) 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..cd99fe557ba 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; @@ -44,6 +45,8 @@ public final class UnsafeLocaleUsage extends BugChecker 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"); @@ -54,11 +57,27 @@ 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 Locale.of static methods (which 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).\n" + + "Please read the Error Prone documentation page for UnsafeLocaleUsage for" + + " more information."); + + fixCallableWithArguments( + descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state); + + return descriptionBuilder.build(); + } return Description.NO_MATCH; } @@ -68,39 +87,42 @@ public Description matchNewClass(NewClassTree tree, VisitorState 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."); + "Avoid using Locale constructors (which 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).\n" + + "Please read the Error Prone documentation page for UnsafeLocaleUsage for" + + " more information."); - // 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..e6a57d7a303 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 (region == null) + ? new Locale(langId) // or Locale.of + : new Locale(langId, region); // 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 (region == null) { + builder.setCountry(langId); + } + 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 From b73d248119fc751e66e9bcadaab387bc1c2cf57b Mon Sep 17 00:00:00 2001 From: Mihai Nita Date: Mon, 20 Oct 2025 02:13:56 -0700 Subject: [PATCH 2/3] Fix sample code in UnsafeLocaleUsage.md Co-authored-by: Stephan Schroevers --- docs/bugpattern/UnsafeLocaleUsage.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/bugpattern/UnsafeLocaleUsage.md b/docs/bugpattern/UnsafeLocaleUsage.md index e6a57d7a303..437dd0ada1b 100644 --- a/docs/bugpattern/UnsafeLocaleUsage.md +++ b/docs/bugpattern/UnsafeLocaleUsage.md @@ -92,9 +92,9 @@ the right fix is to use a `Locale.Builder()`. ```java // Initial code void someMethod(@NotNull String langId, String regionId) { - Locale locale (region == null) + Locale locale (regionId == null) ? new Locale(langId) // or Locale.of - : new Locale(langId, region); // or Locale.of + : new Locale(langId, regionId); // or Locale.of // use the locale } @@ -102,8 +102,8 @@ void someMethod(@NotNull String langId, String regionId) { void someMethod(@NotNull String langId, String regionId) { Locale.Builder builder = new Locale.Builder(); builder.setLanguage(langId); - if (region == null) { - builder.setCountry(langId); + if (regionId == null) { + builder.setCountry(regionId); } Locale locale = builder.build(); // use the locale From 3cd7fea96d2566ee73004cda7f5273e302c269ab Mon Sep 17 00:00:00 2001 From: Mihai Nita Date: Mon, 20 Oct 2025 02:55:10 -0700 Subject: [PATCH 3/3] Implement feedback from review --- .../bugpatterns/UnsafeLocaleUsage.java | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) 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 cd99fe557ba..15ccb40dcbd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java @@ -38,7 +38,7 @@ /** 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 { @@ -50,6 +50,13 @@ public final class UnsafeLocaleUsage extends BugChecker 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)) { @@ -65,13 +72,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (LOCALE_OF.matches(tree, state)) { Description.Builder descriptionBuilder = buildDescription(tree) - .setMessage( - "Avoid using Locale.of static methods (which 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).\n" - + "Please read the Error Prone documentation page for UnsafeLocaleUsage for" - + " more information."); + .setMessage("Avoid using the Locale.of static methods." + DESCRIPTION); fixCallableWithArguments( descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state); @@ -85,14 +86,7 @@ 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 (which 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).\n" - + "Please read the Error Prone documentation page for UnsafeLocaleUsage for" - + " more information."); + buildDescription(tree).setMessage("Avoid using the Locale constructors." + DESCRIPTION); fixCallableWithArguments( descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state);