Merge "Binary compatibility tests for interface methods"
diff --git a/src/main/java/com/android/tools/metalava/ApiLint.kt b/src/main/java/com/android/tools/metalava/ApiLint.kt
index b57cdde..7586d57 100644
--- a/src/main/java/com/android/tools/metalava/ApiLint.kt
+++ b/src/main/java/com/android/tools/metalava/ApiLint.kt
@@ -80,7 +80,6 @@
import com.android.tools.metalava.Issues.GETTER_ON_BUILDER
import com.android.tools.metalava.Issues.GETTER_SETTER_NAMES
import com.android.tools.metalava.Issues.HEAVY_BIT_SET
-import com.android.tools.metalava.Issues.ILLEGAL_STATE_EXCEPTION
import com.android.tools.metalava.Issues.INTENT_BUILDER_NAME
import com.android.tools.metalava.Issues.INTENT_NAME
import com.android.tools.metalava.Issues.INTERFACE_CONSTANT
@@ -154,7 +153,6 @@
import com.android.tools.metalava.model.ParameterItem
import com.android.tools.metalava.model.SetMinSdkVersion
import com.android.tools.metalava.model.TypeItem
-import com.android.tools.metalava.model.configuration
import com.android.tools.metalava.model.psi.PsiMethodItem
import com.android.tools.metalava.model.visitors.ApiVisitor
import com.intellij.psi.JavaRecursiveElementVisitor
@@ -1442,10 +1440,7 @@
private fun checkExceptions(method: MethodItem, filterReference: Predicate<Item>) {
for (exception in method.filteredThrowsTypes(filterReference)) {
- if (
- isUncheckedException(exception) &&
- configuration.getSeverity(BANNED_THROW) != Severity.HIDDEN
- ) {
+ if (isUncheckedException(exception)) {
report(
BANNED_THROW, method,
"Methods must not throw unchecked exceptions"
@@ -1475,15 +1470,6 @@
}
}
}
- "java.lang.IllegalArgumentException",
- "java.lang.NullPointerException" -> {
- if (method.parameters().isEmpty()) {
- report(
- ILLEGAL_STATE_EXCEPTION, method,
- "Methods taking no arguments should throw `IllegalStateException` instead of `$qualifiedName`"
- )
- }
- }
}
}
}
diff --git a/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt b/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
index e9617b8..2ed0b19 100644
--- a/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
+++ b/src/main/java/com/android/tools/metalava/CompatibilityCheck.kt
@@ -653,13 +653,18 @@
val oldVisibility = oldModifiers.getVisibilityString()
val newVisibility = newModifiers.getVisibilityString()
if (oldVisibility != newVisibility) {
- // TODO: Use newModifiers.asAccessibleAs(oldModifiers) to provide different error messages
- // based on whether this seems like a reasonable change, e.g. making a private or final method more
- // accessible is fine (no overridden method affected) but not making methods less accessible etc
- report(
- Issues.CHANGED_SCOPE, new,
- "${describe(new, capitalize = true)} changed visibility from $oldVisibility to $newVisibility"
- )
+ // Only report issue if the change is a decrease in access; e.g. public -> protected
+ if (!newModifiers.asAccessibleAs(oldModifiers)) {
+ report(
+ Issues.CHANGED_SCOPE, new,
+ "${
+ describe(
+ new,
+ capitalize = true
+ )
+ } changed visibility from $oldVisibility to $newVisibility"
+ )
+ }
}
if (oldModifiers.isStatic() != newModifiers.isStatic()) {
@@ -678,12 +683,6 @@
)
}
- if (oldModifiers.isTransient() != newModifiers.isTransient()) {
- report(
- Issues.CHANGED_TRANSIENT, new, "${describe(new, capitalize = true)} has changed 'transient' qualifier"
- )
- }
-
if (oldModifiers.isVolatile() != newModifiers.isVolatile()) {
report(
Issues.CHANGED_VOLATILE, new, "${describe(new, capitalize = true)} has changed 'volatile' qualifier"
diff --git a/src/test/java/com/android/tools/metalava/ApiLintTest.kt b/src/test/java/com/android/tools/metalava/ApiLintTest.kt
index 0ebd514..2ca80e3 100644
--- a/src/test/java/com/android/tools/metalava/ApiLintTest.kt
+++ b/src/test/java/com/android/tools/metalava/ApiLintTest.kt
@@ -1636,8 +1636,6 @@
src/android/pkg/MyClass.java:6: error: Methods must not throw generic exceptions (`java.lang.Exception`) [GenericException] [See https://s.android.com/api-guidelines#appropriate-exception]
src/android/pkg/MyClass.java:7: error: Methods must not throw generic exceptions (`java.lang.Throwable`) [GenericException] [See https://s.android.com/api-guidelines#appropriate-exception]
src/android/pkg/MyClass.java:8: error: Methods must not throw generic exceptions (`java.lang.Error`) [GenericException] [See https://s.android.com/api-guidelines#appropriate-exception]
- src/android/pkg/MyClass.java:9: warning: Methods taking no arguments should throw `IllegalStateException` instead of `java.lang.IllegalArgumentException` [IllegalStateException] [See https://s.android.com/api-guidelines#appropriate-exception]
- src/android/pkg/MyClass.java:10: warning: Methods taking no arguments should throw `IllegalStateException` instead of `java.lang.NullPointerException` [IllegalStateException] [See https://s.android.com/api-guidelines#appropriate-exception]
src/android/pkg/MyClass.java:11: error: Methods calling system APIs should rethrow `RemoteException` as `RuntimeException` (but do not list it in the throws clause) [RethrowRemoteException] [See https://s.android.com/api-guidelines#appropriate-exception]
""",
expectedFail = DefaultLintErrorMessage,
@@ -2193,32 +2191,6 @@
}
@Test
- fun `Check for banned runtime exceptions`() {
- check(
- apiLint = "", // enabled
- expectedIssues = """
- src/android/pkg/MyClass.java:6: error: Methods must not throw unchecked exceptions [BannedThrow]
- src/android/pkg/MyClass.java:7: error: Methods must not throw unchecked exceptions [BannedThrow]
- """,
- expectedFail = DefaultLintErrorMessage,
- sourceFiles = arrayOf(
- java(
- """
- package android.pkg;
-
- public class MyClass {
- private MyClass() throws NullPointerException {} // OK, private
- @SuppressWarnings("RedundantThrows") public MyClass(int i) throws java.io.IOException {} // OK, not runtime exception
- public MyClass(double l) throws ClassCastException {} // error
- public void foo() throws SecurityException {} // error
- }
- """
- )
- )
- )
- }
-
- @Test
fun `Check for extending errors`() {
check(
apiLint = "", // enabled
diff --git a/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt b/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
index 677b429..4811a7a 100644
--- a/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
+++ b/src/test/java/com/android/tools/metalava/CompatibilityCheckTest.kt
@@ -787,7 +787,6 @@
src/test/pkg/Parent.java:6: error: Field test.pkg.Parent.field3 has changed type from int to char [ChangedType]
src/test/pkg/Parent.java:7: error: Field test.pkg.Parent.field4 has added 'final' qualifier [AddedFinal]
src/test/pkg/Parent.java:8: error: Field test.pkg.Parent.field5 has changed 'static' qualifier [ChangedStatic]
- src/test/pkg/Parent.java:9: error: Field test.pkg.Parent.field6 has changed 'transient' qualifier [ChangedTransient]
src/test/pkg/Parent.java:10: error: Field test.pkg.Parent.field7 has changed 'volatile' qualifier [ChangedVolatile]
src/test/pkg/Parent.java:20: error: Field test.pkg.Parent.field94 has changed value from 1 to 42 [ChangedValue]
""",
@@ -1341,7 +1340,6 @@
fun `Incompatible field change -- visibility and removing final`() {
check(
expectedIssues = """
- src/test/pkg/MyClass.java:5: error: Field test.pkg.MyClass.myField1 changed visibility from protected to public [ChangedScope]
src/test/pkg/MyClass.java:6: error: Field test.pkg.MyClass.myField2 changed visibility from public to protected [ChangedScope]
""",
checkCompatibilityApiReleased = """
@@ -3600,6 +3598,59 @@
}
@Test
+ fun `Allow increased field access for classes`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ class Foo {
+ field public int bar;
+ field protected int baz;
+ field protected int spam;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ class Foo {
+ field protected int bar;
+ field private int baz;
+ field internal int spam;
+ }
+ }
+ """
+ )
+ }
+
+ @Test
+ fun `Block decreased field access in classes`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar changed visibility from public to protected [ChangedScope]
+ TESTROOT/load-api.txt:4: error: Field test.pkg.Foo.baz changed visibility from protected to private [ChangedScope]
+ TESTROOT/load-api.txt:5: error: Field test.pkg.Foo.spam changed visibility from protected to internal [ChangedScope]
+ """,
+ signatureSource = """
+ package test.pkg {
+ class Foo {
+ field protected int bar;
+ field private int baz;
+ field internal int spam;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ class Foo {
+ field public int bar;
+ field protected int baz;
+ field protected int spam;
+ }
+ }
+ """
+ )
+ }
+
+ @Test
fun `Allow increased access`() {
check(
signatureSource = """
diff --git a/src/test/java/com/android/tools/metalava/binarycompatibility/BinaryCompatibilityClassFieldsTest.kt b/src/test/java/com/android/tools/metalava/binarycompatibility/BinaryCompatibilityClassFieldsTest.kt
new file mode 100644
index 0000000..ff1a785
--- /dev/null
+++ b/src/test/java/com/android/tools/metalava/binarycompatibility/BinaryCompatibilityClassFieldsTest.kt
@@ -0,0 +1,256 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * 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.android.tools.metalava.binarycompatibility
+
+import com.android.tools.metalava.DriverTest
+import org.junit.Test
+
+class BinaryCompatibilityClassFieldsTest : DriverTest() {
+
+ @Test
+ fun `Change type of API field (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar has changed type from java.lang.String to java.lang.Int [ChangedType]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public Int bar;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public String bar;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change value of API field, compile-time constant (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar has changed value from 8 to 7 [ChangedValue]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public static final int bar = 7;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public static final int bar = 8;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Decrease access from protected to default or private, or public to protected, default, or private (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar changed visibility from protected to private [ChangedScope]
+ TESTROOT/load-api.txt:4: error: Field test.pkg.Foo.baz changed visibility from public to protected [ChangedScope]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field private static final int bar = 8;
+ field protected static final int baz = 8;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field protected static final int bar = 8;
+ field public static final int baz = 8;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Increase access from protected to public (Compatible)`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field protected static final int bar = 8;
+ field public static final int baz = 8;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field private static final int bar = 8;
+ field protected static final int baz = 8;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change final to non-final, non-static (Compatible)`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public int bar;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public final int bar;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change final to non-final, static with compile-time constant value (Incompatible)`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public static int bar = 0;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public static final int bar = 0;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change non-final to final (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar has added 'final' qualifier [AddedFinal]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public final int bar;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public int bar;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change static to non-static (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar has changed 'static' qualifier [ChangedStatic]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public int bar = 0;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public static int bar = 0;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change non-static to static (Incompatible)`() {
+ check(
+ expectedIssues = """
+ TESTROOT/load-api.txt:3: error: Field test.pkg.Foo.bar has changed 'static' qualifier [ChangedStatic]
+ """,
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public static int bar = 0;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public int bar = 0;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change transient to non-transient (Compatible)`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public int bar = 0;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public transient int bar = 0;
+ }
+ }
+ """
+ )
+ }
+ @Test
+ fun `Change non-transient to transient (Compatible)`() {
+ check(
+ signatureSource = """
+ package test.pkg {
+ public class Foo {
+ field public transient int bar = 0;
+ }
+ }
+ """,
+ checkCompatibilityApiReleased = """
+ package test.pkg {
+ public class Foo {
+ field public int bar = 0;
+ }
+ }
+ """
+ )
+ }
+}