Nest Access: Private access inside a class
- private accesses inside a class were rewritten
to a call to the bridge, if the bridge was needed for other
reasons. This was correct but was not necessary.
- added tests for it (Turns out d8 was sometimes generating
extra bridges)
Bug:132147078
Change-Id: I3ac6b7cebd81eaa1424d591d3190b95280ba31e4
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/NestBasedAccessDesugaringRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/NestBasedAccessDesugaringRewriter.java
index d4e25df..4eb29a1 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/NestBasedAccessDesugaringRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/NestBasedAccessDesugaringRewriter.java
@@ -94,7 +94,7 @@
DexMethod method,
Map<DexField, DexMethod> fieldToMethodMap) {
DexMethod newTarget = fieldToMethodMap.get(fieldInstruction.getField());
- if (newTarget != null && method != newTarget) {
+ if (newTarget != null && method.holder != newTarget.holder) {
instructions.replaceCurrentInstruction(
new InvokeStatic(newTarget, fieldInstruction.outValue(), fieldInstruction.inValues()));
}
@@ -126,7 +126,7 @@
new InvokeStatic(newTarget, invokeMethod.outValue(), invokeMethod.arguments()));
} else {
newTarget = initializerMap.get(methodCalled);
- if (newTarget != null && encodedMethod.method != newTarget) {
+ if (newTarget != null && encodedMethod.method.holder != newTarget.holder) {
// insert extra null value and replace call.
instructions.previous();
Value extraNullValue =
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/NestedPrivateMethodLense.java b/src/main/java/com/android/tools/r8/ir/desugar/NestedPrivateMethodLense.java
index a324d61b..0411ea4 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/NestedPrivateMethodLense.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/NestedPrivateMethodLense.java
@@ -164,10 +164,9 @@
// All generated initializer bridges are direct.
newType = Invoke.Type.DIRECT;
}
- if (newMethod == context) {
- // Bridges should not rewrite themselves.
- // TODO(b/130529338): Best would be that if we are in Class A and targeting class
- // A private method, we do not rewrite it.
+ if (context != null && newMethod.holder == context.holder) {
+ // Without context we conservatively rewrite the method.
+ // Private calls inside a class do not need to be rewritten.
return previous;
}
return new GraphLenseLookupResult(newMethod, newType);
diff --git a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassFields.java b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassFields.java
index a16f6a6..f23ef4b 100644
--- a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassFields.java
+++ b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassFields.java
@@ -6,6 +6,7 @@
public class BasicNestHostWithInnerClassFields {
+ private String fieldWithoutBridge = "noBridge";
private String field = "field";
private static String staticField = "staticField";
@@ -13,7 +14,7 @@
public String accessNested(BasicNestedClass o) {
o.field = "RW" + o.field;
o.staticField = "RW" + o.field;
- return o.field + o.staticField + BasicNestedClass.staticField;
+ return o.field + o.staticField + BasicNestedClass.staticField + fieldWithoutBridge;
}
public static class BasicNestedClass {
diff --git a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassMethods.java b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassMethods.java
index 1313ef5..1b6e1f1 100644
--- a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassMethods.java
+++ b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassMethods.java
@@ -6,6 +6,10 @@
public class BasicNestHostWithInnerClassMethods {
+ private String methodWithoutBridge() {
+ return "noBridge";
+ }
+
private String method() {
return "hostMethod";
}
@@ -16,7 +20,7 @@
@SuppressWarnings("static-access") // we want to test that too.
public String accessNested(BasicNestedClass o) {
- return o.method() + o.staticMethod() + BasicNestedClass.staticMethod();
+ return o.method() + o.staticMethod() + BasicNestedClass.staticMethod() + methodWithoutBridge();
}
public static class BasicNestedClass {
diff --git a/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAccessControlTest.java b/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAccessControlTest.java
index f3e1851..16312c7 100644
--- a/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAccessControlTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAccessControlTest.java
@@ -27,6 +27,7 @@
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FoundClassSubject;
import com.android.tools.r8.utils.codeinspector.FoundMethodSubject;
@@ -206,6 +207,32 @@
testR8("all");
}
+ private void assertOnlyRequiredBridges(CodeInspector inspector) {
+ // The following 2 classes have an extra private member which does not require a bridge.
+
+ // Two bridges for method and staticMethod.
+ int methodNumBridges = parameters.isCfRuntime() ? 0 : 2;
+ ClassSubject methodMainClass = inspector.clazz(getMainClass("methods"));
+ assertEquals(
+ methodNumBridges, methodMainClass.allMethods(FoundMethodSubject::isSynthetic).size());
+
+ // Four bridges for field and staticField, both get & set.
+ int fieldNumBridges = parameters.isCfRuntime() ? 0 : 4;
+ ClassSubject fieldMainClass = inspector.clazz(getMainClass("fields"));
+ assertEquals(
+ fieldNumBridges, fieldMainClass.allMethods(FoundMethodSubject::isSynthetic).size());
+ }
+
+ @Test
+ public void testOnlyRequiredBridges() throws Exception {
+ if (parameters.isDexRuntime()) {
+ d8CompilationResult.apply(parameters.getApiLevel()).inspect(this::assertOnlyRequiredBridges);
+ }
+ r8CompilationResult
+ .apply(parameters.getBackend(), parameters.getApiLevel())
+ .inspect(this::assertOnlyRequiredBridges);
+ }
+
private static void checkNestMateAttributes(CodeInspector inspector) {
// Interface method desugaring may add extra classes
assertTrue(NUMBER_OF_TEST_CLASSES <= inspector.allClasses().size());
