Fail on partially implemented Proguard options
The following options are parsed, but not fully implemented:
-keepdirectories
-adaptresourcefilenames
-adaptresourcefilecontents
Bug: 37139570
Bug: 74279367
Bug: 76377381
Bug: 36847655
Change-Id: I588311e6fa73aec8d1df4862bffe76ec7c43fcf7
diff --git a/src/main/java/com/android/tools/r8/R8Command.java b/src/main/java/com/android/tools/r8/R8Command.java
index 4fc0590..573f6df 100644
--- a/src/main/java/com/android/tools/r8/R8Command.java
+++ b/src/main/java/com/android/tools/r8/R8Command.java
@@ -66,6 +66,8 @@
// Internal compatibility mode for use from CompatProguard tool.
Path proguardCompatibilityRulesOutput = null;
+ private boolean allowPartiallyImplementedProguardOptions = false;
+
private StringConsumer mainDexListConsumer = null;
// TODO(zerny): Consider refactoring CompatProguardCommandBuilder to avoid subclassing.
@@ -286,7 +288,8 @@
mainDexKeepRules = parser.getConfig().getRules();
}
- ProguardConfigurationParser parser = new ProguardConfigurationParser(factory, reporter);
+ ProguardConfigurationParser parser = new ProguardConfigurationParser(
+ factory, reporter, !allowPartiallyImplementedProguardOptions);
if (!proguardConfigs.isEmpty()) {
parser.parse(proguardConfigs);
}
@@ -384,6 +387,11 @@
c.accept(builder);
};
}
+
+ // Internal for-testing method to add post-processors of the proguard configuration.
+ void allowPartiallyImplementedProguardOptions() {
+ allowPartiallyImplementedProguardOptions = true;
+ }
}
// Wrapper class to ensure that R8 does not allow DEX as program inputs.
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 4a25e42..ed9cbdb 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationParser.java
@@ -47,6 +47,7 @@
private final DexItemFactory dexItemFactory;
private final Reporter reporter;
+ private final boolean failOnPartiallyImplementedOptions;
private static final List<String> IGNORED_SINGLE_ARG_OPTIONS = ImmutableList.of(
"protomapping",
@@ -96,10 +97,16 @@
public ProguardConfigurationParser(
DexItemFactory dexItemFactory, Reporter reporter) {
+ this(dexItemFactory, reporter, true);
+ }
+
+ public ProguardConfigurationParser(
+ DexItemFactory dexItemFactory, Reporter reporter, boolean failOnPartiallyImplementedOptions) {
this.dexItemFactory = dexItemFactory;
configurationBuilder = ProguardConfiguration.builder(dexItemFactory, reporter);
this.reporter = reporter;
+ this.failOnPartiallyImplementedOptions = failOnPartiallyImplementedOptions;
}
public ProguardConfiguration.Builder getConfigurationBuilder() {
@@ -219,6 +226,10 @@
ProguardCheckDiscardRule rule = parseCheckDiscardRule();
configurationBuilder.addRule(rule);
} else if (acceptString("keepdirectories")) {
+ // TODO(74279367): Report an error until it's fully supported.
+ if (failOnPartiallyImplementedOptions) {
+ failPartiallyImplementedOption("-keepdirectories", optionStart);
+ }
parsePathFilter(configurationBuilder::addKeepDirectories);
} else if (acceptString("keep")) {
ProguardKeepRule rule = parseKeepRule();
@@ -339,11 +350,17 @@
} else if (acceptString("adaptclassstrings")) {
parseClassFilter(configurationBuilder::addAdaptClassStringsPattern);
} else if (acceptString("adaptresourcefilenames")) {
- // TODO(b/76377381): should be report an error until it's fully supported.
+ // TODO(76377381): Report an error until it's fully supported.
+ if (failOnPartiallyImplementedOptions) {
+ failPartiallyImplementedOption("-adaptresourcefilenames", optionStart);
+ }
parsePathFilter(configurationBuilder::addAdaptResourceFilenames);
} else if (acceptString("adaptresourcefilecontents")) {
+ // TODO(36847655): Report an error until it's fully supported.
+ if (failOnPartiallyImplementedOptions) {
+ failPartiallyImplementedOption("-adaptresourcefilecontents", optionStart);
+ }
parsePathFilter(configurationBuilder::addAdaptResourceFilecontents);
- // TODO(b/37139570): should be report an error until it's fully supported.
} else if (acceptString("identifiernamestring")) {
configurationBuilder.addRule(parseIdentifierNameStringRule());
} else if (acceptString("if")) {
@@ -1473,6 +1490,11 @@
"Option -" + optionName + " overrides -" + victim, origin, getPosition(start)));
}
+ private void failPartiallyImplementedOption(String optionName, TextPosition start) {
+ throw reporter.fatalError(new StringDiagnostic(
+ "Option " + optionName + " currently not supported", origin, getPosition(start)));
+ }
+
private Position getPosition(TextPosition start) {
if (start.getOffset() == position) {
return start;
diff --git a/src/test/java/com/android/tools/r8/ToolHelper.java b/src/test/java/com/android/tools/r8/ToolHelper.java
index afa8c12..b433474 100644
--- a/src/test/java/com/android/tools/r8/ToolHelper.java
+++ b/src/test/java/com/android/tools/r8/ToolHelper.java
@@ -1328,6 +1328,12 @@
return builder;
}
+ public static R8Command.Builder allowPartiallyImplementedProguardOptions(
+ R8Command.Builder builder) {
+ builder.allowPartiallyImplementedProguardOptions();
+ return builder;
+ }
+
public static AndroidApp getApp(BaseCommand command) {
return command.getInputApp();
}
diff --git a/src/test/java/com/android/tools/r8/internal/CompilationTestBase.java b/src/test/java/com/android/tools/r8/internal/CompilationTestBase.java
index a542b70..dfe24c5 100644
--- a/src/test/java/com/android/tools/r8/internal/CompilationTestBase.java
+++ b/src/test/java/com/android/tools/r8/internal/CompilationTestBase.java
@@ -99,6 +99,7 @@
builder.setMode(mode);
builder.setProgramConsumer(DexIndexedConsumer.emptyConsumer());
builder.setMinApiLevel(AndroidApiLevel.L.getLevel());
+ ToolHelper.allowPartiallyImplementedProguardOptions(builder);
ToolHelper.addProguardConfigurationConsumer(
builder,
pgConfig -> {
diff --git a/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java b/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
index f558ee7..c176ff3 100644
--- a/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ProguardConfigurationParserTest.java
@@ -149,6 +149,13 @@
parser = new ProguardConfigurationParser(new DexItemFactory(), reporter);
}
+ @Before
+ public void resetAllowPartiallyImplementedOptions() {
+ handler = new KeepingDiagnosticHandler();
+ reporter = new Reporter(handler);
+ parser = new ProguardConfigurationParser(new DexItemFactory(), reporter, false);
+ }
+
@Test
public void parse() throws Exception {
ProguardConfigurationParser parser;
@@ -859,7 +866,7 @@
@Test
public void parseKeepdirectories() throws Exception {
ProguardConfigurationParser parser =
- new ProguardConfigurationParser(new DexItemFactory(), reporter);
+ new ProguardConfigurationParser(new DexItemFactory(), reporter, false);
parser.parse(Paths.get(KEEPDIRECTORIES));
verifyParserEndsCleanly();
}
@@ -1216,6 +1223,7 @@
@Test
public void parse_adaptresourcexxx_keepdirectories_noArguments1() {
+ resetAllowPartiallyImplementedOptions();
ProguardConfiguration config = parseAndVerifyParserEndsCleanly(ImmutableList.of(
"-adaptresourcefilenames",
"-adaptresourcefilecontents",
@@ -1228,6 +1236,7 @@
@Test
public void parse_adaptresourcexxx_keepdirectories_noArguments2() {
+ resetAllowPartiallyImplementedOptions();
ProguardConfiguration config = parseAndVerifyParserEndsCleanly(ImmutableList.of(
"-keepdirectories",
"-adaptresourcefilenames",
@@ -1240,6 +1249,7 @@
@Test
public void parse_adaptresourcexxx_keepdirectories_noArguments3() {
+ resetAllowPartiallyImplementedOptions();
ProguardConfiguration config = parseAndVerifyParserEndsCleanly(ImmutableList.of(
"-adaptresourcefilecontents",
"-keepdirectories",
@@ -1261,6 +1271,7 @@
@Test
public void parse_adaptresourcexxx_keepdirectories_singleArgument() {
+ resetAllowPartiallyImplementedOptions();
ProguardConfiguration config = parseAndVerifyParserEndsCleanly(ImmutableList.of(
"-adaptresourcefilenames " + FILE_FILTER_SINGLE,
"-adaptresourcefilecontents " + FILE_FILTER_SINGLE,
@@ -1293,6 +1304,7 @@
@Test
public void parse_adaptresourcexxx_keepdirectories_multipleArgument() {
+ resetAllowPartiallyImplementedOptions();
ProguardConfiguration config = parseAndVerifyParserEndsCleanly(ImmutableList.of(
"-adaptresourcefilenames " + FILE_FILTER_MULTIPLE,
"-adaptresourcefilecontents " + FILE_FILTER_MULTIPLE,
@@ -1309,7 +1321,7 @@
"-adaptresourcefilenames", "-adaptresourcefilecontents", "-keepdirectories");
for (String option : options) {
try {
- reset();
+ resetAllowPartiallyImplementedOptions();
parser.parse(createConfigurationForTesting(ImmutableList.of(option + " ,")));
fail("Expect to fail due to the lack of path filter.");
} catch (AbortException e) {
@@ -1324,7 +1336,7 @@
"-adaptresourcefilenames", "-adaptresourcefilecontents", "-keepdirectories");
for (String option : options) {
try {
- reset();
+ resetAllowPartiallyImplementedOptions();
parser.parse(createConfigurationForTesting(ImmutableList.of(option + " xxx,,yyy")));
fail("Expect to fail due to the lack of path filter.");
} catch (AbortException e) {
@@ -1339,7 +1351,7 @@
"-adaptresourcefilenames", "-adaptresourcefilecontents", "-keepdirectories");
for (String option : options) {
try {
- reset();
+ resetAllowPartiallyImplementedOptions();
parser.parse(createConfigurationForTesting(ImmutableList.of(option + " xxx,")));
fail("Expect to fail due to the lack of path filter.");
} catch (AbortException e) {
@@ -1745,6 +1757,24 @@
verifyWithProguard(proguardConfig);
}
+ public void testNotSupported(String option) {
+ try {
+ reset();
+ parser.parse(createConfigurationForTesting(ImmutableList.of(option)));
+ fail("Expect to fail due to unsupported option.");
+ } catch (AbortException e) {
+ checkDiagnostic(handler.errors, null, 1, 1, "Option " + option + " currently not supported");
+ }
+ }
+
+ @Test
+ public void parse_pariallyImplemented_notSupported() {
+ testNotSupported("-keepdirectories");
+ testNotSupported("-adaptresourcefilenames");
+ testNotSupported("-adaptresourcefilecontents");
+ }
+
+
private ProguardConfiguration parseAndVerifyParserEndsCleanly(List<String> config) {
parser.parse(createConfigurationForTesting(config));
verifyParserEndsCleanly();
