Avoid adding static / private interface methods to the root set.
Bug: 121240523
Change-Id: I336165d90ed0f29399ac686203c560f79c7e639a
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index 64bdee0..8ebc02f 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -496,7 +496,6 @@
// At this point all code has been mapped according to the graph lens. We cannot remove the
// graph lens entirely, though, since it is needed for mapping all field and method signatures
// back to the original program.
- GraphLense finalLense = appView.graphLense();
timing.begin("AppliedGraphLens construction");
appView.setGraphLense(new AppliedGraphLens(appView, application.classes()));
timing.end();
@@ -647,8 +646,7 @@
// Validity checks.
assert application.classes().stream().allMatch(DexClass::isValid);
- // Use the final lense while checking the validity of the final app against the root set.
- assert rootSet.verifyKeptItemsAreKept(application, appView.appInfo(), finalLense, options);
+ assert rootSet.verifyKeptItemsAreKept(application, appView.appInfo());
assert appView
.graphLense()
.verifyMappingToOriginalProgram(
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 0b5e17e..7065297 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -180,7 +180,7 @@
this.stringConcatRewriter = new StringConcatRewriter(appInfo);
this.lambdaRewriter = options.enableDesugaring ? new LambdaRewriter(this) : null;
this.interfaceMethodRewriter =
- (options.enableDesugaring && enableInterfaceMethodDesugaring())
+ options.isInterfaceMethodDesugaringEnabled()
? new InterfaceMethodRewriter(this, options) : null;
this.twrCloseResourceRewriter =
(options.enableDesugaring && enableTwrCloseResourceDesugaring())
@@ -285,16 +285,6 @@
this(appView.appInfo(), options, timing, printer, appView, mainDexClasses, rootSet);
}
- private boolean enableInterfaceMethodDesugaring() {
- switch (options.interfaceMethodDesugaring) {
- case Off:
- return false;
- case Auto:
- return !options.canUseDefaultAndStaticInterfaceMethods();
- }
- throw new Unreachable();
- }
-
private boolean enableTwrCloseResourceDesugaring() {
return enableTryWithResourcesDesugaring() && !options.canUseTwrCloseResourceMethod();
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java b/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
index 86b29a8..f3ac326 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
@@ -273,6 +273,19 @@
return false;
}
+ public boolean isSpecific() {
+ switch (getRuleType()) {
+ case ALL:
+ // fall through
+ case ALL_FIELDS:
+ // fall through
+ case ALL_METHODS:
+ return false;
+ default:
+ return Iterables.size(getWildcards()) == 0;
+ }
+ }
+
Iterable<ProguardWildcard> getWildcards() {
return Iterables.concat(
ProguardTypeMatcher.getWildcardsOrEmpty(annotation),
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index 43c39a6..9e6be06 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -30,7 +30,6 @@
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.MethodSignatureEquivalence;
-import com.android.tools.r8.utils.OffOrAuto;
import com.android.tools.r8.utils.StringDiagnostic;
import com.android.tools.r8.utils.ThreadUtils;
import com.google.common.base.Equivalence.Wrapper;
@@ -896,11 +895,30 @@
ProguardMemberRule rule,
DexDefinition precondition) {
if (context instanceof ProguardKeepRule) {
- if (item.isDexEncodedMethod() && item.asDexEncodedMethod().accessFlags.isSynthetic()) {
- // Don't keep synthetic methods (see b/120971047 for additional details).
- // TODO(b/122819537): need to distinguish lambda desugared synthetic methods v.s. kotlinc
- // synthetic methods?
- return;
+ if (item.isDexEncodedMethod()) {
+ DexEncodedMethod encodedMethod = item.asDexEncodedMethod();
+ if (encodedMethod.isSyntheticMethod()) {
+ // Don't keep synthetic methods (see b/120971047 for additional details).
+ // TODO(b/122819537): need to distinguish lambda desugared synthetic methods v.s. kotlinc
+ // synthetic methods?
+ return;
+ }
+ // If desugaring is enabled, private and static interface methods will be moved to a
+ // companion class. So we don't need to add them to the root set in the beginning.
+ if (options.isInterfaceMethodDesugaringEnabled()
+ && encodedMethod.hasCode()
+ && (encodedMethod.isPrivateMethod() || encodedMethod.isStaticMember())) {
+ DexClass holder = appView.appInfo().definitionFor(encodedMethod.method.getHolder());
+ if (holder != null && holder.isInterface()) {
+ if (rule.isSpecific()) {
+ options.reporter.warning(
+ new StringDiagnostic(
+ "The rule `" + rule + "` is ignored because the targeting interface method `"
+ + encodedMethod.method.toSourceString() + "` will be desugared."));
+ }
+ return;
+ }
+ }
}
ProguardKeepRule keepRule = (ProguardKeepRule) context;
@@ -1041,8 +1059,8 @@
this.keepUnusedArguments = keepUnusedArguments;
this.neverClassInline = neverClassInline;
this.neverMerge = Collections.unmodifiableSet(neverMerge);
- this.noSideEffects = Collections.unmodifiableMap(noSideEffects);
- this.assumedValues = Collections.unmodifiableMap(assumedValues);
+ this.noSideEffects = noSideEffects;
+ this.assumedValues = assumedValues;
this.dependentNoShrinking = dependentNoShrinking;
this.identifierNameStrings = Collections.unmodifiableSet(identifierNameStrings);
this.ifRules = Collections.unmodifiableSet(ifRules);
@@ -1064,8 +1082,10 @@
lense.rewriteMutableMethodsConservatively(previous.keepUnusedArguments);
this.neverClassInline = lense.rewriteMutableTypesConservatively(previous.neverClassInline);
this.neverMerge = lense.rewriteTypesConservatively(previous.neverMerge);
- this.noSideEffects = rewriteReferenceKeys(previous.noSideEffects, lense::lookupReference);
- this.assumedValues = rewriteReferenceKeys(previous.assumedValues, lense::lookupReference);
+ this.noSideEffects =
+ rewriteMutableReferenceKeys(previous.noSideEffects, lense::lookupReference);
+ this.assumedValues =
+ rewriteMutableReferenceKeys(previous.assumedValues, lense::lookupReference);
this.dependentNoShrinking =
rewriteDependentReferenceKeys(previous.dependentNoShrinking, lense::lookupReference);
this.identifierNameStrings =
@@ -1109,6 +1129,37 @@
dependentNoShrinking.getOrDefault(item.toReference(), Collections.emptyMap()));
}
+ public void copy(DexReference original, DexReference rewritten) {
+ if (noShrinking.containsKey(original)) {
+ noShrinking.put(rewritten, noShrinking.get(original));
+ }
+ if (noOptimization.contains(original)) {
+ noOptimization.add(rewritten);
+ }
+ if (noObfuscation.contains(original)) {
+ noObfuscation.add(rewritten);
+ }
+ if (noSideEffects.containsKey(original)) {
+ noSideEffects.put(rewritten, noSideEffects.get(original));
+ }
+ if (assumedValues.containsKey(original)) {
+ assumedValues.put(rewritten, assumedValues.get(original));
+ }
+ }
+
+ public void prune(DexReference reference) {
+ noShrinking.remove(reference);
+ noOptimization.remove(reference);
+ noObfuscation.remove(reference);
+ noSideEffects.remove(reference);
+ assumedValues.remove(reference);
+ }
+
+ public void move(DexReference original, DexReference rewritten) {
+ copy(original, rewritten);
+ prune(original);
+ }
+
public boolean verifyKeptFieldsAreAccessedAndLive(AppInfoWithLiveness appInfo) {
for (DexReference reference : noShrinking.keySet()) {
if (reference.isDexField()) {
@@ -1173,40 +1224,7 @@
return false;
}
- // TODO(b/121240523): Ideally, these default interface methods should be delegated.
- private boolean isDesugaredMethod(
- AppInfo appInfo, GraphLense lense, DexClass clazz, DexMethod method) {
- if (clazz == null) {
- clazz = appInfo.definitionFor(method.getHolder());
- }
- Set<DexMethod> lookupMethods = lense.lookupMethodInAllContexts(method);
- if (lookupMethods.size() == 1) {
- for (DexMethod lookupMethod : lookupMethods) {
- if (lookupMethod == method) {
- continue;
- }
- DexEncodedMethod encodedMethod = appInfo.definitionFor(lookupMethod);
- if (clazz != null
- && clazz.isInterface()
- && encodedMethod != null
- && encodedMethod.hasCode()) {
- return true;
- }
- }
- }
- return false;
- }
-
- public boolean verifyKeptItemsAreKept(
- DexApplication application,
- AppInfo appInfo,
- GraphLense lense,
- InternalOptions options) {
- boolean isInterfaceMethodDesugaringEnabled =
- options.enableDesugaring
- && options.interfaceMethodDesugaring == OffOrAuto.Auto
- && !options.canUseDefaultAndStaticInterfaceMethods();
-
+ public boolean verifyKeptItemsAreKept(DexApplication application, AppInfo appInfo) {
Set<DexReference> pinnedItems =
appInfo.hasLiveness() ? appInfo.withLiveness().pinnedItems : null;
@@ -1214,10 +1232,7 @@
Map<DexType, Set<DexReference>> requiredReferencesPerType = new IdentityHashMap<>();
for (DexReference reference : noShrinking.keySet()) {
// Check that `pinnedItems` is a super set of the root set.
- assert pinnedItems == null || pinnedItems.contains(reference)
- || (reference.isDexMethod()
- && isInterfaceMethodDesugaringEnabled
- && isDesugaredMethod(appInfo, lense, null, reference.asDexMethod()));
+ assert pinnedItems == null || pinnedItems.contains(reference);
if (reference.isDexType()) {
DexType type = reference.asDexType();
requiredReferencesPerType.putIfAbsent(type, Sets.newIdentityHashSet());
@@ -1251,10 +1266,6 @@
assert fields.contains(requiredField);
} else if (requiredReference.isDexMethod()) {
DexMethod requiredMethod = requiredReference.asDexMethod();
- if (isInterfaceMethodDesugaringEnabled
- && isDesugaredMethod(appInfo, lense, clazz, requiredMethod)) {
- continue;
- }
if (methods == null) {
// Create a Set of the methods to avoid quadratic behavior.
methods =
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 7f4fbe1..a35100e 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -586,6 +586,16 @@
return isGeneratingClassFiles() || hasMinApi(AndroidApiLevel.N);
}
+ public boolean isInterfaceMethodDesugaringEnabled() {
+ // This condition is to filter out tests that never set program consumer.
+ if (!hasConsumer()) {
+ return false;
+ }
+ return enableDesugaring
+ && interfaceMethodDesugaring == OffOrAuto.Auto
+ && !canUseDefaultAndStaticInterfaceMethods();
+ }
+
public boolean canUseMultidex() {
assert isGeneratingDex();
return intermediate || hasMinApi(AndroidApiLevel.L);
diff --git a/src/test/java/com/android/tools/r8/R8TestCompileResult.java b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
index a9c02ff..ecb0a92 100644
--- a/src/test/java/com/android/tools/r8/R8TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
@@ -55,6 +55,11 @@
return state.getDiagnosticsMessages();
}
+ public R8TestCompileResult inspectDiagnosticMessages(Consumer<TestDiagnosticMessages> consumer) {
+ consumer.accept(state.getDiagnosticsMessages());
+ return self();
+ }
+
@Override
public CodeInspector inspector() throws IOException, ExecutionException {
return new CodeInspector(app, proguardMap);
diff --git a/src/test/java/com/android/tools/r8/shaking/desugar/KeepRuleWarningTest.java b/src/test/java/com/android/tools/r8/shaking/desugar/KeepRuleWarningTest.java
new file mode 100644
index 0000000..ab37cb0
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/desugar/KeepRuleWarningTest.java
@@ -0,0 +1,102 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.shaking.desugar;
+
+import static org.hamcrest.CoreMatchers.containsString;
+
+import com.android.tools.r8.NeverMerge;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestDiagnosticMessages;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.StringUtils;
+import org.junit.Test;
+
+@NeverMerge
+interface I {
+ static void foo() {
+ System.out.println("static::foo");
+ }
+
+ default void bar() {
+ foo();
+ System.out.println("default::bar");
+ }
+}
+
+class C implements I {
+}
+
+class KeepRuleWarningTestRunner {
+ public static void main(String[] args) {
+ C obj = new C();
+ obj.bar();
+ }
+}
+
+public class KeepRuleWarningTest extends TestBase {
+ private static final Class<?> MAIN = KeepRuleWarningTestRunner.class;
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("static::foo", "default::bar");
+
+ @Test
+ public void test_allMethods() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(I.class, C.class, MAIN)
+ .setMinApi(AndroidApiLevel.L)
+ .enableMergeAnnotations()
+ .addKeepMainRule(MAIN)
+ .addKeepRules("-keep interface **.I { <methods>; }")
+ .compile()
+ .inspectDiagnosticMessages(TestDiagnosticMessages::assertNoMessages)
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
+ }
+
+ @Test
+ public void test_asterisk() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(I.class, C.class, MAIN)
+ .setMinApi(AndroidApiLevel.L)
+ .enableMergeAnnotations()
+ .addKeepMainRule(MAIN)
+ .addKeepRules("-keep interface **.I { *(); }")
+ .compile()
+ .inspectDiagnosticMessages(TestDiagnosticMessages::assertNoMessages)
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
+ }
+
+ @Test
+ public void test_stillNotSpecific() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(I.class, C.class, MAIN)
+ .setMinApi(AndroidApiLevel.L)
+ .enableMergeAnnotations()
+ .addKeepMainRule(MAIN)
+ .addKeepRules("-keep interface **.I { *** f*(); }")
+ .compile()
+ .inspectDiagnosticMessages(TestDiagnosticMessages::assertNoMessages)
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
+ }
+
+ @Test
+ public void test_specific() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(I.class, C.class, MAIN)
+ .setMinApi(AndroidApiLevel.L)
+ .enableMergeAnnotations()
+ .addKeepMainRule(MAIN)
+ .addKeepRules("-keep interface **.I { static void foo(); }")
+ .compile()
+ .inspectDiagnosticMessages(m -> {
+ m.assertWarningsCount(1)
+ .assertWarningMessageThatMatches(containsString("static void foo()"))
+ .assertWarningMessageThatMatches(containsString("is ignored"))
+ .assertWarningMessageThatMatches(containsString("will be desugared"));
+ })
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
+ }
+
+}
\ No newline at end of file
diff --git a/src/test/java/com/android/tools/r8/shaking/desugar/interfacemethods/BridgeInliningTest.java b/src/test/java/com/android/tools/r8/shaking/desugar/interfacemethods/BridgeInliningTest.java
new file mode 100644
index 0000000..5038729
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/desugar/interfacemethods/BridgeInliningTest.java
@@ -0,0 +1,73 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.shaking.desugar.interfacemethods;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverMerge;
+import com.android.tools.r8.TestBase;
+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.MethodSubject;
+import java.lang.reflect.Method;
+import org.junit.Test;
+
+@NeverMerge
+interface I {
+ default void m() {
+ System.out.println("I::m");
+ }
+}
+
+class C implements I {
+}
+
+class BridgeInliningTestRunner {
+ public static void main(String[] args) throws Exception {
+ C obj = new C();
+ for (Method m : obj.getClass().getDeclaredMethods()) {
+ m.invoke(obj, null);
+ }
+ }
+}
+
+public class BridgeInliningTest extends TestBase {
+ private static final Class<?> MAIN = BridgeInliningTestRunner.class;
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("I::m");
+
+ @Test
+ public void test() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(I.class, C.class, MAIN)
+ .setMinApi(AndroidApiLevel.L)
+ .enableMergeAnnotations()
+ .addKeepMainRule(MAIN)
+ .addKeepRules("-keep interface **.I { m(); }")
+ .run(MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(this::inspect);
+ }
+
+ private void inspect(CodeInspector codeInspector) {
+ ClassSubject c = codeInspector.clazz(C.class);
+ assertThat(c, isPresent());
+ MethodSubject m = c.uniqueMethodWithName("m");
+ assertThat(m, isPresent());
+ assertTrue(m.getMethod().hasCode());
+ // TODO(b/123778921): why are I and C not merged without merge annotations?
+ //assertTrue(
+ // m.iterateInstructions(i -> i.isConstString("I::m", JumboStringMode.ALLOW)).hasNext());
+
+ // TODO(b/123778921): No companion class is in the output.
+ //codeInspector.forAllClasses(classSubject -> {
+ // assertFalse(classSubject.getOriginalDescriptor().contains("$-CC"));
+ //});
+ }
+
+}
