Handle problematic resource qualifiers in references to individual resources

ResourceFilter already handles problematic qualifiers in the
resource_configuration_filters attribute. Also handle them in references to
individual resources.

RELNOTES: none
PiperOrigin-RevId: 160969753

3 files changed, 128 insertions, 15 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
index 76a8eb702c..5e9ec5fda4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
@@ -220,7 +220,7 @@ public class ResourceFilter {
     // Clean up deprecated representations of resource qualifiers that FolderConfiguration can't
     // handle.
     for (DeprecatedQualifierHandler handler : deprecatedQualifierHandlers) {
-      filter = handler.replaceIfNeeded(ruleErrorConsumer, filter);
+      filter = handler.fixAttributeIfNeeded(ruleErrorConsumer, filter);
     }
 
     return FolderConfiguration.getConfigForQualifierString(filter);
@@ -231,7 +231,8 @@ public class ResourceFilter {
     private final String replacement;
     private final String description;
 
-    private boolean warned = false;
+    private boolean warnedForAttribute = false;
+    private boolean warnedForResources = false;
 
     private DeprecatedQualifierHandler(String pattern, String replacement, String description) {
       this.pattern = Pattern.compile(pattern);
@@ -239,7 +240,7 @@ public class ResourceFilter {
       this.description = description;
     }
 
-    private String replaceIfNeeded(RuleErrorConsumer ruleErrorConsumer, String qualifier) {
+    private String fixAttributeIfNeeded(RuleErrorConsumer ruleErrorConsumer, String qualifier) {
       Matcher matcher = pattern.matcher(qualifier);
 
       if (!matcher.matches()) {
@@ -248,15 +249,40 @@ public class ResourceFilter {
 
       String fixed = matcher.replaceFirst(replacement);
       // We don't want to spam users. Only warn about this kind of issue once per target.
-      // TODO(asteinb): Will this cause problems when settings are propogated via dynamic
+      // TODO(asteinb): Will this cause problems when settings are propagated via dynamic
       // configuration?
-      if (!warned) {
+      if (!warnedForAttribute) {
         ruleErrorConsumer.attributeWarning(
             RESOURCE_CONFIGURATION_FILTERS_NAME,
             String.format(
                 "When referring to %s, use of qualifier '%s' is deprecated. Use '%s' instead.",
                 description, matcher.group(), fixed));
-        warned = true;
+        warnedForAttribute = true;
+      }
+      return fixed;
+    }
+
+    private String fixResourceIfNeeded(
+        RuleErrorConsumer ruleErrorConsumer, String qualifier, String resourceFolder) {
+      Matcher matcher = pattern.matcher(qualifier);
+
+      if (!matcher.matches()) {
+        return qualifier;
+      }
+
+      String fixed = matcher.replaceFirst(replacement);
+
+      // We don't want to spam users. Only warn about this kind of issue once per target.
+      // TODO(asteinb): Will this cause problems when settings are propagated via dynamic
+      // configuration?
+      if (!warnedForResources) {
+        warnedForResources = true;
+
+        ruleErrorConsumer.ruleWarning(
+            String.format(
+                "For resource folder %s, when referring to %s, use of qualifier '%s' is deprecated."
+                    + " Use '%s' instead.",
+                resourceFolder, description, matcher.group(), fixed));
       }
 
       return fixed;
@@ -422,7 +448,7 @@ public class ResourceFilter {
    * Tracks the best artifact for a desired density for each combination of filename and non-density
    * qualifiers.
    */
-  private static class BestArtifactsForDensity {
+  private class BestArtifactsForDensity {
     private final RuleErrorConsumer ruleErrorConsumer;
     private final Density desiredDensity;
     private final Map<String, Artifact> nameAndConfigurationToBestArtifact = new HashMap<>();
@@ -519,9 +545,22 @@ public class ResourceFilter {
     }
   }
 
-  private static FolderConfiguration getConfigForArtifact(
+  private FolderConfiguration getConfigForArtifact(
       RuleErrorConsumer ruleErrorConsumer, Artifact artifact) {
     String containingFolder = getContainingFolder(artifact);
+
+    if (containingFolder.contains("-")) {
+      String[] parts = containingFolder.split("-", 2);
+      String prefix = parts[0];
+      String qualifiers = parts[1];
+
+      for (DeprecatedQualifierHandler handler : deprecatedQualifierHandlers) {
+        qualifiers = handler.fixResourceIfNeeded(ruleErrorConsumer, qualifiers, containingFolder);
+      }
+
+      containingFolder = String.format("%s-%s", prefix, qualifiers);
+    }
+
     FolderConfiguration config = FolderConfiguration.getConfigForFolder(containingFolder);
 
     if (config == null) {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
index b7aaf7d1d0..5ad26b7637 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
@@ -154,7 +154,11 @@ public class ResourceFilterTest extends ResourceTestBase {
         ImmutableList.of("sr-Latn", "sr-rLatn", "sr_Latn", "b+sr+Latn"),
         ImmutableList.of(),
         FilterBehavior.FILTER_IN_ANALYSIS,
-        ImmutableList.of("values-sr/foo.xml", "values-b+sr+Latn/foo.xml"),
+        ImmutableList.of(
+            "values-sr/foo.xml",
+            "values-b+sr+Latn/foo.xml",
+            "values-sr-Latn/foo.xml",
+            "values-sr-rLatn/foo.xml"),
         // Serbian in explicitly Cyrillic characters should be filtered out.
         ImmutableList.of("values-b+sr+Cyrl/foo.xml"));
 
@@ -171,7 +175,11 @@ public class ResourceFilterTest extends ResourceTestBase {
         ImmutableList.of(),
         FilterBehavior.FILTER_IN_ANALYSIS,
         ImmutableList.of(
-            "values-sr/foo.xml", "values-b+sr+Latn/foo.xml", "values-b+sr+Cyrl/foo.xml"));
+            "values-sr/foo.xml",
+            "values-b+sr+Latn/foo.xml",
+            "values-b+sr+Cyrl/foo.xml",
+            "values-sr-Latn/foo.xml",
+            "values-sr-rLatn/foo.xml"));
 
     errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
@@ -182,7 +190,7 @@ public class ResourceFilterTest extends ResourceTestBase {
         ImmutableList.of("es-419", "es_419", "b+es+419"),
         ImmutableList.of(),
         FilterBehavior.FILTER_IN_ANALYSIS,
-        ImmutableList.of("values-es/foo.xml", "values-b+es+419/foo.xml"),
+        ImmutableList.of("values-es/foo.xml", "values-b+es+419/foo.xml", "values-es-419/foo.xml"),
         // Spanish with another region specified should be filtered out.
         ImmutableList.of("values-es-rUS/foo.xml"));
 
@@ -198,7 +206,11 @@ public class ResourceFilterTest extends ResourceTestBase {
         ImmutableList.of("es"),
         ImmutableList.of(),
         FilterBehavior.FILTER_IN_ANALYSIS,
-        ImmutableList.of("values-es/foo.xml", "values-b+es+419/foo.xml", "values-es-rUS/foo.xml"));
+        ImmutableList.of(
+            "values-es/foo.xml",
+            "values-b+es+419/foo.xml",
+            "values-es-rUS/foo.xml",
+            "values-es-419/foo.xml"));
 
     errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
@@ -208,9 +220,9 @@ public class ResourceFilterTest extends ResourceTestBase {
    * problem in each resource filter object.
    */
   @Test
-  public void testFilterDeprecatedQualifierFormatsWarnings() {
+  public void testFilterDeprecatedQualifierFormatsAttributeWarnings() {
     ImmutableList<String> badQualifiers =
-        ImmutableList.of("en_US", "sr-Latn", "es-419", "mcc310-en_US", "sr_Latn", "es_419");
+        ImmutableList.of("en_US", "sr-Latn", "es-419", "mcc310-en_US", "sr_rLatn", "es_419");
 
     ImmutableList<String> expectedWarnings =
         ImmutableList.of(
@@ -229,10 +241,12 @@ public class ResourceFilterTest extends ResourceTestBase {
             errorConsumer.getAndClearAttributeWarnings(
                 ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME))
         .containsExactlyElementsIn(expectedWarnings);
+    errorConsumer.assertNoRuleWarnings();
 
     // Filtering again with this filter should not produce additional warnings
     filter.filter(errorConsumer, ImmutableList.of());
     errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
+    errorConsumer.assertNoRuleWarnings();
 
     // Filtering with a new filter should produce warnings again, since it is working on a different
     // target
@@ -243,6 +257,50 @@ public class ResourceFilterTest extends ResourceTestBase {
             errorConsumer.getAndClearAttributeWarnings(
                 ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME))
         .containsExactlyElementsIn(expectedWarnings);
+    errorConsumer.assertNoRuleWarnings();
+  }
+
+  @Test
+  public void testFilterDeprecatedQualifierFormatsRuleWarnings() {
+    ImmutableList<Artifact> badResources =
+        getResources(
+            "values-es_US/foo.xml",
+            "drawables-sr-Latn/foo.xml",
+            "stylables-es-419/foo.xml",
+            "values-mcc310-es_US/foo.xml",
+            "values-sr_rLatn/foo.xml",
+            "drawables-es_419/foo.xml");
+
+    ImmutableList<String> expectedWarnings =
+        ImmutableList.of(
+            "For resource folder drawables-sr-Latn, when referring to Serbian in Latin characters, "
+                + "use of qualifier 'sr-Latn' is deprecated. Use 'b+sr+Latn' instead.",
+            "For resource folder stylables-es-419, when referring to Spanish for Latin America and "
+                + "the Caribbean, use of qualifier 'es-419' is deprecated. Use 'b+es+419' instead.",
+            "For resource folder values-es_US, when referring to locale qualifiers with regions, "
+                + "use of qualifier 'es_US' is deprecated. Use 'es-rUS' instead.");
+
+    ResourceFilter filter =
+        new ResourceFilter(
+            ImmutableList.of("en"), ImmutableList.of(), FilterBehavior.FILTER_IN_ANALYSIS);
+
+    filter.filter(errorConsumer, badResources);
+    assertThat(errorConsumer.getAndClearRuleWarnings()).containsExactlyElementsIn(expectedWarnings);
+    errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
+
+    // Filtering again with this filter should not produce additional warnings
+    filter.filter(errorConsumer, badResources);
+    errorConsumer.assertNoRuleWarnings();
+    errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
+
+    // Filtering with a new filter should produce warnings again, since it is working on a different
+    // target
+    filter =
+        new ResourceFilter(
+            ImmutableList.of("en"), ImmutableList.of(), FilterBehavior.FILTER_IN_ANALYSIS);
+    filter.filter(errorConsumer, badResources);
+    assertThat(errorConsumer.getAndClearRuleWarnings()).containsExactlyElementsIn(expectedWarnings);
+    errorConsumer.assertNoAttributeWarnings(ResourceFilter.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   private void testNoopFilter(
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
index cada5518d9..27ca3510df 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
@@ -28,7 +28,9 @@ import com.google.devtools.build.lib.packages.RuleErrorConsumer;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 import org.junit.After;
 import org.junit.Before;
 
@@ -51,12 +53,16 @@ public abstract class ResourceTestBase {
     private String attributeErrorAttribute = null;
     private String attributeErrorMessage = null;
 
+    private final List<String> ruleWarnings = new ArrayList<>();
+
     // Use an ArrayListMultimap since it allows duplicates - we'll want to know if a warning is
     // reported twice.
     private final Multimap<String, String> attributeWarnings = ArrayListMultimap.create();
 
     @Override
-    public void ruleWarning(String message) {}
+    public void ruleWarning(String message) {
+      ruleWarnings.add(message);
+    }
 
     @Override
     public void ruleError(String message) {
@@ -74,6 +80,16 @@ public abstract class ResourceTestBase {
       attributeErrorMessage = message;
     }
 
+    public Collection<String> getAndClearRuleWarnings() {
+      Collection<String> warnings = ImmutableList.copyOf(ruleWarnings);
+      ruleWarnings.clear();
+      return warnings;
+    }
+
+    public void assertNoRuleWarnings() {
+      assertThat(ruleWarnings).isEmpty();
+    }
+
     public Collection<String> getAndClearAttributeWarnings(String attrName) {
       if (!attributeWarnings.containsKey(attrName)) {
         return ImmutableList.of();