Update UnsafeLocaleUsage with detection for Locale.of

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,70 +38,85 @@
 
 /** 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<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");
 
+  // 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)) {
       return buildDescription(tree)
           .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;
   }
 
   @Override
   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<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)));
+    }
+  }
 }

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