Nest private member access clean-up
- in D8, private constructor calls inside
a class should not require bridge
(Add test for this case)
- in r8, no bridges are needed for private
field access within a class
(Also move nest access desugar logic from
LensCodeRewriter to corresponding Lens)
- in R8, nest based desugaring should work
when other lens are present.
- in R8, the lens can be ignored only if none
of the map are used.
(Add test for this case)
Bug:132147078
Change-Id: I2aaa95a906709e4e930eddfdacec0994d7e6d6d0
diff --git a/src/main/java/com/android/tools/r8/graph/GraphLense.java b/src/main/java/com/android/tools/r8/graph/GraphLense.java
index 5b6ef08..fc24fd2 100644
--- a/src/main/java/com/android/tools/r8/graph/GraphLense.java
+++ b/src/main/java/com/android/tools/r8/graph/GraphLense.java
@@ -458,19 +458,19 @@
public abstract DexField lookupField(DexField field);
- public DexMethod lookupStaticGetFieldForMethod(DexField field) {
+ public DexMethod lookupStaticGetFieldForMethod(DexField field, DexMethod context) {
return null;
}
- public DexMethod lookupStaticPutFieldForMethod(DexField field) {
+ public DexMethod lookupStaticPutFieldForMethod(DexField field, DexMethod context) {
return null;
}
- public DexMethod lookupInstanceGetFieldForMethod(DexField field) {
+ public DexMethod lookupInstanceGetFieldForMethod(DexField field, DexMethod context) {
return null;
}
- public DexMethod lookupInstancePutFieldForMethod(DexField field) {
+ public DexMethod lookupInstancePutFieldForMethod(DexField field, DexMethod context) {
return null;
}
@@ -970,6 +970,26 @@
return previousLense.lookupPrototypeChanges(method);
}
+ @Override
+ public DexMethod lookupStaticGetFieldForMethod(DexField field, DexMethod context) {
+ return previousLense.lookupStaticGetFieldForMethod(field, context);
+ }
+
+ @Override
+ public DexMethod lookupStaticPutFieldForMethod(DexField field, DexMethod context) {
+ return previousLense.lookupStaticPutFieldForMethod(field, context);
+ }
+
+ @Override
+ public DexMethod lookupInstanceGetFieldForMethod(DexField field, DexMethod context) {
+ return previousLense.lookupInstanceGetFieldForMethod(field, context);
+ }
+
+ @Override
+ public DexMethod lookupInstancePutFieldForMethod(DexField field, DexMethod context) {
+ return previousLense.lookupInstancePutFieldForMethod(field, context);
+ }
+
/**
* Default invocation type mapping.
*
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java b/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
index 2203096..cb56a09 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
@@ -278,8 +278,8 @@
DexField field = instanceGet.getField();
DexField actualField = graphLense.lookupField(field);
DexMethod replacementMethod =
- graphLense.lookupInstanceGetFieldForMethod(actualField);
- if (replacementMethod != null && method.method != replacementMethod) {
+ graphLense.lookupInstanceGetFieldForMethod(actualField, method.method);
+ if (replacementMethod != null) {
iterator.replaceCurrentInstruction(
new InvokeStatic(replacementMethod, current.outValue(), current.inValues()));
} else if (actualField != field) {
@@ -295,8 +295,8 @@
DexField field = instancePut.getField();
DexField actualField = graphLense.lookupField(field);
DexMethod replacementMethod =
- graphLense.lookupInstancePutFieldForMethod(actualField);
- if (replacementMethod != null && method.method != replacementMethod) {
+ graphLense.lookupInstancePutFieldForMethod(actualField, method.method);
+ if (replacementMethod != null) {
iterator.replaceCurrentInstruction(
new InvokeStatic(replacementMethod, current.outValue(), current.inValues()));
} else if (actualField != field) {
@@ -309,8 +309,8 @@
DexField field = staticGet.getField();
DexField actualField = graphLense.lookupField(field);
DexMethod replacementMethod =
- graphLense.lookupStaticGetFieldForMethod(actualField);
- if (replacementMethod != null && method.method != replacementMethod) {
+ graphLense.lookupStaticGetFieldForMethod(actualField, method.method);
+ if (replacementMethod != null) {
iterator.replaceCurrentInstruction(
new InvokeStatic(replacementMethod, current.outValue(), current.inValues()));
} else if (actualField != field) {
@@ -323,8 +323,8 @@
DexField field = staticPut.getField();
DexField actualField = graphLense.lookupField(field);
DexMethod replacementMethod =
- graphLense.lookupStaticPutFieldForMethod(actualField);
- if (replacementMethod != null && method.method != replacementMethod) {
+ graphLense.lookupStaticPutFieldForMethod(actualField, method.method);
+ if (replacementMethod != null) {
iterator.replaceCurrentInstruction(
new InvokeStatic(replacementMethod, current.outValue(), current.inValues()));
} else if (actualField != field) {
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 16ae320..288b6a0 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
@@ -123,7 +123,7 @@
DexMethod methodCalled = invokeMethod.getInvokedMethod();
registerInvoke(methodCalled, nest, currentClass);
DexMethod newTarget = methodMap.get(methodCalled);
- if (newTarget != null && encodedMethod.method != newTarget) {
+ if (newTarget != null && encodedMethod.method.holder != newTarget.holder) {
instructions.replaceCurrentInstruction(
new InvokeStatic(newTarget, invokeMethod.outValue(), invokeMethod.arguments()));
} else {
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 0411ea4..6878f86 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
@@ -48,7 +48,12 @@
}
public GraphLense build(GraphLense previousLense, DexType nestConstructorType) {
- if (methodMap.isEmpty() && fieldMap.isEmpty()) {
+ if (methodMap.isEmpty()
+ && staticGetToMethodMap.isEmpty()
+ && staticPutToMethodMap.isEmpty()
+ && instanceGetToMethodMap.isEmpty()
+ && instancePutToMethodMap.isEmpty()
+ && initializerMap.isEmpty()) {
return previousLense;
}
return new NestedPrivateMethodLense(
@@ -103,23 +108,33 @@
}
@Override
- public DexMethod lookupStaticGetFieldForMethod(DexField field) {
- return staticGetToMethodMap.get(field);
+ public DexMethod lookupStaticGetFieldForMethod(DexField field, DexMethod context) {
+ return lookupFieldAccessForMethod(field, context, staticGetToMethodMap);
}
@Override
- public DexMethod lookupStaticPutFieldForMethod(DexField field) {
- return staticPutToMethodMap.get(field);
+ public DexMethod lookupStaticPutFieldForMethod(DexField field, DexMethod context) {
+ return lookupFieldAccessForMethod(field, context, staticPutToMethodMap);
}
@Override
- public DexMethod lookupInstanceGetFieldForMethod(DexField field) {
- return instanceGetToMethodMap.get(field);
+ public DexMethod lookupInstanceGetFieldForMethod(DexField field, DexMethod context) {
+ return lookupFieldAccessForMethod(field, context, instanceGetToMethodMap);
}
@Override
- public DexMethod lookupInstancePutFieldForMethod(DexField field) {
- return instancePutToMethodMap.get(field);
+ public DexMethod lookupInstancePutFieldForMethod(DexField field, DexMethod context) {
+ return lookupFieldAccessForMethod(field, context, instancePutToMethodMap);
+ }
+
+ private DexMethod lookupFieldAccessForMethod(
+ DexField field, DexMethod context, Map<DexField, DexMethod> map) {
+ DexMethod replacementMethod = map.get(field);
+ // We replace the field access by the bridge only if on a different class than context.
+ if (replacementMethod != null && context.holder != replacementMethod.holder) {
+ return replacementMethod;
+ }
+ return null;
}
@Override
@@ -127,6 +142,12 @@
return false;
}
+ // This is true because mappings specific to this class can be filled.
+ @Override
+ protected boolean isLegitimateToHaveEmptyMappings() {
+ return true;
+ }
+
@Override
public RewrittenPrototypeDescription lookupPrototypeChanges(DexMethod method) {
DexType[] parameters = method.proto.parameters.values;
diff --git a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassConstructors.java b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassConstructors.java
index 8b8f6ce..d61d351 100644
--- a/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassConstructors.java
+++ b/src/test/examplesJava11/nestHostExample/BasicNestHostWithInnerClassConstructors.java
@@ -12,6 +12,10 @@
this.field = field;
}
+ private BasicNestHostWithInnerClassConstructors(int intVal) {
+ this.field = String.valueOf(intVal);
+ }
+
public static BasicNestedClass createNestedInstance(String field) {
return new BasicNestedClass(field);
}
@@ -33,8 +37,11 @@
BasicNestHostWithInnerClassConstructors outer = BasicNestedClass.createOuterInstance("field");
BasicNestedClass inner =
BasicNestHostWithInnerClassConstructors.createNestedInstance("nest1SField");
+ BasicNestHostWithInnerClassConstructors noBridge =
+ new BasicNestHostWithInnerClassConstructors(1);
System.out.println(outer.field);
System.out.println(inner.field);
+ System.out.println(noBridge.field);
}
}
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 0d6bd56..c62c4bd 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
@@ -36,6 +36,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import org.hamcrest.Matcher;
@@ -99,7 +100,7 @@
StringUtils.lines(
"nestMethodstaticNestMethodstaticNestMethodnoBridge",
"hostMethodstaticHostMethodstaticNestMethod"),
- "constructors", StringUtils.lines("field", "nest1SField"),
+ "constructors", StringUtils.lines("field", "nest1SField", "1"),
"anonymous",
StringUtils.lines(
"fieldstaticFieldstaticFieldhostMethodstaticHostMethodstaticHostMethod"),
@@ -216,6 +217,13 @@
assertEquals(
methodNumBridges, methodMainClass.allMethods(FoundMethodSubject::isSynthetic).size());
+ // Two bridges for method and staticMethod.
+ int constructorNumBridges = parameters.isCfRuntime() ? 0 : 1;
+ ClassSubject constructorMainClass = inspector.clazz(getMainClass("constructors"));
+ assertEquals(
+ constructorNumBridges,
+ constructorMainClass.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"));
@@ -314,6 +322,27 @@
testIncompleteNestError(false);
}
+ @Test
+ public void testR8SingleNest() throws Exception {
+ for (Map.Entry<String, String> entry : MAIN_CLASSES.entrySet()) {
+ List<Path> matchingClasses =
+ CLASS_NAMES.stream()
+ .filter(name -> containsString(entry.getValue()).matches(name))
+ .map(name -> CLASSES_PATH.resolve(name + CLASS_EXTENSION))
+ .collect(toList());
+ testForR8(parameters.getBackend())
+ .noTreeShaking()
+ .noMinification()
+ .addKeepAllAttributes()
+ .setMinApi(parameters.getApiLevel())
+ .addProgramFiles(matchingClasses)
+ .addOptionsModification(options -> options.enableNestBasedAccessDesugaring = true)
+ .compile()
+ .run(parameters.getRuntime(), getMainClass(entry.getKey()))
+ .assertSuccessWithOutput(getExpectedResult(entry.getKey()));
+ }
+ }
+
private D8TestCompileResult compileClassesWithD8ProgramClassesMatching(Matcher<String> matcher)
throws Exception {
List<Path> matchingClasses =
