Refactor Skylark rules and attributes in preparation to Skylark aspects.

1. attr.<type> functions return a wrapper object instead of
    Attribute.Builder dierctly.
2. RuleClass is created once per the life-time of RuleFunction, during
    export
3. Attributes are added to the RuleClass at exporting.

--
MOS_MIGRATED_REVID=108774581

3 files changed, 98 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index 5d8775378e..f445eb6699 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -184,11 +184,11 @@ public final class SkylarkAttr {
     return builder;
   }
 
-  private static Attribute.Builder<?> createAttribute(
+  private static Descriptor createAttribute(
       Map<String, Object> kwargs, Type<?> type, FuncallExpression ast, Environment env)
       throws EvalException {
     try {
-      return createAttribute(type, kwargs, ast, env);
+      return new Descriptor(createAttribute(type, kwargs, ast, env));
     } catch (ConversionException e) {
       throw new EvalException(ast.getLocation(), e.getMessage());
     }
@@ -198,7 +198,7 @@ public final class SkylarkAttr {
     name = "int",
     doc = "Creates an attribute of type int.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -221,7 +221,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction integer =
       new BuiltinFunction("int") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Integer defaultInt,
             Boolean mandatory,
             SkylarkList values,
@@ -243,7 +243,7 @@ public final class SkylarkAttr {
     name = "string",
     doc = "Creates an attribute of type string.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -266,7 +266,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction string =
       new BuiltinFunction("string") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             String defaultString,
             Boolean mandatory,
             SkylarkList values,
@@ -291,7 +291,7 @@ public final class SkylarkAttr {
             + "If you need a dependency that the user cannot overwrite, make the attribute "
             + "private (starts with <code>_</code>).",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -349,7 +349,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction label =
       new BuiltinFunction("label") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Object defaultO,
             Boolean executable,
             Object allowFiles,
@@ -390,7 +390,7 @@ public final class SkylarkAttr {
     name = "string_list",
     doc = "Creates an attribute of type list of strings",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalPositionals = {
       @Param(
         name = DEFAULT_ARG,
@@ -409,7 +409,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction stringList =
       new BuiltinFunction("string_list") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             SkylarkList defaultList,
             Boolean mandatory,
             Boolean nonEmpty,
@@ -430,7 +430,7 @@ public final class SkylarkAttr {
     name = "int_list",
     doc = "Creates an attribute of type list of ints",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalPositionals = {
       @Param(
         name = DEFAULT_ARG,
@@ -449,7 +449,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction intList =
       new BuiltinFunction("int_list") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             SkylarkList defaultList,
             Boolean mandatory,
             Boolean nonEmpty,
@@ -472,7 +472,7 @@ public final class SkylarkAttr {
         "Creates an attribute of type list of labels. "
             + "See <a href=\"attr.html#label\">label</a> for more information.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -529,7 +529,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction labelList =
       new BuiltinFunction("label_list") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Object defaultList,
             Object allowFiles,
             Object allowRules,
@@ -570,7 +570,7 @@ public final class SkylarkAttr {
     name = "bool",
     doc = "Creates an attribute of type bool. Its default value is False.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(name = DEFAULT_ARG, type = Boolean.class, defaultValue = "False", doc = DEFAULT_DOC),
       @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC
@@ -581,7 +581,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction bool =
       new BuiltinFunction("bool") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Boolean defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.bool", ast.getLocation());
@@ -600,7 +600,7 @@ public final class SkylarkAttr {
             + "The user provides a file name (string) and the rule must create an action that "
             + "generates the file.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -617,7 +617,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction output =
       new BuiltinFunction("output") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.output", ast.getLocation());
@@ -635,7 +635,7 @@ public final class SkylarkAttr {
         "Creates an attribute of type list of outputs. Its default value is <code>[]</code>. "
             + "See <a href=\"attr.html#output\">output</a> above for more information.",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(
         name = DEFAULT_ARG,
@@ -654,7 +654,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction outputList =
       new BuiltinFunction("output_list") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             SkylarkList defaultList,
             Boolean mandatory,
             Boolean nonEmpty,
@@ -677,7 +677,7 @@ public final class SkylarkAttr {
         "Creates an attribute of type dictionary, mapping from string to string. "
             + "Its default value is dict().",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(name = DEFAULT_ARG, type = Map.class, defaultValue = "{}", doc = DEFAULT_DOC),
       @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC
@@ -690,7 +690,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction stringDict =
       new BuiltinFunction("string_dict") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Map<?, ?> defaultO,
             Boolean mandatory,
             Boolean nonEmpty,
@@ -713,7 +713,7 @@ public final class SkylarkAttr {
         "Creates an attribute of type dictionary, mapping from string to list of string. "
             + "Its default value is dict().",
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       @Param(name = DEFAULT_ARG, type = Map.class, defaultValue = "{}", doc = DEFAULT_DOC),
       @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC
@@ -726,7 +726,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction stringListDict =
       new BuiltinFunction("string_list_dict") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Map<?, ?> defaultO,
             Boolean mandatory,
             Boolean nonEmpty,
@@ -748,7 +748,7 @@ public final class SkylarkAttr {
     doc = "Creates an attribute of type license. Its default value is NO_LICENSE.",
     // TODO(bazel-team): Implement proper license support for Skylark.
     objectType = SkylarkAttr.class,
-    returnType = Attribute.Builder.class,
+    returnType = Descriptor.class,
     optionalNamedOnly = {
       // TODO(bazel-team): ensure this is the correct default value
       @Param(name = DEFAULT_ARG, defaultValue = "None", noneable = true, doc = DEFAULT_DOC),
@@ -760,7 +760,7 @@ public final class SkylarkAttr {
   )
   private static BuiltinFunction license =
       new BuiltinFunction("license") {
-        public Attribute.Builder<?> invoke(
+        public Descriptor invoke(
             Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env)
             throws EvalException {
           env.checkLoadingPhase("attr.license", ast.getLocation());
@@ -772,6 +772,21 @@ public final class SkylarkAttr {
         }
       };
 
+  /**
+   * A descriptor of an attribute defined in Skylark.
+   */
+  public static final class Descriptor {
+    private final Attribute.Builder<?> attributeBuilder;
+
+    public Descriptor(Attribute.Builder<?> attributeBuilder) {
+      this.attributeBuilder = attributeBuilder;
+    }
+
+    public Attribute.Builder<?> getAttributeBuilder() {
+      return attributeBuilder;
+    }
+  }
+
   static {
     SkylarkSignatureProcessor.configureSkylarkFunctions(SkylarkAttr.class);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index ed5892486a..63b6307525 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -84,6 +84,7 @@ import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
 import com.google.devtools.build.lib.syntax.SkylarkValue;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.util.Pair;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -264,18 +265,22 @@ public class SkylarkRuleClassFunctions {
 
       // We'll set the name later, pass the empty string for now.
       RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent);
+      ImmutableList.Builder<Pair<String, SkylarkAttr.Descriptor>> attributes =
+          ImmutableList.builder();
 
       if (attrs != Runtime.NONE) {
-        for (Map.Entry<String, Attribute.Builder> attr :
-            castMap(attrs, String.class, Attribute.Builder.class, "attrs").entrySet()) {
-          Attribute.Builder<?> attrBuilder = (Attribute.Builder<?>) attr.getValue();
+        for (Map.Entry<String, SkylarkAttr.Descriptor> attr :
+            castMap(attrs, String.class, SkylarkAttr.Descriptor.class, "attrs").entrySet()) {
+          SkylarkAttr.Descriptor attrDescriptor = attr.getValue();
           String attrName =
-              attributeToNative(attr.getKey(), ast.getLocation(), attrBuilder.hasLateBoundValue());
-          addAttribute(builder, attrBuilder.build(attrName));
+              attributeToNative(attr.getKey(), ast.getLocation(),
+                  attrDescriptor.getAttributeBuilder().hasLateBoundValue());
+          attributes.add(Pair.of(attrName, attrDescriptor));
         }
       }
       if (executable || test) {
         addAttribute(
+            ast.getLocation(),
             builder,
             attr("$is_executable", BOOLEAN)
                 .value(true)
@@ -307,16 +312,9 @@ public class SkylarkRuleClassFunctions {
       registerRequiredFragments(fragments, hostFragments, builder);
       builder.setConfiguredTargetFunction(implementation);
       builder.setRuleDefinitionEnvironment(funcallEnv);
-      return new RuleFunction(builder, type);
+      return new RuleFunction(builder, type, attributes.build(), ast.getLocation());
     }
 
-    private void addAttribute(RuleClass.Builder builder, Attribute attribute) throws EvalException {
-      try {
-        builder.addOrOverrideAttribute(attribute);
-      } catch (IllegalArgumentException ex) {
-        throw new EvalException(location, ex);
-      }
-    }
 
     private void registerRequiredFragments(
         SkylarkList fragments, SkylarkList hostFragments, RuleClass.Builder builder)
@@ -336,6 +334,16 @@ public class SkylarkRuleClassFunctions {
     }
   };
 
+  private static void addAttribute(
+      Location location, RuleClass.Builder builder, Attribute attribute) throws EvalException {
+    try {
+      builder.addOrOverrideAttribute(attribute);
+    } catch (IllegalArgumentException ex) {
+      throw new EvalException(location, ex);
+    }
+  }
+
+
   @SkylarkSignature(
     name = "aspect",
     returnType = SkylarkAspect.class,
@@ -367,17 +375,22 @@ public class SkylarkRuleClassFunctions {
 
   /** The implementation for the magic function "rule" that creates Skylark rule classes */
   public static final class RuleFunction extends BaseFunction {
-    // Note that this means that we can reuse the same builder.
-    // This is fine since we don't modify the builder from here.
-    private final RuleClass.Builder builder;
+    private RuleClass.Builder builder;
+
+    private RuleClass ruleClass;
     private final RuleClassType type;
+    private ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes;
+    private final Location definitionLocation;
     private Label skylarkLabel;
-    private String ruleClassName;
 
-    public RuleFunction(Builder builder, RuleClassType type) {
+    public RuleFunction(Builder builder, RuleClassType type,
+        ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes,
+        Location definitionLocation) {
       super("rule", FunctionSignature.KWARGS);
       this.builder = builder;
       this.type = type;
+      this.attributes = attributes;
+      this.definitionLocation = definitionLocation;
     }
 
     @Override
@@ -387,15 +400,10 @@ public class SkylarkRuleClassFunctions {
         throws EvalException, InterruptedException, ConversionException {
       env.checkLoadingPhase(getName(), ast.getLocation());
       try {
-        if (ruleClassName == null || skylarkLabel == null) {
+        if (ruleClass == null) {
           throw new EvalException(ast.getLocation(),
               "Invalid rule class hasn't been exported by a Skylark file");
         }
-        if (type == RuleClassType.TEST != TargetUtils.isTestRuleName(ruleClassName)) {
-          throw new EvalException(ast.getLocation(), "Invalid rule class name '" + ruleClassName
-              + "', test rule class names must end with '_test' and other rule classes must not");
-        }
-        RuleClass ruleClass = builder.build(ruleClassName);
         PackageContext pkgContext = (PackageContext) env.lookup(PackageFactory.PKG_CONTEXT);
         return RuleFactory.createAndAddRule(
             pkgContext, ruleClass, (Map<String, Object>) args[0], ast, env);
@@ -407,18 +415,32 @@ public class SkylarkRuleClassFunctions {
     /**
      * Export a RuleFunction from a Skylark file with a given name.
      */
-    void export(Label skylarkLabel, String ruleClassName) {
+    void export(Label skylarkLabel, String ruleClassName) throws EvalException {
+      Preconditions.checkState(ruleClass == null && builder != null);
       this.skylarkLabel = skylarkLabel;
-      this.ruleClassName = ruleClassName;
+      if (type == RuleClassType.TEST != TargetUtils.isTestRuleName(ruleClassName)) {
+        throw new EvalException(definitionLocation, "Invalid rule class name '" + ruleClassName
+            + "', test rule class names must end with '_test' and other rule classes must not");
+      }
+      for (Pair<String, SkylarkAttr.Descriptor> attribute : attributes) {
+        addAttribute(definitionLocation, builder,
+            attribute.getSecond().getAttributeBuilder().build(attribute.getFirst()));
+      }
+      this.ruleClass = builder.build(ruleClassName);
+
+      this.builder = null;
+      this.attributes = null;
     }
 
     @VisibleForTesting
-    public RuleClass.Builder getBuilder() {
-      return builder;
+    public RuleClass getRuleClass() {
+      Preconditions.checkState(ruleClass != null && builder == null);
+      return ruleClass;
     }
   }
 
-  public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel) {
+  public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel)
+      throws EvalException {
     for (String name : env.getGlobals().getDirectVariableNames()) {
       try {
         Object value = env.lookup(name);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 9baa935cd1..5eed6a6ec4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment.Extension;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.LoadStatement;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -376,7 +377,11 @@ public class SkylarkImportLookupFunction implements SkyFunction {
               mutability, eventHandler, ast.getContentHashCode(), importMap)
           .setupOverride("native", packageFactory.getNativeModule());
       ast.exec(extensionEnv, eventHandler);
-      SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel);
+      try {
+        SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel);
+      } catch (EvalException e) {
+        eventHandler.handle(Event.error(e.getLocation(), e.getMessage()));
+      }
 
       Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
       if (eventHandler.hasErrors()) {