Make it an error to attempt to expand an attribute that does not exist

PiperOrigin-RevId: 171684595
agoulti · Oct 11, 2017 · 00d128c · 00d128c
1 parent 0257c29
commit 00d128c
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 16 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Expander.java b/src/main/java/com/google/devtools/build/lib/analysis/Expander.java
@@ -172,14 +172,10 @@ private ImmutableList<String> expandAndTokenizeList(
 
   /**
    * Obtains the value of the attribute, expands all values, and returns the resulting list. If the
-   * attribute does not exist or is not of type {@link Type#STRING_LIST}, then this method returns
-   * an empty list.
+   * attribute does not exist or is not of type {@link Type#STRING_LIST}, then this method throws
+   * an error.
    */
   public ImmutableList<String> list(String attrName) {
-    if (!ruleContext.getRule().isAttrDefined(attrName, Type.STRING_LIST)) {
-      // TODO(bazel-team): This should be an error.
-      return ImmutableList.of();
-    }
     return list(attrName, ruleContext.attributes().get(attrName, Type.STRING_LIST));
   }
 
@@ -192,13 +188,9 @@ public ImmutableList<String> list(String attrName, List<String> values) {
 
   /**
    * Obtains the value of the attribute, expands, and tokenizes all values. If the attribute does
-   * not exist or is not of type {@link Type#STRING_LIST}, then this method returns an empty list.
+   * not exist or is not of type {@link Type#STRING_LIST}, then this method throws an error.
    */
   public ImmutableList<String> tokenized(String attrName) {
-    if (!ruleContext.getRule().isAttrDefined(attrName, Type.STRING_LIST)) {
-      // TODO(bazel-team): This should be an error.
-      return ImmutableList.of();
-    }
     return tokenized(attrName, ruleContext.attributes().get(attrName, Type.STRING_LIST));
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -427,6 +428,11 @@ public static RunfilesSupport withExecutable(
   private static CommandLine computeArgs(
       RuleContext ruleContext,
       CommandLine additionalArgs) {
+    if (!ruleContext.getRule().isAttrDefined("args", Type.STRING_LIST)) {
+      // Some non-_binary rules create RunfilesSupport instances; it is fine to not have an args
+      // attribute here.
+      return additionalArgs;
+    }
     return CommandLine.concat(
         ruleContext.getExpander().withDataLocations().tokenized("args"),
         additionalArgs);

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -193,6 +193,8 @@ public final class AndroidRuleClasses {
       fromTemplates("%{name}_images/emulator-meta-data.pb");
   static final FileType APK = FileType.of(".apk");
 
+  public static final String NOCOMPRESS_EXTENSIONS_ATTR = "nocompress_extensions";
+
   /** The default label of android_sdk option */
   public static LateBoundDefault<?, Label> getAndroidSdkLabel(Label androidSdk) {
     return LateBoundDefault.fromTargetConfiguration(

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
@@ -25,9 +25,11 @@
 import com.google.devtools.build.lib.rules.java.JavaCommon;
 import com.google.devtools.build.lib.rules.java.JavaHelper;
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.List;
 
 /**
  * A class for coordinating APK building, signing and zipaligning.
@@ -337,8 +339,18 @@ private void buildApk(RuleContext ruleContext, Artifact outApk) {
       singleJarCommandLine.addExecPath("--sources", inputZip);
     }
 
-    ImmutableList<String> noCompressExtensions =
-        ruleContext.getExpander().withDataLocations().tokenized("nocompress_extensions");
+    List<String> noCompressExtensions;
+    if (ruleContext.getRule().isAttrDefined(
+        AndroidRuleClasses.NOCOMPRESS_EXTENSIONS_ATTR, Type.STRING_LIST)) {
+      noCompressExtensions =
+          ruleContext
+              .getExpander()
+              .withDataLocations()
+              .tokenized(AndroidRuleClasses.NOCOMPRESS_EXTENSIONS_ATTR);
+    } else {
+      // This code is also used by android_test, which doesn't have this attribute.
+      noCompressExtensions = ImmutableList.of();
+    }
     if (!noCompressExtensions.isEmpty()) {
       compressedApkCommandLine.addAll("--nocompress_suffixes", noCompressExtensions);
       singleJarCommandLine.addAll("--nocompress_suffixes", noCompressExtensions);

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
@@ -804,8 +804,18 @@ public ResourceApk packWithResources(
 
     ResourceFilter resourceFilter = ResourceFilter.fromRuleContext(ruleContext);
 
-    List<String> uncompressedExtensions =
-        ruleContext.getExpander().withDataLocations().tokenized("nocompress_extensions");
+    List<String> uncompressedExtensions;
+    if (ruleContext.getRule().isAttrDefined(
+        AndroidRuleClasses.NOCOMPRESS_EXTENSIONS_ATTR, Type.STRING_LIST)) {
+      uncompressedExtensions =
+          ruleContext
+              .getExpander()
+              .withDataLocations()
+              .tokenized(AndroidRuleClasses.NOCOMPRESS_EXTENSIONS_ATTR);
+    } else {
+      // This code is also used by android_test, which doesn't have this attribute.
+      uncompressedExtensions = ImmutableList.of();
+    }
 
     ImmutableList.Builder<String> additionalAaptOpts = ImmutableList.builder();
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import java.util.List;
 import javax.annotation.Nullable;
@@ -511,9 +512,17 @@ public static ImmutableList<String> getDefaultJavacOpts(
     ConfiguredTarget javaToolchainConfigTarget =
         (ConfiguredTarget) skylarkRuleContext.getAttr().getValue(javaToolchainAttr);
     JavaToolchainProvider toolchain = getJavaToolchainProvider(javaToolchainConfigTarget);
+    ImmutableList<String> javacOptsFromAttr;
+    if (ruleContext.getRule().isAttrDefined("javacopts", Type.STRING_LIST)) {
+      javacOptsFromAttr = ruleContext.getExpander().withDataLocations().tokenized("javacopts");
+    } else {
+      // This can also be called from Skylark rules that may or may not have an appropriate
+      // javacopts attribute.
+      javacOptsFromAttr = ImmutableList.of();
+    }
     return ImmutableList.copyOf(Iterables.concat(
         toolchain.getJavacOptions(),
-        ruleContext.getExpander().withDataLocations().tokenized("javacopts")));
+        javacOptsFromAttr));
   }
 
   @SkylarkCallable(