Metalava: Move Issue severity configuration into ErrorConfiguration am: 534a4f8e6b am: 76c0db848d

Change-Id: Icdb7a7094979bb898093513adb77c1f28be41b6e

6 files changed, 144 insertions, 116 deletions

diff --git a/src/main/java/com/android/tools/metalava/Baseline.kt b/src/main/java/com/android/tools/metalava/Baseline.kt
index da91cc1..205448e 100644
--- a/src/main/java/com/android/tools/metalava/Baseline.kt
+++ b/src/main/java/com/android/tools/metalava/Baseline.kt
@@ -265,7 +265,7 @@ class Baseline(
         for (entry in list) {
             val count = entry.value
             val issue = entry.key
-            writer.println("    ${String.format("%5d", count)} ${String.format("%-30s", issue.name)} ${issue.level}")
+            writer.println("    ${String.format("%5d", count)} ${String.format("%-30s", issue.name)} ${configuration.getSeverity(issue)}")
             total += count
         }
         writer.println("" +
diff --git a/src/main/java/com/android/tools/metalava/Options.kt b/src/main/java/com/android/tools/metalava/Options.kt
index 40bcc90..cbc96be 100644
--- a/src/main/java/com/android/tools/metalava/Options.kt
+++ b/src/main/java/com/android/tools/metalava/Options.kt
@@ -20,6 +20,7 @@ import com.android.SdkConstants
 import com.android.sdklib.SdkVersionInfo
 import com.android.tools.metalava.CompatibilityCheck.CheckRequest
 import com.android.tools.metalava.doclava1.Issues
+import com.android.tools.metalava.model.defaultConfiguration
 import com.android.utils.SdkUtils.wrap
 import com.google.common.base.CharMatcher
 import com.google.common.base.Splitter
@@ -30,6 +31,7 @@ import java.io.IOException
 import java.io.OutputStreamWriter
 import java.io.PrintWriter
 import java.io.StringWriter
+import java.lang.NumberFormatException
 import java.util.Locale
 import kotlin.reflect.KMutableProperty1
 import kotlin.reflect.full.memberProperties
@@ -1018,10 +1020,18 @@ class Options(
                     annotationCoverageMemberReport = stringToNewFile(getValue(args, ++index))
                 }
 
-                ARG_ERROR, "-error" -> Issues.setIssueLevel(getValue(args, ++index), Severity.ERROR, true)
-                ARG_WARNING, "-warning" -> Issues.setIssueLevel(getValue(args, ++index), Severity.WARNING, true)
-                ARG_LINT, "-lint" -> Issues.setIssueLevel(getValue(args, ++index), Severity.LINT, true)
-                ARG_HIDE, "-hide" -> Issues.setIssueLevel(getValue(args, ++index), Severity.HIDDEN, true)
+                ARG_ERROR, "-error" -> setIssueSeverity(
+                    getValue(args, ++index),
+                    Severity.ERROR,
+                    arg
+                )
+                ARG_WARNING, "-warning" -> setIssueSeverity(
+                    getValue(args, ++index),
+                    Severity.WARNING,
+                    arg
+                )
+                ARG_LINT, "-lint" -> setIssueSeverity(getValue(args, ++index), Severity.LINT, arg)
+                ARG_HIDE, "-hide" -> setIssueSeverity(getValue(args, ++index), Severity.HIDDEN, arg)
 
                 ARG_WARNINGS_AS_ERRORS -> warningsAreErrors = true
                 ARG_LINTS_AS_ERRORS -> lintsAreErrors = true
@@ -2258,5 +2268,41 @@ class Options(
             return COMPAT_MODE_BY_DEFAULT && !args.contains("$ARG_COMPAT_OUTPUT=no") &&
                 (args.none { it.startsWith("$ARG_FORMAT=") } || args.contains("--format=v1"))
         }
+
+        private fun setIssueSeverity(
+            id: String,
+            severity: Severity,
+            arg: String
+        ) {
+            if (id.contains(",")) { // Handle being passed in multiple comma separated id's
+                id.split(",").forEach {
+                    setIssueSeverity(it.trim(), severity, arg)
+                }
+                return
+            }
+
+            val numericId = try {
+                id.toInt()
+            } catch (e: NumberFormatException) {
+                -1
+            }
+
+            val issue = Issues.findIssueById(id)
+                ?: Issues.findIssueById(numericId)?.also {
+                    reporter.report(
+                        Issues.DEPRECATED_OPTION, null as File?,
+                        "Issue lookup by numeric id is deprecated, use " +
+                            "$arg ${it.name} instead of $arg $id"
+                    )
+                } ?: Issues.findIssueByIdIgnoringCase(id)?.also {
+                    reporter.report(
+                        Issues.DEPRECATED_OPTION, null as File?,
+                        "Case-insensitive issue matching is deprecated, use " +
+                            "$arg ${it.name} instead of $arg $id"
+                    )
+                } ?: throw DriverException("Unknown issue id: $arg $id")
+
+            defaultConfiguration.setSeverity(issue, severity)
+        }
     }
 }
diff --git a/src/main/java/com/android/tools/metalava/doclava1/Issues.java b/src/main/java/com/android/tools/metalava/doclava1/Issues.java
index 33ff0a6..c1aefd8 100644
--- a/src/main/java/com/android/tools/metalava/doclava1/Issues.java
+++ b/src/main/java/com/android/tools/metalava/doclava1/Issues.java
@@ -16,7 +16,6 @@
 package com.android.tools.metalava.doclava1;
 
 import com.android.tools.metalava.Severity;
-import com.google.common.base.Splitter;
 import org.jetbrains.annotations.Nullable;
 
 import java.lang.reflect.Field;
@@ -35,9 +34,7 @@ public class Issues {
         @Nullable
         String fieldName;
 
-        private Severity level;
-        private final Severity defaultLevel;
-        boolean setByUser;
+        public final Severity defaultLevel;
 
         /**
          * The name of this issue if known
@@ -49,7 +46,7 @@ public class Issues {
          * When {@code level} is set to {@link Severity#INHERIT}, this is the parent from
          * which the issue will inherit its level.
          */
-        private final Issue parent;
+        public final Issue parent;
 
         /** Related rule, if any */
         public final String rule;
@@ -60,27 +57,26 @@ public class Issues {
         /** Applicable category */
         public final Category category;
 
-        private Issue(int code, Severity level) {
-            this(code, level, Category.UNKNOWN);
+        private Issue(int code, Severity defaultLevel) {
+            this(code, defaultLevel, Category.UNKNOWN);
         }
 
-        private Issue(int code, Severity level, Category category) {
-            this(code, level, null, category, null, null);
+        private Issue(int code, Severity defaultLevel, Category category) {
+            this(code, defaultLevel, null, category, null, null);
         }
 
-        private Issue(int code, Severity level, Category category, String rule) {
-            this(code, level, null, category, rule, null);
+        private Issue(int code, Severity defaultLevel, Category category, String rule) {
+            this(code, defaultLevel, null, category, rule, null);
         }
 
         private Issue(int code, Issue parent, Category category) {
             this(code, Severity.INHERIT, parent, category, null, null);
         }
 
-        private Issue(int code, Severity level, Issue parent, Category category,
+        private Issue(int code, Severity defaultLevel, Issue parent, Category category,
                       String rule, String explanation) {
             this.code = code;
-            this.level = level;
-            this.defaultLevel = level;
+            this.defaultLevel = defaultLevel;
             this.parent = parent;
             this.category = category;
             this.rule = rule;
@@ -88,53 +84,6 @@ public class Issues {
             ISSUES.add(this);
         }
 
-        /**
-         * Returns the implied level for this issue.
-         * <p>
-         * If the level is {@link Severity#INHERIT}, the level will be returned for the
-         * parent.
-         *
-         * @throws IllegalStateException if the level is {@link Severity#INHERIT} and the
-         *                               parent is {@code null}
-         */
-        public Severity getLevel() {
-            if (level == Severity.INHERIT) {
-                if (parent == null) {
-                    throw new IllegalStateException("Issue with level INHERIT must have non-null parent");
-                }
-                return parent.getLevel();
-            }
-            return level;
-        }
-
-        public boolean isInherit() {
-            return level == Severity.INHERIT;
-        }
-
-        public Issue getParent() {
-            return parent;
-        }
-
-        /**
-         * Sets the level.
-         * <p>
-         * Valid arguments are:
-         * <ul>
-         * <li>{@link Severity#HIDDEN}
-         * <li>{@link Severity#WARNING}
-         * <li>{@link Severity#ERROR}
-         * </ul>
-         *
-         * @param level the level to set
-         */
-        void setLevel(Severity level) {
-            if (level == Severity.INHERIT) {
-                throw new IllegalArgumentException("Issue level may not be set to INHERIT");
-            }
-            this.level = level;
-            this.setByUser = true;
-        }
-
         public String toString() {
             return "Issue #" + this.code + " (" + this.name + ")";
         }
@@ -383,51 +332,14 @@ public class Issues {
         return nameToIssue.get(id);
     }
 
-    public static boolean setIssueLevel(String id, Severity level, boolean setByUser) {
-        if (id.contains(",")) { // Handle being passed in multiple comma separated id's
-            boolean ok = true;
-            for (String individualId : Splitter.on(',').trimResults().split(id)) {
-                ok = setIssueLevel(individualId, level, setByUser) && ok;
-            }
-            return ok;
-        }
-        int code = -1;
-        if (Character.isDigit(id.charAt(0))) {
-            code = Integer.parseInt(id);
-        }
-
-        Issue issue = nameToIssue.get(id);
-        if (issue == null) {
-            try {
-                int n = Integer.parseInt(id);
-                issue = idToIssue.get(n);
-            } catch (NumberFormatException ignore) {
-            }
-        }
-
-        if (issue == null) {
-            for (Issue e : ISSUES) {
-                if (e.code == code || id.equalsIgnoreCase(e.name)) {
-                    issue = e;
-                    break;
-                }
+    @Nullable
+    public static Issue findIssueByIdIgnoringCase(String id) {
+        for (Issue e : ISSUES) {
+            if (id.equalsIgnoreCase(e.name)) {
+                return e;
             }
         }
-
-        if (issue != null) {
-            issue.setLevel(level);
-            issue.setByUser = setByUser;
-            return true;
-        }
-        return false;
-    }
-
-    // Primary needed by unit tests; ensure that a previous test doesn't influence
-    // a later one
-    public static void resetLevels() {
-        for (Issue issue : ISSUES) {
-            issue.level = issue.defaultLevel;
-        }
+        return null;
     }
 }
 
diff --git a/src/main/java/com/android/tools/metalava/model/ErrorConfiguration.kt b/src/main/java/com/android/tools/metalava/model/ErrorConfiguration.kt
index 64eb74e..bac176c 100644
--- a/src/main/java/com/android/tools/metalava/model/ErrorConfiguration.kt
+++ b/src/main/java/com/android/tools/metalava/model/ErrorConfiguration.kt
@@ -26,15 +26,14 @@ class ErrorConfiguration {
     /** Returns the severity of the given issue */
     fun getSeverity(issue: Issues.Issue): Severity {
         overrides[issue]?.let { return it }
-        // TODO(b/153446763): We cannot just use the inherit logic in issue.level because that
-        //  doesn't know if the parent has an override applied.
-        if (issue.isInherit) {
+        if (issue.defaultLevel == Severity.INHERIT) {
             return getSeverity(issue.parent)
         }
-        return issue.level
+        return issue.defaultLevel
     }
 
-    private fun setSeverity(issue: Issues.Issue, severity: Severity) {
+    fun setSeverity(issue: Issues.Issue, severity: Severity) {
+        check(severity != Severity.INHERIT)
         overrides[issue] = severity
     }
 
@@ -47,6 +46,10 @@ class ErrorConfiguration {
     fun hide(issue: Issues.Issue) {
         setSeverity(issue, Severity.HIDDEN)
     }
+
+    fun reset() {
+        overrides.clear()
+    }
 }
 
 /** Default error configuration: uses all the severities initialized in the [Issues] class */
diff --git a/src/test/java/com/android/tools/metalava/DriverTest.kt b/src/test/java/com/android/tools/metalava/DriverTest.kt
index 556bc41..6a022b9 100644
--- a/src/test/java/com/android/tools/metalava/DriverTest.kt
+++ b/src/test/java/com/android/tools/metalava/DriverTest.kt
@@ -31,8 +31,8 @@ import com.android.tools.lint.checks.infrastructure.TestFiles
 import com.android.tools.lint.checks.infrastructure.TestFiles.java
 import com.android.tools.lint.checks.infrastructure.stripComments
 import com.android.tools.metalava.doclava1.ApiFile
-import com.android.tools.metalava.doclava1.Issues
 import com.android.tools.metalava.model.SUPPORT_TYPE_USE_ANNOTATIONS
+import com.android.tools.metalava.model.defaultConfiguration
 import com.android.tools.metalava.model.parseDocument
 import com.android.utils.FileUtils
 import com.android.utils.SdkUtils
@@ -424,7 +424,7 @@ abstract class DriverTest {
                 "Can't specify both compatibilityMode and mergeInclusionAnnotations"
             )
         }
-        Issues.resetLevels()
+        defaultConfiguration.reset()
 
         @Suppress("NAME_SHADOWING")
         val expectedFail = expectedFail ?: if ((checkCompatibilityApi != null ||
diff --git a/src/test/java/com/android/tools/metalava/OptionsTest.kt b/src/test/java/com/android/tools/metalava/OptionsTest.kt
index 8b585eb..ed7c4e6 100644
--- a/src/test/java/com/android/tools/metalava/OptionsTest.kt
+++ b/src/test/java/com/android/tools/metalava/OptionsTest.kt
@@ -16,6 +16,8 @@
 
 package com.android.tools.metalava
 
+import com.android.tools.metalava.doclava1.Issues
+import com.android.tools.metalava.model.defaultConfiguration
 import org.junit.Assert.assertEquals
 import org.junit.Test
 import java.io.PrintWriter
@@ -438,4 +440,69 @@ $FLAGS
 """.trimIndent(), stdout.toString()
         )
     }
+
+    @Test
+    fun `Test issue severity options`() {
+        check(
+            extraArguments = arrayOf(
+                "--hide",
+                "StartWithLower",
+                "--lint",
+                "EndsWithImpl",
+                "--warning",
+                "StartWithUpper",
+                "--error",
+                "ArrayReturn"
+            )
+        )
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.START_WITH_LOWER))
+        assertEquals(Severity.LINT, defaultConfiguration.getSeverity(Issues.ENDS_WITH_IMPL))
+        assertEquals(Severity.WARNING, defaultConfiguration.getSeverity(Issues.START_WITH_UPPER))
+        assertEquals(Severity.ERROR, defaultConfiguration.getSeverity(Issues.ARRAY_RETURN))
+    }
+
+    @Test
+    fun `Test multiple issue severity options`() {
+        check(
+            extraArguments = arrayOf("--hide", "StartWithLower,StartWithUpper,ArrayReturn")
+        )
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.START_WITH_LOWER))
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.START_WITH_UPPER))
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.ARRAY_RETURN))
+    }
+
+    @Test
+    fun `Test issue severity options with inheriting issues`() {
+        check(
+            extraArguments = arrayOf("--error", "RemovedClass")
+        )
+        assertEquals(Severity.ERROR, defaultConfiguration.getSeverity(Issues.REMOVED_CLASS))
+        assertEquals(Severity.ERROR, defaultConfiguration.getSeverity(Issues.REMOVED_DEPRECATED_CLASS))
+    }
+
+    @Test
+    fun `Test issue severity options by numeric id`() {
+        check(
+            extraArguments = arrayOf("--hide", "366"),
+            warnings = "warning: Issue lookup by numeric id is deprecated, use --hide ArrayReturn instead of --hide 366 [DeprecatedOption]"
+        )
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.ARRAY_RETURN))
+    }
+
+    @Test
+    fun `Test issue severity options with case insensitive names`() {
+        check(
+            extraArguments = arrayOf("--hide", "arrayreturn"),
+            warnings = "warning: Case-insensitive issue matching is deprecated, use --hide ArrayReturn instead of --hide arrayreturn [DeprecatedOption]"
+        )
+        assertEquals(Severity.HIDDEN, defaultConfiguration.getSeverity(Issues.ARRAY_RETURN))
+    }
+
+    @Test
+    fun `Test issue severity options with non-existing issue`() {
+        check(
+            extraArguments = arrayOf("--hide", "ThisIssueDoesNotExist"),
+            expectedFail = "Unknown issue id: --hide ThisIssueDoesNotExist"
+        )
+    }
 }