Introduce keepinfo for signature removal
This CL will also allow R8 in full mode to strip generic signatures for classes that are not kept when having -dontoptimize
Bug: 221404266
Bug: 190469643
Change-Id: I9c008b5e46b424ace805d738b646bd3980ace639
diff --git a/src/main/java/com/android/tools/r8/graph/GenericSignatureCorrectnessHelper.java b/src/main/java/com/android/tools/r8/graph/GenericSignatureCorrectnessHelper.java
index 950e824..7864ee8 100644
--- a/src/main/java/com/android/tools/r8/graph/GenericSignatureCorrectnessHelper.java
+++ b/src/main/java/com/android/tools/r8/graph/GenericSignatureCorrectnessHelper.java
@@ -139,7 +139,7 @@
KeepClassInfo classInfo = appView.getKeepInfo().getClassInfo(clazz);
if (appView.hasLiveness() && !classInfo.isShrinkingAllowed(appView.options())) {
// If/when this no longer holds it should be moved into the condition.
- assert !classInfo.isSignatureAttributeRemovalAllowed(appView.options());
+ assert !classInfo.isSignatureRemovalAllowed(appView.options());
appView
.options()
.reporter
@@ -167,7 +167,7 @@
if (appView.hasLiveness()
&& !methodInfo.isShrinkingAllowed(appView.options())) {
// If/when this no longer holds it should be moved into the condition.
- assert !methodInfo.isSignatureAttributeRemovalAllowed(appView.options());
+ assert !methodInfo.isSignatureRemovalAllowed(appView.options());
appView
.options()
.reporter
@@ -195,7 +195,7 @@
// ensure we do not spam the developer with messages they can do nothing about.
if (appView.hasLiveness() && !fieldInfo.isShrinkingAllowed(appView.options())) {
// If/when this no longer holds it should be moved into the condition.
- assert !fieldInfo.isSignatureAttributeRemovalAllowed(appView.options());
+ assert !fieldInfo.isSignatureRemovalAllowed(appView.options());
appView
.options()
.reporter
diff --git a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
index 1b2e03c..4b9385d 100644
--- a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
@@ -217,7 +217,7 @@
Set<KotlinPropertyInfo> pinnedKotlinProperties) {
KeepMemberInfo<?, ?> memberInfo = appView.getKeepInfo().getMemberInfo(member);
removeAnnotations(member, memberInfo);
- if (memberInfo.isSignatureAttributeRemovalAllowed(options)) {
+ if (memberInfo.isSignatureRemovalAllowed(options)) {
member.clearGenericSignature();
}
if (!member.getKotlinInfo().isProperty()
@@ -346,8 +346,7 @@
clazz.removeInnerClasses(
attribute ->
canRemoveInnerClassAttribute(clazz, attribute, clazz.getEnclosingMethodAttribute()));
- if (clazz.getClassSignature().isValid()
- && keepInfo.isSignatureAttributeRemovalAllowed(options)) {
+ if (clazz.getClassSignature().isValid() && keepInfo.isSignatureRemovalAllowed(options)) {
clazz.clearClassSignature();
}
}
diff --git a/src/main/java/com/android/tools/r8/shaking/KeepInfo.java b/src/main/java/com/android/tools/r8/shaking/KeepInfo.java
index bcf9467..713633b 100644
--- a/src/main/java/com/android/tools/r8/shaking/KeepInfo.java
+++ b/src/main/java/com/android/tools/r8/shaking/KeepInfo.java
@@ -21,6 +21,7 @@
private final boolean allowMinification;
private final boolean allowOptimization;
private final boolean allowShrinking;
+ private final boolean allowSignatureRemoval;
private final boolean checkDiscarded;
private final boolean requireAccessModificationForRepackaging;
@@ -30,6 +31,7 @@
boolean allowMinification,
boolean allowOptimization,
boolean allowShrinking,
+ boolean allowSignatureRemoval,
boolean checkDiscarded,
boolean requireAccessModificationForRepackaging) {
this.allowAccessModification = allowAccessModification;
@@ -37,6 +39,7 @@
this.allowMinification = allowMinification;
this.allowOptimization = allowOptimization;
this.allowShrinking = allowShrinking;
+ this.allowSignatureRemoval = allowSignatureRemoval;
this.checkDiscarded = checkDiscarded;
this.requireAccessModificationForRepackaging = requireAccessModificationForRepackaging;
}
@@ -48,6 +51,7 @@
builder.isMinificationAllowed(),
builder.isOptimizationAllowed(),
builder.isShrinkingAllowed(),
+ builder.isSignatureRemovalAllowed(),
builder.isCheckDiscardedEnabled(),
builder.isAccessModificationRequiredForRepackaging());
}
@@ -136,6 +140,24 @@
}
/**
+ * True if an item may have its generic signature removed.
+ *
+ * <p>This method requires knowledge of the global configuration as that can override the concrete
+ * value on a given item.
+ */
+ public boolean isSignatureRemovalAllowed(GlobalKeepInfoConfiguration configuration) {
+ if (!configuration.isKeepAttributesSignatureEnabled()) {
+ return true;
+ }
+ return !configuration.isForceProguardCompatibilityEnabled()
+ && internalIsSignatureRemovalAllowed();
+ }
+
+ boolean internalIsSignatureRemovalAllowed() {
+ return allowSignatureRemoval;
+ }
+
+ /**
* True if an item may be repackaged.
*
* <p>This method requires knowledge of the global configuration as that can override the concrete
@@ -165,13 +187,6 @@
return allowAccessModification;
}
- public boolean isSignatureAttributeRemovalAllowed(GlobalKeepInfoConfiguration configuration) {
- if (!configuration.isKeepAttributesSignatureEnabled()) {
- return true;
- }
- return !(configuration.isForceProguardCompatibilityEnabled() || isPinned(configuration));
- }
-
public boolean isEnclosingMethodAttributeRemovalAllowed(
GlobalKeepInfoConfiguration configuration,
EnclosingMethodAttribute enclosingMethodAttribute,
@@ -218,6 +233,7 @@
&& (allowMinification || !other.internalIsMinificationAllowed())
&& (allowOptimization || !other.internalIsOptimizationAllowed())
&& (allowShrinking || !other.internalIsShrinkingAllowed())
+ && (allowSignatureRemoval || !other.internalIsSignatureRemovalAllowed())
&& (!checkDiscarded || other.internalIsCheckDiscardedEnabled());
}
@@ -240,6 +256,7 @@
private boolean allowMinification;
private boolean allowOptimization;
private boolean allowShrinking;
+ private boolean allowSignatureRemoval;
private boolean checkDiscarded;
private boolean requireAccessModificationForRepackaging;
@@ -254,6 +271,7 @@
allowMinification = original.internalIsMinificationAllowed();
allowOptimization = original.internalIsOptimizationAllowed();
allowShrinking = original.internalIsShrinkingAllowed();
+ allowSignatureRemoval = original.internalIsSignatureRemovalAllowed();
checkDiscarded = original.internalIsCheckDiscardedEnabled();
requireAccessModificationForRepackaging =
original.internalIsAccessModificationRequiredForRepackaging();
@@ -265,6 +283,7 @@
disallowMinification();
disallowOptimization();
disallowShrinking();
+ disallowSignatureRemoval();
unsetCheckDiscarded();
requireAccessModificationForRepackaging();
return self();
@@ -276,6 +295,7 @@
allowMinification();
allowOptimization();
allowShrinking();
+ allowSignatureRemoval();
unsetCheckDiscarded();
unsetRequireAccessModificationForRepackaging();
return self();
@@ -302,6 +322,7 @@
&& isMinificationAllowed() == other.internalIsMinificationAllowed()
&& isOptimizationAllowed() == other.internalIsOptimizationAllowed()
&& isShrinkingAllowed() == other.internalIsShrinkingAllowed()
+ && isSignatureRemovalAllowed() == other.internalIsSignatureRemovalAllowed()
&& isCheckDiscardedEnabled() == other.internalIsCheckDiscardedEnabled()
&& isAccessModificationRequiredForRepackaging()
== other.internalIsAccessModificationRequiredForRepackaging();
@@ -335,6 +356,10 @@
return allowShrinking;
}
+ public boolean isSignatureRemovalAllowed() {
+ return allowSignatureRemoval;
+ }
+
public B setAllowMinification(boolean allowMinification) {
this.allowMinification = allowMinification;
return self();
@@ -426,6 +451,19 @@
public B disallowAnnotationRemoval() {
return setAllowAnnotationRemoval(false);
}
+
+ private B setAllowSignatureRemoval(boolean allowSignatureRemoval) {
+ this.allowSignatureRemoval = allowSignatureRemoval;
+ return self();
+ }
+
+ public B allowSignatureRemoval() {
+ return setAllowSignatureRemoval(true);
+ }
+
+ public B disallowSignatureRemoval() {
+ return setAllowSignatureRemoval(false);
+ }
}
/** Joiner to construct monotonically increasing keep info object. */
@@ -519,6 +557,11 @@
return self();
}
+ public J disallowSignatureRemoval() {
+ builder.disallowSignatureRemoval();
+ return self();
+ }
+
public J setCheckDiscarded() {
builder.setCheckDiscarded();
return self();
@@ -536,6 +579,7 @@
applyIf(!builder.isMinificationAllowed(), Joiner::disallowMinification);
applyIf(!builder.isOptimizationAllowed(), Joiner::disallowOptimization);
applyIf(!builder.isShrinkingAllowed(), Joiner::disallowShrinking);
+ applyIf(!builder.isSignatureRemovalAllowed(), Joiner::disallowSignatureRemoval);
applyIf(builder.isCheckDiscardedEnabled(), Joiner::setCheckDiscarded);
applyIf(
builder.isAccessModificationRequiredForRepackaging(),
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
index 524fe08..b10e649 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
@@ -1532,6 +1532,13 @@
context.markAsUsed();
}
+ if (appView.options().isKeepAttributesSignatureEnabled()) {
+ dependentMinimumKeepInfo
+ .getOrCreateMinimumKeepInfoFor(preconditionEvent, item.getReference())
+ .disallowSignatureRemoval();
+ context.markAsUsed();
+ }
+
if (appView.options().isMinificationEnabled() && !modifiers.allowsObfuscation) {
dependentMinimumKeepInfo
.getOrCreateMinimumKeepInfoFor(preconditionEvent, item.getReference())
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 7db2e06..12e0a78 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -636,12 +636,12 @@
}
public boolean parseSignatureAttribute() {
- return proguardConfiguration == null || isKeepAttributesSignatureEnabled();
+ return isKeepAttributesSignatureEnabled();
}
@Override
public boolean isKeepAttributesSignatureEnabled() {
- return proguardConfiguration.getKeepAttributes().signature;
+ return proguardConfiguration == null || proguardConfiguration.getKeepAttributes().signature;
}
@Override
diff --git a/src/test/java/com/android/tools/r8/graph/genericsignature/GenericSignatureDontOptimizeTest.java b/src/test/java/com/android/tools/r8/graph/genericsignature/GenericSignatureDontOptimizeTest.java
index 7c55472..94adea2 100644
--- a/src/test/java/com/android/tools/r8/graph/genericsignature/GenericSignatureDontOptimizeTest.java
+++ b/src/test/java/com/android/tools/r8/graph/genericsignature/GenericSignatureDontOptimizeTest.java
@@ -7,6 +7,7 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
@@ -45,11 +46,10 @@
.addKeepClassRules(Foo.class)
.addKeepMainRule(Main.class)
.addDontOptimize()
- .run(parameters.getRuntime(), Main.class)
+ .compile()
.inspect(this::inspect)
- // TODO(b/221404266): We should fail with an exception since the parameterized generic type
- // should be removed.
- .assertSuccessWithOutputLines(EXPECTED);
+ .run(parameters.getRuntime(), Main.class)
+ .assertFailureWithErrorThatThrows(ClassCastException.class);
}
private void inspect(CodeInspector inspector) {
@@ -57,11 +57,9 @@
assertThat(foo, isPresent());
assertEquals("<T:Ljava/lang/Object;>Ljava/lang/Object;", foo.getFinalSignatureAttribute());
- // TODO(b/221404266): We should not keep generic signatures when having -dontoptimize
ClassSubject main$1 = inspector.clazz(Main.class.getTypeName() + "$1");
assertThat(main$1, isPresent());
- assertEquals(
- "L" + binaryName(Foo.class) + "<Ljava/lang/String;>;", main$1.getFinalSignatureAttribute());
+ assertNull(main$1.getFinalSignatureAttribute());
}
public static class Foo<T> {
