Disable the check by default, make it allow args castable to Formatta…

…ble and only fail when they're known to be Formattables
google · Jul 23, 2024 · 102a28f · 102a28f
1 parent 2b7813d
commit 102a28f
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 7 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DefaultLocale.java b/core/src/main/java/com/google/errorprone/bugpatterns/DefaultLocale.java
@@ -461,6 +461,7 @@ private Description handleMessageFormatFormat(MethodInvocationTree tree, Visitor
   private boolean shouldRefactorStringFormat(
       ExpressionTree pattern, List<? extends ExpressionTree> arguments, VisitorState state) {
     String patternValue = ASTHelpers.constValue(pattern, String.class);
+    // TODO: add a flag to be stricter and reformat whenever the pattern is not a constant
     if (patternValue != null && !onlyContainsSpecifiersInAllowList(patternValue)) {
       return true;
     }
@@ -484,8 +485,9 @@ private boolean mightBeFormattable(ExpressionTree tree, VisitorState state) {
     if (tree instanceof LiteralTree) {
       return false;
     }
-    var type = ASTHelpers.getResultType(tree);
-    return type == null || ASTHelpers.isCastable(type, FORMATTABLE.get(state), state);
+    // TODO: add a flag to be stricter and detect any argument that could be cast to Formattable
+    //       (rather than only the ones that are proven to be Formattable)
+    return ASTHelpers.isSubtype(ASTHelpers.getResultType(tree), FORMATTABLE.get(state), state);
   }
 
   private Fix messageFormatFormatFix(

diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -890,7 +890,6 @@ public static ScannerSupplier warningChecks() {
           DateFormatConstant.class,
           DeeplyNested.class,
           DefaultCharset.class,
-          DefaultLocale.class,
           DefaultPackage.class,
           DeprecatedVariable.class,
           DirectInvocationOnMock.class,
@@ -1145,6 +1144,7 @@ public static ScannerSupplier warningChecks() {
           ConstantField.class,
           ConstantPatternCompile.class,
           DeduplicateConstants.class,
+          DefaultLocale.class, // TODO: enable this by default.
           DepAnn.class,
           DifferentNameButSame.class,
           DoNotUseRuleChain.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DefaultLocaleTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DefaultLocaleTest.java
@@ -76,12 +76,17 @@ public void formatMethods() {
             "Test.java",
             "import java.io.*;",
             "import java.text.*;",
-            "import java.util.Formatter;",
+            "import java.util.Formattable;",
             "class Test {",
             "  static final String PATTERN = \"%d\";",
-            "  void f(PrintStream ps, PrintWriter pw) throws Exception {",
+            "  static abstract class F implements Formattable {};",
+            "  void f(PrintStream ps, PrintWriter pw, String pattern, F formattable) throws Exception {",
             "    // BUG: Diagnostic contains: ps.format(Locale.getDefault(FORMAT), PATTERN, 42);",
             "    ps.format(PATTERN, 42);",
+            "    // BUG: Diagnostic contains: ps.format(Locale.getDefault(FORMAT), \"%s\", formattable);",
+            "    ps.format(\"%s\", formattable);",
+            "    // BUG: Diagnostic contains: ps.format(Locale.getDefault(FORMAT), pattern, formattable);",
+            "    ps.format(pattern, formattable);",
             "    // BUG: Diagnostic contains: ps.format(Locale.getDefault(FORMAT), \"%d\", 42);",
             "    ps.format(\"%d\", 42);",
             "    // BUG: Diagnostic contains: ps.printf(Locale.getDefault(FORMAT), \"%d\", 42);",
@@ -109,7 +114,7 @@ public void formatMethods_negative() {
             "import java.util.Formattable;",
             "class Test {",
             "  static final String PATTERN = \"%s\";",
-            "  final class B {}",
+            "  final static class B {}",
             "  void f(String s, int i, B b) throws Exception {",
             // On System.out and System.err
             "    System.out.format(\"%d\", 42);",
@@ -124,6 +129,8 @@ public void formatMethods_negative() {
             "    String.format(\"%c\", i);",
             "    String.format(\"%1$s %<h %<b\", b);",
             "    MessageFormat.format(\"%s\", s);",
+            // Let non-constants pass
+            "    String.format(s, 42);",
             "  }",
             "}")
         .doTest();
@@ -303,7 +310,6 @@ public void formatConstructors() {
             "Test.java",
             "import java.io.*;",
             "import java.text.*;",
-            "import java.util.Formatter;",
             "class Test {",
             "  void f() throws Exception {",
             "    // BUG: Diagnostic contains: new MessageFormat(\"%d\", Locale.getDefault(FORMAT));",