Skip to content

Update UnsafeLocaleUsage with detection for Locale.of #5279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
+270 −52
Closed

Update UnsafeLocaleUsage with detection for Locale.of #5279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8da2d79
Update UnsafeLocaleUsage with detection for Locale.of
mihnita Oct 19, 2025
b73d248
Fix sample code in UnsafeLocaleUsage.md
mihnita Oct 20, 2025
3cd7fea
Implement feedback from review
mihnita Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 53 additions & 31 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -44,6 +45,8 @@ public final class UnsafeLocaleUsage extends BugChecker

private static final Matcher<ExpressionTree> LOCALE_TO_STRING =
instanceMethod().onExactClass("java.util.Locale").named("toString");
private static final Matcher<ExpressionTree> LOCALE_OF =
staticMethod().onClass("java.util.Locale").named("of");
private static final Matcher<ExpressionTree> LOCALE_CONSTRUCTOR =
constructor().forClass("java.util.Locale");

Expand All @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the message is the BugPattern.summary. Is there information here that applies to all findings that could go in the summary instead?

If you want to customize this to mention e.g. Locale.of vs. Locale constructors, WDYT about refactoring and using a format string to share the text that's the same for both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between constructors and Locale.of there is a lot of sharing (they are basically the same).
I refactored the messages to share, as suggested.

toString throws a wrench in reusing much in the summary.
But while reviewing it I discovered that java.util.Locale is not a library :-), and I fixed that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

+ " 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.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated diagnostics automatically include the link to the full 'explanation', so writing this out here isn't necessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


fixCallableWithArguments(
descriptionBuilder, ImmutableList.copyOf(tree.getArguments()), tree, state);

return descriptionBuilder.build();
}
return Description.NO_MATCH;
}

Expand All @@ -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<ExpressionTree> 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<? extends ExpressionTree> 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)));
}
}
}
153 changes: 151 additions & 2 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnsafeLocaleUsageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -208,7 +338,7 @@ String getLocaleDisplayString() {
}

@Test
public void unsafeLocaleUsageCheck_multipleErrors_shouldFlag() {
public void unsafeLocaleUsageCheck_multipleErrorsWithNew_shouldFlag() {
compilationHelper
.addSourceLines(
"Test.java",
Expand All @@ -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
Expand Down
Loading
Loading