All DateFormats should be constants, not just the simple ones

RELNOTES: SimpleDateFormatConstant now handles all DateFormats, and is renamed to DateFormatConstant ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177409321
google · Dec 1, 2017 · 4796400 · 4796400
1 parent e189e4b
commit 4796400
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 27 deletions.
diff --git a/...bugpatterns/SimpleDateFormatConstant.java → ...prone/bugpatterns/DateFormatConstant.java b/...bugpatterns/SimpleDateFormatConstant.java → ...prone/bugpatterns/DateFormatConstant.java
@@ -21,10 +21,11 @@
 import static com.google.errorprone.fixes.SuggestedFixes.renameVariable;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.isSameType;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
 
 import com.google.common.base.CaseFormat;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.ProvidesFix;
 import com.google.errorprone.BugPattern.StandardTags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
@@ -43,13 +44,14 @@
 
 /** @author cushon@google.com (Liam Miller-Cushon) */
 @BugPattern(
-  name = "SimpleDateFormatConstant",
+  name = "DateFormatConstant",
   category = JDK,
-  summary = "SimpleDateFormat is not thread-safe, and should not be used as a constant field.",
+  summary = "DateFormat is not thread-safe, and should not be used as a constant field.",
   severity = WARNING,
-  tags = StandardTags.FRAGILE_CODE
+  tags = StandardTags.FRAGILE_CODE,
+  providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
 )
-public class SimpleDateFormatConstant extends BugChecker implements VariableTreeMatcher {
+public class DateFormatConstant extends BugChecker implements VariableTreeMatcher {
 
   @Override
   public Description matchVariable(VariableTree tree, VisitorState state) {
@@ -67,7 +69,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
     if (!name.equals(name.toUpperCase())) {
       return NO_MATCH;
     }
-    if (!isSameType(getType(tree), state.getTypeFromString("java.text.SimpleDateFormat"), state)) {
+    if (!isSubtype(getType(tree), state.getTypeFromString("java.text.DateFormat"), state)) {
       return NO_MATCH;
     }
     return buildDescription(tree)

diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -55,6 +55,7 @@
 import com.google.errorprone.bugpatterns.ConstantOverflow;
 import com.google.errorprone.bugpatterns.ConstructorInvokesOverridable;
 import com.google.errorprone.bugpatterns.ConstructorLeaksThis;
+import com.google.errorprone.bugpatterns.DateFormatConstant;
 import com.google.errorprone.bugpatterns.DeadException;
 import com.google.errorprone.bugpatterns.DeadThread;
 import com.google.errorprone.bugpatterns.DefaultCharset;
@@ -183,7 +184,6 @@
 import com.google.errorprone.bugpatterns.SelfEquals;
 import com.google.errorprone.bugpatterns.ShortCircuitBoolean;
 import com.google.errorprone.bugpatterns.ShouldHaveEvenArgs;
-import com.google.errorprone.bugpatterns.SimpleDateFormatConstant;
 import com.google.errorprone.bugpatterns.SizeGreaterThanOrEqualsZero;
 import com.google.errorprone.bugpatterns.StaticQualifiedUsingExpression;
 import com.google.errorprone.bugpatterns.StreamToString;
@@ -500,7 +500,7 @@ public static ScannerSupplier errorChecks() {
           ReferenceEquality.class,
           RequiredModifiersChecker.class,
           ShortCircuitBoolean.class,
-          SimpleDateFormatConstant.class,
+          DateFormatConstant.class,
           StaticGuardedByInstance.class,
           SynchronizeOnNonFinalField.class,
           ThreadJoinLoop.class,

diff --git a/...atterns/SimpleDateFormatConstantTest.java → ...e/bugpatterns/DateFormatConstantTest.java b/...atterns/SimpleDateFormatConstantTest.java → ...e/bugpatterns/DateFormatConstantTest.java
@@ -24,12 +24,30 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** {@link java.text.SimpleDateFormat}Test */
+/** {@link DateFormatConstant}Test */
 @RunWith(JUnit4.class)
-public class SimpleDateFormatConstantTest {
+public class DateFormatConstantTest {
+  @Test
+  public void positive() throws IOException {
+    CompilationTestHelper.newInstance(DateFormatConstant.class, getClass())
+        .addSourceLines(
+            "Test.java",
+            "import java.text.DateFormat;",
+            "import java.text.SimpleDateFormat;",
+            "class Test {",
+            "  // BUG: Diagnostic contains:",
+            "  private static final DateFormat DATE_FORMAT1 =",
+            "    new SimpleDateFormat(\"yyyy-MM-dd HH:mm\");",
+            "  // BUG: Diagnostic contains:",
+            "  private static final SimpleDateFormat DATE_FORMAT2 =",
+            "    new SimpleDateFormat(\"yyyy-MM-dd HH:mm\");",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void negative() throws IOException {
-    CompilationTestHelper.newInstance(SimpleDateFormatConstant.class, getClass())
+    CompilationTestHelper.newInstance(DateFormatConstant.class, getClass())
         .addSourceLines(
             "Test.java",
             "import java.text.SimpleDateFormat;",
@@ -55,13 +73,14 @@ public void negative() throws IOException {
 
   @Test
   public void threadLocalFix() throws IOException {
-    BugCheckerRefactoringTestHelper.newInstance(new SimpleDateFormatConstant(), getClass())
+    BugCheckerRefactoringTestHelper.newInstance(new DateFormatConstant(), getClass())
         .addInputLines(
             "in/Test.java",
             "import java.text.SimpleDateFormat;",
+            "import java.text.DateFormat;",
             "import java.util.Date;",
             "class Test {",
-            "  private static final SimpleDateFormat DATE_FORMAT =",
+            "  private static final DateFormat DATE_FORMAT =",
             "    new SimpleDateFormat(\"yyyy-MM-dd HH:mm\");",
             "  static String f(Date d) {",
             "    return DATE_FORMAT.format(d);",
@@ -70,9 +89,10 @@ public void threadLocalFix() throws IOException {
         .addOutputLines(
             "out/Test.java",
             "import java.text.SimpleDateFormat;",
+            "import java.text.DateFormat;",
             "import java.util.Date;",
             "class Test {",
-            "  private static final ThreadLocal<SimpleDateFormat> DATE_FORMAT = ",
+            "  private static final ThreadLocal<DateFormat> DATE_FORMAT = ",
             "    ThreadLocal.withInitial(() -> new SimpleDateFormat(\"yyyy-MM-dd HH:mm\"));",
             "  static String f(Date d) {",
             "    return DATE_FORMAT.get().format(d);",
@@ -83,13 +103,14 @@ public void threadLocalFix() throws IOException {
 
   @Test
   public void lowerCamelCaseFix() throws IOException {
-    BugCheckerRefactoringTestHelper.newInstance(new SimpleDateFormatConstant(), getClass())
+    BugCheckerRefactoringTestHelper.newInstance(new DateFormatConstant(), getClass())
         .addInputLines(
             "in/Test.java",
             "import java.text.SimpleDateFormat;",
+            "import java.text.DateFormat;",
             "import java.util.Date;",
             "class Test {",
-            "  private static final SimpleDateFormat DATE_FORMAT =",
+            "  private static final DateFormat DATE_FORMAT =",
             "    new SimpleDateFormat(\"yyyy-MM-dd HH:mm\");",
             "  static String f(Date d) {",
             "    return DATE_FORMAT.format(d);",
@@ -98,9 +119,10 @@ public void lowerCamelCaseFix() throws IOException {
         .addOutputLines(
             "out/Test.java",
             "import java.text.SimpleDateFormat;",
+            "import java.text.DateFormat;",
             "import java.util.Date;",
             "class Test {",
-            "  private static final SimpleDateFormat dateFormat =",
+            "  private static final DateFormat dateFormat =",
             "    new SimpleDateFormat(\"yyyy-MM-dd HH:mm\");",
             "  static String f(Date d) {",
             "    return dateFormat.format(d);",

diff --git a/docs/bugpattern/SimpleDateFormatConstant.md → docs/bugpattern/DateFormatConstant.md b/docs/bugpattern/SimpleDateFormatConstant.md → docs/bugpattern/DateFormatConstant.md
@@ -1,21 +1,21 @@
-[`SimpleDateFormat`][] is not thread-safe. The documentation recommendeds
-creating separate format instances for each thread. If multiple threads access a
-format concurrently, it must be synchronized externally.
+[`DateFormat`][] is not thread-safe. The documentation recommendeds creating
+separate format instances for each thread. If multiple threads access a format
+concurrently, it must be synchronized externally.
 
 The [Google Java Style Guide §5.2.4][style] requires `CONSTANT_CASE` to only be
 used for static final fields whose contents are deeply immutable and whose
-methods have no detectable side effects, so fields of type `SimpleDateFormat`
-should not use `CONSTANT_CASE`.
+methods have no detectable side effects, so fields of type `DateFormat` should
+not use `CONSTANT_CASE`.
 
 TIP: Consider using the `java.time` API added in Java8, in particular
-[`DateTimeFormatter`][]. One its many advantages over `SimpleDateFormat` is that
-it is immutable and thread-safe.
+[`DateTimeFormatter`][]. One its many advantages over `DateFormat` is that it is
+immutable and thread-safe.
 
 If the date formatter is accessed by multiple threads, consider using
 [`ThreadLocal`][]:
 
 ```java
-private static final ThreadLocal<SimpleDateFormat> DATE_FORMAT =
+private static final ThreadLocal<DateFormat> DATE_FORMAT =
     ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd HH:mm"));
 
 ```
@@ -25,12 +25,12 @@ If the field is never accessed by multiple threads, rename it to use
 
 ```java
 @NotThreadSafe
-private static final SimpleDateFormat dateFormat =
+private static final DateFormat dateFormat =
     new SimpleDateFormat("yyyy-MM-dd HH:mm");
 
 ```
 
-[`SimpleDateFormat`]: http://docs.oracle.com/javase/8/docs/api/java/text/SimpleDateFormat.html
+[`DateFormat`]: http://docs.oracle.com/javase/8/docs/api/java/text/DateFormat.html
 [`ThreadLocal`]: http://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html
 [`DateTimeFormatter`]: https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html