Warn when default init member rules are synthesized
This also changes the current default behavior so that the synthesis of default <init> member rules is enabled.
Bug: b/356350498
Change-Id: I1b2d3ca2b038831ec81e432cd8c80c6b0b655ed8
diff --git a/src/main/java/com/android/tools/r8/L8Command.java b/src/main/java/com/android/tools/r8/L8Command.java
index 339e80b..0bd58e6 100644
--- a/src/main/java/com/android/tools/r8/L8Command.java
+++ b/src/main/java/com/android/tools/r8/L8Command.java
@@ -387,6 +387,7 @@
R8Command.Builder r8Builder =
R8Command.builder(getReporter())
.addProgramResourceProvider((ProgramResourceProvider) l8CfConsumer)
+ .setEnableEmptyMemberRulesToDefaultInitRuleConversion(false)
.setSynthesizedClassesPrefix(
desugaredLibrarySpecification.getSynthesizedLibraryClassesPackagePrefix())
.setMinApiLevel(getMinApiLevel())
diff --git a/src/main/java/com/android/tools/r8/errors/EmptyMemberRulesToDefaultInitRuleConversionDiagnostic.java b/src/main/java/com/android/tools/r8/errors/EmptyMemberRulesToDefaultInitRuleConversionDiagnostic.java
new file mode 100644
index 0000000..9a441ce
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/errors/EmptyMemberRulesToDefaultInitRuleConversionDiagnostic.java
@@ -0,0 +1,52 @@
+// Copyright (c) 2024, 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.errors;
+
+import com.android.tools.r8.Diagnostic;
+import com.android.tools.r8.keepanno.annotations.KeepForApi;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.Position;
+import com.android.tools.r8.shaking.ProguardConfigurationRule;
+import com.android.tools.r8.utils.StringUtils;
+
+@KeepForApi
+public class EmptyMemberRulesToDefaultInitRuleConversionDiagnostic implements Diagnostic {
+
+ private final ProguardConfigurationRule rule;
+
+ EmptyMemberRulesToDefaultInitRuleConversionDiagnostic(ProguardConfigurationRule rule) {
+ this.rule = rule;
+ }
+
+ @Override
+ public Origin getOrigin() {
+ return rule.getOrigin();
+ }
+
+ @Override
+ public Position getPosition() {
+ return rule.getPosition();
+ }
+
+ @Override
+ public String getDiagnosticMessage() {
+ // TODO(b/356350498): Explain how to opt-in to the new behavior when this is implemented in AGP.
+ return StringUtils.joinLines(
+ "The current version of R8 implicitly keeps the default constructor for Proguard"
+ + " configuration rules that have no member pattern. If the following rule should"
+ + " continue to keep the default constructor in the next major version of R8, then it"
+ + " must be augmented with the member pattern `{ void <init>(); }` to explicitly keep"
+ + " the default constructor:",
+ rule.toString());
+ }
+
+ // Non-kept factory to avoid that the constructor of the diagnostic is public API.
+ public static class Factory {
+
+ public static EmptyMemberRulesToDefaultInitRuleConversionDiagnostic create(
+ ProguardConfigurationRule rule) {
+ return new EmptyMemberRulesToDefaultInitRuleConversionDiagnostic(rule);
+ }
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
index fc42207..f511bee 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
@@ -10,6 +10,7 @@
import com.android.tools.r8.InputDependencyGraphConsumer;
import com.android.tools.r8.Version;
import com.android.tools.r8.dex.Constants;
+import com.android.tools.r8.errors.EmptyMemberRulesToDefaultInitRuleConversionDiagnostic;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
@@ -32,6 +33,7 @@
import com.android.tools.r8.utils.ThrowingAction;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
@@ -127,6 +129,7 @@
dexItemFactory,
reporter,
ProguardConfigurationParserOptions.builder()
+ .setEnableEmptyMemberRulesToDefaultInitRuleConversion(false)
.setEnableExperimentalCheckEnumUnboxed(false)
.setEnableExperimentalConvertCheckNotNull(false)
.setEnableExperimentalWhyAreYouNotInlining(false)
@@ -846,28 +849,31 @@
.setStart(start);
parseRuleTypeAndModifiers(keepRuleBuilder);
parseClassSpec(keepRuleBuilder);
- if (options.isEmptyMemberRulesToDefaultInitRuleConversionEnabled(configurationBuilder)) {
- if (keepRuleBuilder.getMemberRules().isEmpty()
- && keepRuleBuilder.getKeepRuleType()
- != ProguardKeepRuleType.KEEP_CLASSES_WITH_MEMBERS) {
- // If there are no member rules, a default rule for the parameterless constructor applies
- // in compatibility mode.
- keepRuleBuilder
- .getMemberRules()
- .add(
- ProguardMemberRule.builder()
- .setName(
- IdentifierPatternWithWildcards.withoutWildcards(
- Constants.INSTANCE_INITIALIZER_NAME))
- .setRuleType(ProguardMemberType.INIT)
- .setArguments(Collections.emptyList())
- .build());
- }
- }
Position end = getPosition();
- keepRuleBuilder.setSource(getSourceSnippet(contents, start, end));
- keepRuleBuilder.setEnd(end);
- return keepRuleBuilder.build();
+ ProguardKeepRule rule =
+ keepRuleBuilder.setSource(getSourceSnippet(contents, start, end)).setEnd(end).build();
+ if (options.isEmptyMemberRulesToDefaultInitRuleConversionEnabled(configurationBuilder)
+ && rule.getMemberRules().isEmpty()
+ && rule.getType() != ProguardKeepRuleType.KEEP_CLASSES_WITH_MEMBERS) {
+ // If there are no member rules, a default rule for the parameterless constructor applies
+ // in compatibility mode.
+ if (options.isEmptyMemberRulesToDefaultInitRuleConversionWarningsEnabled(
+ configurationBuilder)) {
+ reporter.warning(
+ EmptyMemberRulesToDefaultInitRuleConversionDiagnostic.Factory.create(rule));
+ }
+ List<ProguardMemberRule> memberRules =
+ Lists.newArrayList(
+ ProguardMemberRule.builder()
+ .setName(
+ IdentifierPatternWithWildcards.withoutWildcards(
+ Constants.INSTANCE_INITIALIZER_NAME))
+ .setRuleType(ProguardMemberType.INIT)
+ .setArguments(Collections.emptyList())
+ .build());
+ rule = keepRuleBuilder.setMemberRules(memberRules).build();
+ }
+ return rule;
}
@SuppressWarnings("NonCanonicalType")
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParserOptions.java b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParserOptions.java
index 157be68..07bea60 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParserOptions.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParserOptions.java
@@ -6,16 +6,18 @@
import static com.android.tools.r8.utils.SystemPropertyUtils.parseSystemPropertyOrDefault;
+import com.android.tools.r8.utils.OptionalBool;
+
public class ProguardConfigurationParserOptions {
- private final boolean enableEmptyMemberRulesToDefaultInitRuleConversion;
+ private final OptionalBool enableEmptyMemberRulesToDefaultInitRuleConversion;
private final boolean enableExperimentalCheckEnumUnboxed;
private final boolean enableExperimentalConvertCheckNotNull;
private final boolean enableExperimentalWhyAreYouNotInlining;
private final boolean enableTestingOptions;
ProguardConfigurationParserOptions(
- boolean enableEmptyMemberRulesToDefaultInitRuleConversion,
+ OptionalBool enableEmptyMemberRulesToDefaultInitRuleConversion,
boolean enableExperimentalCheckEnumUnboxed,
boolean enableExperimentalConvertCheckNotNull,
boolean enableExperimentalWhyAreYouNotInlining,
@@ -34,8 +36,16 @@
public boolean isEmptyMemberRulesToDefaultInitRuleConversionEnabled(
ProguardConfiguration.Builder configurationBuilder) {
- return enableEmptyMemberRulesToDefaultInitRuleConversion
- || configurationBuilder.isForceProguardCompatibility();
+ // TODO(b/356344563): Disable in full mode in the next major version.
+ return configurationBuilder.isForceProguardCompatibility()
+ || enableEmptyMemberRulesToDefaultInitRuleConversion.getOrDefault(true);
+ }
+
+ public boolean isEmptyMemberRulesToDefaultInitRuleConversionWarningsEnabled(
+ ProguardConfiguration.Builder configurationBuilder) {
+ assert isEmptyMemberRulesToDefaultInitRuleConversionEnabled(configurationBuilder);
+ return !configurationBuilder.isForceProguardCompatibility()
+ && enableEmptyMemberRulesToDefaultInitRuleConversion.isUnknown();
}
public boolean isExperimentalCheckEnumUnboxedEnabled() {
@@ -56,7 +66,7 @@
public static class Builder {
- private boolean enableEmptyMemberRulesToDefaultInitRuleConversion;
+ private OptionalBool enableEmptyMemberRulesToDefaultInitRuleConversion = OptionalBool.UNKNOWN;
private boolean enableExperimentalCheckEnumUnboxed;
private boolean enableExperimentalConvertCheckNotNull;
private boolean enableExperimentalWhyAreYouNotInlining;
@@ -65,7 +75,8 @@
public Builder readEnvironment() {
enableEmptyMemberRulesToDefaultInitRuleConversion =
parseSystemPropertyOrDefault(
- "com.android.tools.r8.enableEmptyMemberRulesToDefaultInitRuleConversion", false);
+ "com.android.tools.r8.enableEmptyMemberRulesToDefaultInitRuleConversion",
+ OptionalBool.UNKNOWN);
enableExperimentalCheckEnumUnboxed =
parseSystemPropertyOrDefault(
"com.android.tools.r8.experimental.enablecheckenumunboxed", false);
@@ -83,7 +94,7 @@
public Builder setEnableEmptyMemberRulesToDefaultInitRuleConversion(
boolean enableEmptyMemberRulesToDefaultInitRuleConversion) {
this.enableEmptyMemberRulesToDefaultInitRuleConversion =
- enableEmptyMemberRulesToDefaultInitRuleConversion;
+ OptionalBool.of(enableEmptyMemberRulesToDefaultInitRuleConversion);
return this;
}
diff --git a/src/main/java/com/android/tools/r8/utils/OptionalBool.java b/src/main/java/com/android/tools/r8/utils/OptionalBool.java
index 06cd098..b7c0708 100644
--- a/src/main/java/com/android/tools/r8/utils/OptionalBool.java
+++ b/src/main/java/com/android/tools/r8/utils/OptionalBool.java
@@ -58,6 +58,11 @@
new OptionalBool() {
@Override
+ public boolean getOrDefault(boolean defaultValue) {
+ return defaultValue;
+ }
+
+ @Override
public boolean isUnknown() {
return true;
}
@@ -93,6 +98,10 @@
return this;
}
+ public boolean getOrDefault(boolean defaultValue) {
+ return toBoolean();
+ }
+
public abstract boolean toBoolean();
@Override
diff --git a/src/main/java/com/android/tools/r8/utils/SystemPropertyUtils.java b/src/main/java/com/android/tools/r8/utils/SystemPropertyUtils.java
index 57cb56d..33dffa1 100644
--- a/src/main/java/com/android/tools/r8/utils/SystemPropertyUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/SystemPropertyUtils.java
@@ -7,6 +7,8 @@
import static com.google.common.base.Predicates.alwaysTrue;
import com.android.tools.r8.Version;
+import com.android.tools.r8.errors.Unreachable;
+import java.util.function.BooleanSupplier;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
@@ -59,19 +61,34 @@
public static boolean parseSystemPropertyOrDefault(String propertyName, boolean defaultValue) {
return internalParseSystemPropertyForDevelopmentOrDefault(
- propertyName, System.getProperty(propertyName), defaultValue);
+ propertyName, System.getProperty(propertyName), () -> defaultValue);
+ }
+
+ public static OptionalBool parseSystemPropertyOrDefault(
+ String propertyName, OptionalBool defaultValue) {
+ if (System.getProperty(propertyName) == null) {
+ return defaultValue;
+ }
+ boolean value =
+ internalParseSystemPropertyForDevelopmentOrDefault(
+ propertyName,
+ System.getProperty(propertyName),
+ () -> {
+ throw new Unreachable();
+ });
+ return OptionalBool.of(value);
}
public static boolean parseSystemPropertyForDevelopmentOrDefault(
String propertyName, boolean defaultValue) {
return internalParseSystemPropertyForDevelopmentOrDefault(
- propertyName, getSystemPropertyForDevelopment(propertyName), defaultValue);
+ propertyName, getSystemPropertyForDevelopment(propertyName), () -> defaultValue);
}
private static boolean internalParseSystemPropertyForDevelopmentOrDefault(
- String propertyName, String propertyValue, boolean defaultValue) {
+ String propertyName, String propertyValue, BooleanSupplier defaultValue) {
if (propertyValue == null) {
- return defaultValue;
+ return defaultValue.getAsBoolean();
}
if (StringUtils.isFalsy(propertyValue)) {
return false;
diff --git a/src/test/java/com/android/tools/r8/compilerapi/diagnostics/ProguardKeepRuleDiagnosticsApiTest.java b/src/test/java/com/android/tools/r8/compilerapi/diagnostics/ProguardKeepRuleDiagnosticsApiTest.java
index fedb449..e4ed3bb 100644
--- a/src/test/java/com/android/tools/r8/compilerapi/diagnostics/ProguardKeepRuleDiagnosticsApiTest.java
+++ b/src/test/java/com/android/tools/r8/compilerapi/diagnostics/ProguardKeepRuleDiagnosticsApiTest.java
@@ -98,6 +98,7 @@
.addProguardConfiguration(
Collections.singletonList("-keep class NotPresent {}"), Origin.unknown())
.addLibraryFiles(getJava8RuntimeJar())
+ .setEnableEmptyMemberRulesToDefaultInitRuleConversion(false)
.setProgramConsumer(DexIndexedConsumer.emptyConsumer())
.setMinApiLevel(1)
.build());
diff --git a/src/test/java/com/android/tools/r8/shaking/EmptyMemberRulesToDefaultInitRuleConversionTest.java b/src/test/java/com/android/tools/r8/shaking/EmptyMemberRulesToDefaultInitRuleConversionTest.java
index bdb0a85..564eac9 100644
--- a/src/test/java/com/android/tools/r8/shaking/EmptyMemberRulesToDefaultInitRuleConversionTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/EmptyMemberRulesToDefaultInitRuleConversionTest.java
@@ -3,13 +3,23 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.shaking;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticMessage;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticType;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf;
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assume.assumeTrue;
+import com.android.tools.r8.R8CompatTestBuilder;
+import com.android.tools.r8.R8TestBuilder;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.ThrowableConsumer;
+import com.android.tools.r8.errors.EmptyMemberRulesToDefaultInitRuleConversionDiagnostic;
import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.StringUtils;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -34,19 +44,63 @@
}
@Test
- public void testCompat() throws Exception {
+ public void testCompatDefault() throws Exception {
+ assumeTrue(enableEmptyMemberRulesToDefaultInitRuleConversion);
+ testCompat(R8TestBuilder::clearEnableEmptyMemberRulesToDefaultInitRuleConversion);
+ }
+
+ @Test
+ public void testCompatExplicit() throws Exception {
+ testCompat(
+ testBuilder ->
+ testBuilder.enableEmptyMemberRulesToDefaultInitRuleConversion(
+ enableEmptyMemberRulesToDefaultInitRuleConversion));
+ }
+
+ private void testCompat(ThrowableConsumer<? super R8CompatTestBuilder> configuration)
+ throws Exception {
testForR8Compat(parameters.getBackend())
.addInnerClasses(getClass())
.addKeepClassRules(Main.class)
- .enableEmptyMemberRulesToDefaultInitRuleConversion(
- enableEmptyMemberRulesToDefaultInitRuleConversion)
+ .apply(configuration)
.setMinApi(parameters)
.compile()
.inspect(inspector -> assertThat(inspector.clazz(Main.class).init(), isPresent()));
}
@Test
- public void testFull() throws Exception {
+ public void testFullDefault() throws Exception {
+ assumeTrue(enableEmptyMemberRulesToDefaultInitRuleConversion);
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addKeepClassRules(Main.class)
+ .allowDiagnosticWarningMessages()
+ .clearEnableEmptyMemberRulesToDefaultInitRuleConversion()
+ .setMinApi(parameters)
+ .compileWithExpectedDiagnostics(
+ diagnostics ->
+ diagnostics
+ .assertOnlyWarnings()
+ .assertWarningsMatch(
+ allOf(
+ diagnosticType(
+ EmptyMemberRulesToDefaultInitRuleConversionDiagnostic.class),
+ diagnosticMessage(
+ equalTo(
+ StringUtils.joinLines(
+ "The current version of R8 implicitly keeps the default"
+ + " constructor for Proguard configuration rules that"
+ + " have no member pattern. If the following rule"
+ + " should continue to keep the default constructor in"
+ + " the next major version of R8, then it must be"
+ + " augmented with the member pattern `{ void <init>();"
+ + " }` to explicitly keep the default constructor:",
+ "-keep class " + Main.class.getTypeName()))))))
+ .inspect(inspector -> assertThat(inspector.clazz(Main.class).init(), isPresent()));
+ }
+
+ @Test
+ public void testFullExplicit() throws Exception {
testForR8(parameters.getBackend())
.addInnerClasses(getClass())
.addKeepClassRules(Main.class)
diff --git a/src/test/testbase/java/com/android/tools/r8/R8TestBuilder.java b/src/test/testbase/java/com/android/tools/r8/R8TestBuilder.java
index 3309783..0b7d432 100644
--- a/src/test/testbase/java/com/android/tools/r8/R8TestBuilder.java
+++ b/src/test/testbase/java/com/android/tools/r8/R8TestBuilder.java
@@ -40,6 +40,7 @@
import com.android.tools.r8.utils.FileUtils;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.MapIdTemplateProvider;
+import com.android.tools.r8.utils.OptionalBool;
import com.android.tools.r8.utils.Pair;
import com.android.tools.r8.utils.SemanticVersion;
import com.android.tools.r8.utils.SourceFileTemplateProvider;
@@ -76,7 +77,7 @@
private AllowedDiagnosticMessages allowedDiagnosticMessages = AllowedDiagnosticMessages.NONE;
private boolean allowUnusedProguardConfigurationRules = false;
- private boolean enableEmptyMemberRulesToDefaultInitRuleConversion = false;
+ private OptionalBool enableEmptyMemberRulesToDefaultInitRuleConversion = OptionalBool.FALSE;
private boolean enableIsolatedSplits = false;
private boolean enableMissingLibraryApiModeling = true;
private boolean enableStartupLayoutOptimization = true;
@@ -140,8 +141,10 @@
ToolHelper.addSyntheticProguardRulesConsumerForTesting(
builder, rules -> box.syntheticProguardRules = rules);
libraryDesugaringTestConfiguration.configure(builder);
- builder.setEnableEmptyMemberRulesToDefaultInitRuleConversion(
- enableEmptyMemberRulesToDefaultInitRuleConversion);
+ if (!enableEmptyMemberRulesToDefaultInitRuleConversion.isUnknown()) {
+ builder.setEnableEmptyMemberRulesToDefaultInitRuleConversion(
+ enableEmptyMemberRulesToDefaultInitRuleConversion.toBoolean());
+ }
builder.setEnableIsolatedSplits(enableIsolatedSplits);
builder.setEnableExperimentalMissingLibraryApiModeling(enableMissingLibraryApiModeling);
builder.setEnableStartupLayoutOptimization(enableStartupLayoutOptimization);
@@ -888,7 +891,12 @@
public T enableEmptyMemberRulesToDefaultInitRuleConversion(
boolean enableEmptyMemberRulesToDefaultInitRuleConversion) {
this.enableEmptyMemberRulesToDefaultInitRuleConversion =
- enableEmptyMemberRulesToDefaultInitRuleConversion;
+ OptionalBool.of(enableEmptyMemberRulesToDefaultInitRuleConversion);
+ return self();
+ }
+
+ public T clearEnableEmptyMemberRulesToDefaultInitRuleConversion() {
+ this.enableEmptyMemberRulesToDefaultInitRuleConversion = OptionalBool.UNKNOWN;
return self();
}
diff --git a/tools/create_r8lib.py b/tools/create_r8lib.py
index c8eb00e..1f02869 100755
--- a/tools/create_r8lib.py
+++ b/tools/create_r8lib.py
@@ -95,6 +95,8 @@
'-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005'
])
cmd.append('-Dcom.android.tools.r8.enableKeepAnnotations=1')
+ # TODO(b/356344563): Remove when this is default.
+ cmd.append('-Dcom.android.tools.r8.enableEmptyMemberRulesToDefaultInitRuleConversion=0')
cmd.extend(['-cp', args.r8compiler, 'com.android.tools.r8.R8'])
cmd.append(args.r8jar)
if args.debug_variant:
