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();