ICU-12717 Improve errorprone reporting

Author: mihnita

icu4j/main/collate/src/test/java/com/ibm/icu/dev/test/collator/CollationMiscTest.java

@@ -2982,6 +2982,7 @@ public void TestMerge() {
 
     /* Test the method public int compareTo(RawCollationKey rhs) */
     @Test
+    @SuppressWarnings("SelfComparison")
     public void TestRawCollationKeyCompareTo(){
         RawCollationKey rck = new RawCollationKey();
         byte[] b = {(byte) 10, (byte) 20};

icu4j/pom.xml

@@ -678,7 +678,8 @@
     </profile>
 
     <profile>
-      <!-- mvn clean test -ntp -DskipTests -DskipITs -l errorprone.log -P errorprone
+      <!-- mvn clean test -ntp -DskipTests -DskipITs -l /tmp/errorprone.log -P errorprone
+           mvn exec:java -f tools/build/ -P errorprone_report -DlogFile=/tmp/errorprone.log
       Then inspect the content of the errorprone.log file.
       We will have some script to convert that to a more friendly format (likely html) -->
       <id>errorprone</id>
@@ -701,7 +702,7 @@
                     So we force them all to be reported as warning.
                     The drawback is that there are not errors now, they get mixed with the real warnings.
                 -->
-                <arg>-Xplugin:ErrorProne -XepAllErrorsAsWarnings</arg>
+                <arg>-Xplugin:ErrorProne -XepAllErrorsAsWarnings -Xep:UnicodeEscape:OFF</arg>
                 <arg>-Xmaxerrs</arg>
                 <arg>10000</arg>
                 <arg>-J-Dfile.encoding=UTF-8</arg>

icu4j/tools/build/pom.xml

@@ -40,7 +40,7 @@
       <!--
       From icu4j:
         mvn clean test -ntp -DskipTests -DskipITs -l /tmp/errorprone.log -P errorprone
-        mvn exec:java -f tools/build/ -P errorprone_report
+        mvn exec:java -f tools/build/ -P errorprone_report -DlogFile=/tmp/errorprone.log
 
       The links to the source files go to the main GitHub repo (`main` branch).
         But when we work locally, or in a fork / branch, the links might point to an incorrect file / line.

icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrorProneReport.java

@@ -17,19 +17,13 @@
 import java.util.Locale;
 import java.util.Map;
 
-import com.google.common.collect.ImmutableBiMap;
-import com.google.errorprone.BugCheckerInfo;
-import com.google.errorprone.scanner.BuiltInCheckerSuppliers;
-
 class ErrorProneReport {
     private static final String HTML_REPORT_FILE = "errorprone1.html";
     private static final String HTML_REPORT_FILE2 = "errorprone2.html";
     private static final String SORTTABLE_JS_FILE = "sorttable.js";
     private static final String SORTTABLE_CSS_FILE = "errorprone.css";
     private static final String [] EMBEDDED_FILES =
         { SORTTABLE_JS_FILE, SORTTABLE_CSS_FILE };
-    private static final ImmutableBiMap<String, BugCheckerInfo> KNOWN_ERRORS =
-            BuiltInCheckerSuppliers.allChecks().getAllChecks();
 
     public static void genReports(String icuDir, String mavenStdOut, String outDir, String baseUrl)
             throws IOException {
@@ -95,6 +89,11 @@ private static void genReport1(Map<String, List<ErrorProneEntry>> errors,
                             ? Map.of("target", "errWin")
                             : Map.of("href", error.url, "target", "errWin");
                     hu.openTag("a", attr).text(error.type).closeTag("a");
+
+                    String tags = ErrorProneUtils.getTags(error.type);
+                    if (tags != null) {
+                        hu.openTag("span", Map.of("class", "tags")).text(" " + tags).closeTag("span");
+                    }
                     hu.closeTag("td");
 
                     outDescription(hu, error);
@@ -145,8 +144,12 @@ private static void genReport2(Map<String, List<ErrorProneEntry>> errors,
                 // "class", "severity_" + errorSeverity)
                 hu.openTag("h3", Map.of("id", "name_" + errorType))
                         .text("[" + firstEntry.severity + "] ")
-                        .openTag("span", Map.of("class", "tag")).text(errorType).closeTag("span")
-                        .text(" [" + errorsOfType.size() + "] ");
+                        .openTag("span", Map.of("class", "tag")).text(errorType).closeTag("span");
+                String tags = ErrorProneUtils.getTags(errorType);
+                if (tags != null) {
+                    hu.openTag("span", Map.of("class", "tags")).text(" " + tags).closeTag("span");
+                }
+                hu.text(" (" + errorsOfType.size() + ") ");
                 String url = ErrorProneUtils.getUrl(errorType);
                 if (url != null) {
                     hu.openTag("a", Map.of("href", url, "target", "errWin"))
@@ -252,8 +255,12 @@ private static void outSummary(HtmlUtils hu, Map<String, List<ErrorProneEntry>>
                     .openTag("span", Map.of("class", "tag"))
                     .text(e.getKey())
                     .closeTag("span")
-                    .closeTag("a")
-                    .text(" [" + e.getValue().size() + "]\n");
+                    .closeTag("a");
+            String tags = ErrorProneUtils.getTags(e.getKey());
+            if (tags != null) {
+                hu.openTag("span", Map.of("class", "tags")).text(" " + tags).closeTag("span");
+            }
+            hu.text(" (" + e.getValue().size() + ")\n");
         }
         if (!first) {
             hu.closeTag("p").nl();

icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrorProneUtils.java

@@ -6,8 +6,8 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
-import com.google.common.collect.ImmutableBiMap;
 import com.google.errorprone.BugCheckerInfo;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.scanner.BuiltInCheckerSuppliers;
@@ -32,7 +32,7 @@ class ErrorProneUtils {
      * So to "get back" the proper severity level we depend on the errorprone library,
      * get all known errors, and then use the `defaultSeverity` field within each `BugCheckerInfo`.
      */
-    private static final ImmutableBiMap<String, BugCheckerInfo> KNOWN_ERRORS =
+    private static final Map<String, BugCheckerInfo> KNOWN_ERRORS =
             BuiltInCheckerSuppliers.allChecks().getAllChecks();
     static final String SEVERITY_UNKNOWN = "UNKNOWN";
     // Some special severity classes, to group what ICU considers important to fix.
@@ -55,10 +55,46 @@ class ErrorProneUtils {
             BugPattern.SeverityLevel.SUGGESTION.toString());
 
     // A special severity class, where we show first what ICU considers important to fix.
-    static final Map<String, String> ICU_SPECIAL_SEVERITIES = Map.of(
+    static final Map<String, String> ICU_SPECIAL_SEVERITIES = Map.ofEntries(
             // Example:
-            // "MissingFail", SEVERITY_ICU_PRI1,
-            // "ReferenceEquality", SEVERITY_ICU_PRI2
+            // Map.entry("MissingFail", SEVERITY_ICU_PRI1),
+            // Map.entry("ReferenceEquality", SEVERITY_ICU_PRI2)
+            Map.entry("DefaultCharset", SEVERITY_ICU_PRI1),
+            Map.entry("Finally", SEVERITY_ICU_PRI1),
+            Map.entry("InvalidBlockTag", SEVERITY_ICU_PRI1),
+            Map.entry("JdkObsolete", SEVERITY_ICU_PRI1),
+            Map.entry("MissingFail", SEVERITY_ICU_PRI1),
+            Map.entry("MutablePublicArray", SEVERITY_ICU_PRI1),
+            Map.entry("ObjectToString", SEVERITY_ICU_PRI1),
+            Map.entry("OperatorPrecedence", SEVERITY_ICU_PRI1),
+            Map.entry("ReferenceEquality", SEVERITY_ICU_PRI1),
+            Map.entry("StringSplitter", SEVERITY_ICU_PRI1),
+            Map.entry("SynchronizeOnNonFinalField", SEVERITY_ICU_PRI1),
+            Map.entry("UnicodeEscape", SEVERITY_ICU_PRI1),
+            Map.entry("UnusedMethod", SEVERITY_ICU_PRI1),
+            Map.entry("UnusedVariable", SEVERITY_ICU_PRI1),
+            Map.entry("AlmostJavadoc", SEVERITY_ICU_PRI2),
+            Map.entry("BadImport", SEVERITY_ICU_PRI2),
+            Map.entry("ClassCanBeStatic", SEVERITY_ICU_PRI2),
+            Map.entry("EmptyCatch", SEVERITY_ICU_PRI2),
+            Map.entry("EqualsGetClass", SEVERITY_ICU_PRI2),
+            Map.entry("EqualsUnsafeCast", SEVERITY_ICU_PRI2),
+            Map.entry("FallThrough", SEVERITY_ICU_PRI2),
+            Map.entry("Finalize", SEVERITY_ICU_PRI2),
+            Map.entry("FloatingPointLiteralPrecision", SEVERITY_ICU_PRI2),
+            Map.entry("IncrementInForLoopAndHeader", SEVERITY_ICU_PRI2),
+            Map.entry("JavaUtilDate", SEVERITY_ICU_PRI2),
+            Map.entry("LabelledBreakTarget", SEVERITY_ICU_PRI2),
+            Map.entry("LockOnNonEnclosingClassLiteral", SEVERITY_ICU_PRI2),
+            Map.entry("NarrowCalculation", SEVERITY_ICU_PRI2),
+            Map.entry("NarrowingCompoundAssignment", SEVERITY_ICU_PRI2),
+            Map.entry("ShortCircuitBoolean", SEVERITY_ICU_PRI2),
+            Map.entry("StaticAssignmentInConstructor", SEVERITY_ICU_PRI2),
+            Map.entry("StringCaseLocaleUsage", SEVERITY_ICU_PRI2),
+            Map.entry("StringCharset", SEVERITY_ICU_PRI2),
+            Map.entry("UndefinedEquals", SEVERITY_ICU_PRI2),
+            Map.entry("UnnecessaryStringBuilder", SEVERITY_ICU_PRI2),
+            Map.entry("UnsynchronizedOverridesSynchronized", SEVERITY_ICU_PRI2)
     );
 
     /**
@@ -90,4 +126,20 @@ static String getUrl(String errorType) {
         BugCheckerInfo found = KNOWN_ERRORS.get(errorType);
         return found == null ? null : found.linkUrl();
     }
+
+    /**
+     * Given an error type (for example `BadImport`, `UnusedVariable`)
+     * it returns the tags associtated with it (`FragileCode`, `Style`, etc.)
+     *
+     * @param errorType the error type, as reported by errorprone
+     * @return the url to a public explanation page
+     */
+    static String getTags(String errorType) {
+        BugCheckerInfo found = KNOWN_ERRORS.get(errorType);
+        if (found == null) {
+            return null;
+        }
+        Set<String> tags = found.getTags();
+        return tags.isEmpty() ? null : tags.toString();
+    }
 }

icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ParseMavenOutForErrorProne.java

@@ -10,17 +10,24 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 class ParseMavenOutForErrorProne {
-	// The `(?:[A-F]:)?` in the beginning is for the Windows drive letter (for example D:)
+    // The `(?:[A-F]:)?` in the beginning is for the Windows drive letter (for example D:)
     private static final String RE_ERROR_PRONE_START =
             "^\\[WARNING\\] ((?:[A-F]:)?[\\\\/a-zA-Z0-9_.\\-]+\\.java):\\[(\\d+),(\\d+)\\]"
                 + " \\[(\\S+)\\] (.+)";
     private static final Pattern PATTERN = Pattern.compile(RE_ERROR_PRONE_START);
 
+    // These are ICU custom tags, but errorprone does not allow us to exclude them.
+    // So we will filter them out in our code.
+    private static final Set<String> CUSTOM_ICU_TAGS = Set.of(
+            "bug", "category", "discouraged", "draft", "icuenhanced", "internal", "icu",
+            "icunote", "obsolete", "provisional", "stable", "summary", "test");
+
     /**
      * The result contains the issues reported by errorprone.
      *
@@ -91,19 +98,44 @@ static Map<String, List<ErrorProneEntry>> parse(String baseDir, String fileName)
         return errorReport;
     }
 
+    private static boolean isCustomIcuTag(ErrorProneEntry crtError) {
+        String errorType = crtError.type;
+        if (errorType.equals("InvalidBlockTag")) {
+            return false;
+        }
+        String message = crtError.message;
+        // We will filter out the custom ICU tags.
+        // There is no programatic way to find the name of the tag, we must extract it from the error message.
+        // Message text 1:
+        // "Tag name `stable` is unknown. If this is a commonly-used custom tag, please click 'not useful' and file a bug."
+        int firstBackTick = message.indexOf('`');
+        if (firstBackTick >= 0) {
+            int secondBackTick = message.indexOf('`', firstBackTick + 1);
+            if (secondBackTick >= 0) {
+                String tagName = message.substring(firstBackTick + 1, secondBackTick);
+                if (CUSTOM_ICU_TAGS.contains(tagName)) {
+                    return true;
+                }
+            }
+        }
+        // Message text 2:
+        // "@summary is not a valid block tag. Should it be an inline tag instead?"
+        if (message.startsWith("@")) {
+            int firstSpace = message.indexOf(' ');
+            if (firstSpace >= 0) {
+                String tagName = message.substring(1, firstSpace);
+                if (CUSTOM_ICU_TAGS.contains(tagName)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     static ErrorProneEntry addErrorToReportAndReset(Map<String, List<ErrorProneEntry>> errorReport,
             ErrorProneEntry crtError) {
-        if (crtError != null) {
-            String errorType = crtError.type;
-            // Fix the severity from parsing, which is never error, to the proper errorprone one
-            crtError.severity = ErrorProneUtils.getErrorLevel(errorType);
-            List<ErrorProneEntry> list = errorReport.computeIfAbsent(
-                    errorType, e -> new ArrayList<ErrorProneEntry>());
-            list.add(crtError);
-            crtError = null;
-        }
         // We want to reset the currentError after we record it.
-        // One errorprone issue can take several lines in the log. 
+        // One errorprone issue can take several lines in the log.
         // The parsing creates currentError when the start of an issue is detected.
         // We add more info to the currentError from the following lines.
         // When we find something that does not look like an errorprone line, or at the end of the
@@ -113,6 +145,18 @@ static ErrorProneEntry addErrorToReportAndReset(Map<String, List<ErrorProneEntry
         // If we return nothing (void method) we would need to do this in the caller (several times):
         //   addErrorToReport(errorReport, currentError); => report
         //   currentError = null; => reset
+        if (crtError == null) {
+            return null;
+        }
+        if (isCustomIcuTag(crtError)) {
+            return null;
+        }
+        // Fix the severity from parsing, which is never error, to the proper errorprone one
+        String errorType = crtError.type;
+        crtError.severity = ErrorProneUtils.getErrorLevel(errorType);
+        List<ErrorProneEntry> list = errorReport.computeIfAbsent(
+                errorType, e -> new ArrayList<ErrorProneEntry>());
+        list.add(crtError);
         return null;
     }
 

icu4j/tools/build/src/main/resources/com/ibm/icu/dev/tool/errorprone/errorprone.css

@@ -39,6 +39,10 @@ table.sortable tbody tr:nth-child(2n+1) td {
   white-space: pre;
 }
 
+.tags {
+  font-size: small;
+}
+
 .severity_warning {
   color:#aa0;
 }