From 43a6c45b20acb4395913eeb152b1d06e28c10022 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Wed, 27 Sep 2023 11:33:38 -0700 Subject: [PATCH] An Error Prone checker for duplicated DateFormat fields. Ignores fields in optional groups. PiperOrigin-RevId: 568912310 --- .../bugpatterns/DuplicateDateFormatField.java | 116 +++++++++++ .../bugpatterns/MisusedDateFormat.java | 3 +- .../scanner/BuiltInCheckerSuppliers.java | 2 + .../DuplicateDateFormatFieldTest.java | 190 ++++++++++++++++++ 4 files changed, 309 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/DuplicateDateFormatField.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/DuplicateDateFormatFieldTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateDateFormatField.java b/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateDateFormatField.java new file mode 100644 index 00000000000..d641a16e2fe --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DuplicateDateFormatField.java @@ -0,0 +1,116 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.constValue; +import static java.util.stream.Collectors.joining; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; +import javax.annotation.Nullable; + +/** Flag DateFormats which use the same field more than once. */ +@BugPattern(summary = "Reuse of DateFormat fields is most likely unintentional", severity = WARNING) +public final class DuplicateDateFormatField extends MisusedDateFormat { + private static final ImmutableSet PATTERN_CHARACTERS = + ImmutableSet.of( + 'G', 'y', 'Y', 'M', 'L', 'w', 'W', 'D', 'd', 'F', 'E', 'u', 'a', 'H', 'k', 'K', 'h', 'm', + 's', 'S', 'z', 'Z', 'X'); + + private static class PatternCounter implements DateFormatConsumer { + + private final Set seen = new HashSet<>(); + private final Set duplicates = new HashSet<>(); + @Nullable private Character prev = null; + private int optionalGroupDepth = 0; + + @Override + public void consumeSpecial(char special) { + if (special == '[') { + optionalGroupDepth++; + } else if (special == ']') { + optionalGroupDepth--; + } + if (!PATTERN_CHARACTERS.contains(special) || optionalGroupDepth > 0) { + prev = null; + return; + } + if (prev == null || prev != special) { + if (!seen.add(special)) { + duplicates.add(special); + } + prev = special; + } + } + + @Override + public void consumeLiteral(char literal) { + prev = null; + } + + public Set getDuplicates() { + return duplicates; + } + + public static ImmutableSet getDuplicates(String pattern) { + PatternCounter counter = new PatternCounter(); + parseDateFormat(pattern, counter); + return ImmutableSet.copyOf(counter.getDuplicates()); + } + } + + @Override + public Optional rewriteTo(String pattern) { + return Optional.empty(); + } + + @Override + Description constructDescription(Tree tree, ExpressionTree patternArg, VisitorState state) { + + Optional pattern = Optional.ofNullable(constValue(patternArg, String.class)); + if (pattern.isEmpty()) { + return NO_MATCH; + } + ImmutableSet duplicates = PatternCounter.getDuplicates(pattern.get()); + if (!duplicates.isEmpty()) { + return buildDescription(tree).setMessage(buildMessage(pattern.get(), duplicates)).build(); + } + return NO_MATCH; + } + + private static String buildMessage(String pattern, ImmutableSet duplicates) { + String duplicatedFields = + duplicates.stream().sorted().map(c -> "'" + c + "'").collect(joining(", ")); + String fieldDescription = + duplicates.size() > 1 + ? String.format("the fields [%s]", duplicatedFields) + : String.format("the field %s", duplicatedFields); + return String.format( + "DateFormat pattern \"%s\" uses %s more than once. Reuse of DateFormat fields is most" + + " likely unintentional.", + pattern, fieldDescription); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MisusedDateFormat.java b/core/src/main/java/com/google/errorprone/bugpatterns/MisusedDateFormat.java index fb91251de40..96443b44004 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MisusedDateFormat.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MisusedDateFormat.java @@ -114,8 +114,7 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { */ abstract Optional rewriteTo(String pattern); - private Description constructDescription( - Tree tree, ExpressionTree patternArg, VisitorState state) { + Description constructDescription(Tree tree, ExpressionTree patternArg, VisitorState state) { return Optional.ofNullable(constValue(patternArg, String.class)) .flatMap(this::rewriteTo) .map(replacement -> describeMatch(tree, replaceArgument(patternArg, replacement, state))) diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 0fa69f50b5b..f21326b3697 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -117,6 +117,7 @@ import com.google.errorprone.bugpatterns.DoNotMockChecker; import com.google.errorprone.bugpatterns.DoNotUseRuleChain; import com.google.errorprone.bugpatterns.DoubleBraceInitialization; +import com.google.errorprone.bugpatterns.DuplicateDateFormatField; import com.google.errorprone.bugpatterns.DuplicateMapKeys; import com.google.errorprone.bugpatterns.EmptyCatch; import com.google.errorprone.bugpatterns.EmptyIfStatement; @@ -874,6 +875,7 @@ public static ScannerSupplier warningChecks() { DoNotClaimAnnotations.class, DoNotMockAutoValue.class, DoubleCheckedLocking.class, + DuplicateDateFormatField.class, EmptyBlockTag.class, EmptyCatch.class, EmptySetMultibindingContributions.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateDateFormatFieldTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateDateFormatFieldTest.java new file mode 100644 index 00000000000..e0806a3094e --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DuplicateDateFormatFieldTest.java @@ -0,0 +1,190 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class DuplicateDateFormatFieldTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(DuplicateDateFormatField.class, getClass()); + + @Test + public void singleDuplicateField() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " SimpleDateFormat format = new SimpleDateFormat(\"mm/dd/yyyy hh:mm:ss\");", + "}") + .doTest(); + } + + @Test + public void doubleDuplicateFields() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " // BUG: Diagnostic contains: uses the fields ['m', 's'] more than once", + " SimpleDateFormat format = new SimpleDateFormat(\"mm/dd/yyyy" + " hh:mm:ss.sss\");", + "}") + .doTest(); + } + + @Test + public void constantWithDuplicateField() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " static final String PATTERN = \"mm/dd/yyyy hh:mm:ss\";", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " SimpleDateFormat format = new SimpleDateFormat(PATTERN);", + "}") + .doTest(); + } + + @Test + public void recognizedDateTimeFormat() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.time.format.DateTimeFormatter;", + "class Test {", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " DateTimeFormatter formatter = DateTimeFormatter.ofPattern(\"mm/dd/yyyy hh:mm:ss\");", + "}") + .doTest(); + } + + @Test + public void simpleDateFormat_applyPattern() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " public void foo() {", + " SimpleDateFormat format = new SimpleDateFormat();", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " format.applyPattern(\"mm/dd/yyyy hh:mm:ss\");", + " }", + "}") + .doTest(); + } + + @Test + public void simpleDateFormat_applyLocalizedPattern() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " public void foo() {", + " SimpleDateFormat format = new SimpleDateFormat();", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " format.applyLocalizedPattern(\"mm/dd/yyyy hh:mm:ss\");", + " }", + "}") + .doTest(); + } + + @Test + public void forgotToEscapteSpecialCharacters() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " // BUG: Diagnostic contains: uses the field 'W' more than once", + " SimpleDateFormat format = new SimpleDateFormat(\"Week W ' of ' L\");", + "}") + .doTest(); + } + + @Test + public void withOptionalGroup() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " SimpleDateFormat format = new SimpleDateFormat(\"hh:mm[:ss] yyyy/mm/dd\");", + "}") + .doTest(); + } + + @Test + public void withNeestedOptionalGroup() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " // BUG: Diagnostic contains: uses the field 'm' more than once", + " SimpleDateFormat format = new SimpleDateFormat(\"hh:mm[:ss[.SSS]] yyyy/mm/dd\");", + "}") + .doTest(); + } + + @Test + public void noDupliatedFields() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " SimpleDateFormat format = new SimpleDateFormat(\"yyyy-MM-dd\");", + "}") + .doTest(); + } + + @Test + public void ignoresEscapedPatternCharacters() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " SimpleDateFormat format = new SimpleDateFormat(\"'Week' W ' of ' L\");", + "}") + .doTest(); + } + + @Test + public void ignoredOptionalGroups() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.text.SimpleDateFormat;", + "class Test {", + " SimpleDateFormat format = ", + " new SimpleDateFormat(\"yyyy'-'MM'-'dd'T'HH':'mm[':'ss][XXX][X]\");", + "}") + .doTest(); + } +}