Expose parameterized aspects to Skylark.

There are no syntactic changes within Skylark; the only difference is that aspects may have non-implicit attributes, so long as they have type 'string' and use the 'values' restriction. Such aspects may only be requested by rules which have attributes with types and names matching the non-implicit, non-defaulted attributes of the aspect.

This is not yet a complete match for native AspectParameters functionality since implicit attributes cannot yet be affected by attribute values, but that will be added later.

Implicit aspects are still required to have default values. Non-implicit aspect attributes are considered "required" unless they have a default value. An error will occur if they are applied to a rule that does not "cover" all required attributes by having attributes of matching name and type. While non-implicit aspect attributes with a default are not required, they will still take on the value of a rule attribute with the same name and type if it is present.

Aspects with non-implicit, non-defaulted ("required") attributes cannot be requested on the command line, only by a rule.

RELNOTES: Expose parameterized aspects to Skylark.

--
MOS_MIGRATED_REVID=121711715

15 files changed, 610 insertions, 157 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index c07d950969..2cced64191 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -560,11 +560,13 @@ public class BuildView {
     // Tests. This must come last, so that the exclusive tests are scheduled after everything else.
     scheduleTestsIfRequested(parallelTests, exclusiveTests, topLevelOptions, allTargetsToTest);
 
-    String error = loadingResult.hasLoadingError() || skyframeAnalysisResult.hasLoadingError()
-          ? "execution phase succeeded, but there were loading phase errors"
-          : skyframeAnalysisResult.hasAnalysisError()
-            ? "execution phase succeeded, but not all targets were analyzed"
-            : null;
+    String error = loadingResult.hasTargetPatternError()
+        ? "execution phase successful, but there were errors parsing the target pattern"
+        : loadingResult.hasLoadingError() || skyframeAnalysisResult.hasLoadingError()
+            ? "execution phase succeeded, but there were loading phase errors"
+            : skyframeAnalysisResult.hasAnalysisError()
+                ? "execution phase succeeded, but not all targets were analyzed"
+                : null;
 
     final WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph();
     final ActionGraph actionGraph = new ActionGraph() {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Aspect.java b/src/main/java/com/google/devtools/build/lib/packages/Aspect.java
index dfd7e5c35d..93f3f4c248 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Aspect.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Aspect.java
@@ -55,7 +55,7 @@ public final class Aspect implements DependencyFilter.AttributeInfoProvider {
       SkylarkAspectClass skylarkAspectClass,
       AspectDefinition definition,
       AspectParameters parameters) {
-    return new Aspect(skylarkAspectClass,  definition, parameters);
+    return new Aspect(skylarkAspectClass, definition, parameters);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index c9b8a3b3a8..4e3f08a777 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -23,6 +23,7 @@ import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Preconditions;
 
 import java.util.Collection;
@@ -289,7 +290,9 @@ public final class AspectDefinition {
      * configuration is available, starting with ':')
      */
     public Builder add(Attribute attribute) {
-      Preconditions.checkArgument(attribute.isImplicit() || attribute.isLateBound());
+      Preconditions.checkArgument(attribute.isImplicit() || attribute.isLateBound()
+          || (attribute.getType() == Type.STRING && attribute.checkAllowedValues()),
+          "Invalid attribute '%s' (%s)", attribute.getName(), attribute.getType());
       Preconditions.checkArgument(!attributes.containsKey(attribute.getName()),
           "An attribute with the name '%s' already exists.", attribute.getName());
       attributes.put(attribute.getName(), attribute);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java b/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java
index 0cf175e130..6862f106ce 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java
@@ -58,7 +58,7 @@ public final class AspectParameters {
   }
 
   /**
-   * Returns collection of values for specified key, or null if key is missing.
+   * Returns collection of values for specified key, or an empty collection if key is missing.
    */
   public ImmutableCollection<String> getAttribute(String key) {
     return attributes.get(key);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 501b46cb05..2b8a738806 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -19,6 +19,7 @@ import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
@@ -61,6 +62,9 @@ public final class Attribute implements Comparable<Attribute> {
 
   public static final Predicate<RuleClass> NO_RULE = Predicates.alwaysFalse();
 
+  /**
+   * Wraps the information necessary to construct an Aspect.
+   */
   private abstract static class RuleAspect<C extends AspectClass> {
     protected final C aspectClass;
     protected final Function<Rule, AspectParameters> parametersExtractor;
@@ -70,6 +74,14 @@ public final class Attribute implements Comparable<Attribute> {
       this.parametersExtractor = parametersExtractor;
     }
 
+    public String getName() {
+      return this.aspectClass.getName();
+    }
+
+    public ImmutableSet<String> getRequiredParameters() {
+      return ImmutableSet.<String>of();
+    }
+
     public abstract Aspect getAspect(Rule rule);
   }
 
@@ -86,19 +98,40 @@ public final class Attribute implements Comparable<Attribute> {
   }
 
   private static class SkylarkRuleAspect extends RuleAspect<SkylarkAspectClass> {
-    private final AspectDefinition definition;
+    private final SkylarkAspect aspect;
+
+    public SkylarkRuleAspect(SkylarkAspect aspect) {
+      super(aspect.getAspectClass(), aspect.getDefaultParametersExtractor());
+      this.aspect = aspect;
+    }
+
+    @Override
+    public ImmutableSet<String> getRequiredParameters() {
+      return aspect.getParamAttributes();
+    }
+
+    @Override
+    public Aspect getAspect(Rule rule) {
+      AspectParameters parameters = parametersExtractor.apply(rule);
+      return Aspect.forSkylark(aspectClass, aspect.getDefinition(parameters), parameters);
+    }
+  }
 
-    public SkylarkRuleAspect(SkylarkAspectClass aspectClass, AspectDefinition definition) {
-      super(aspectClass, NO_PARAMETERS);
-      this.definition = definition;
+  /**
+   * A RuleAspect that just wraps a pre-existing Aspect that doesn't vary with the Rule.
+   * For instance, this may come from a DeserializedSkylarkAspect.
+   */
+  private static class PredefinedRuleAspect extends RuleAspect<AspectClass> {
+    private Aspect aspect;
+
+    public PredefinedRuleAspect(Aspect aspect) {
+      super(aspect.getAspectClass(), null);
+      this.aspect = aspect;
     }
 
     @Override
     public Aspect getAspect(Rule rule) {
-      return Aspect.forSkylark(
-          aspectClass,
-          definition,
-          parametersExtractor.apply(rule));
+      return aspect;
     }
   }
 
@@ -351,14 +384,13 @@ public final class Attribute implements Comparable<Attribute> {
     }
   }
 
-  private static final Function<Rule, AspectParameters> NO_PARAMETERS =
-      new Function<Rule, AspectParameters>() {
-        @Override
-        public AspectParameters apply(Rule input) {
-          return AspectParameters.EMPTY;
-        }
-      };
-
+  public ImmutableMap<String, ImmutableSet<String>> getRequiredAspectParameters() {
+    ImmutableMap.Builder<String, ImmutableSet<String>> paramBuilder = ImmutableMap.builder();
+    for (RuleAspect<?> aspect : aspects) {
+      paramBuilder.put(aspect.getName(), aspect.getRequiredParameters());
+    }
+    return paramBuilder.build();
+  }
 
   /**
    * Creates a new attribute builder.
@@ -871,8 +903,13 @@ public final class Attribute implements Comparable<Attribute> {
       return this.aspect(aspect, noParameters);
     }
 
-    public Builder<TYPE> aspect(SkylarkAspectClass aspectClass, AspectDefinition definition) {
-      this.aspects.add(new SkylarkRuleAspect(aspectClass, definition));
+    public Builder<TYPE> aspect(SkylarkAspect skylarkAspect) {
+      this.aspects.add(new SkylarkRuleAspect(skylarkAspect));
+      return this;
+    }
+
+    public Builder<TYPE> aspect(final Aspect aspect) {
+      this.aspects.add(new PredefinedRuleAspect(aspect));
       return this;
     }
 
@@ -1637,8 +1674,9 @@ public final class Attribute implements Comparable<Attribute> {
   /**
    * Returns a replica builder of this Attribute.
    */
-  public Attribute.Builder<?> cloneBuilder() {
-    Builder<?> builder = new Builder<>(name, this.type);
+  public <TYPE> Attribute.Builder<TYPE> cloneBuilder(Type<TYPE> tp) {
+    Preconditions.checkArgument(tp == this.type);
+    Builder<TYPE> builder = new Builder<TYPE>(name, tp);
     builder.allowedFileTypesForLabels = allowedFileTypesForLabels;
     builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels;
     builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning;
@@ -1655,4 +1693,8 @@ public final class Attribute implements Comparable<Attribute> {
 
     return builder;
   }
+
+  public Attribute.Builder<?> cloneBuilder() {
+    return cloneBuilder(this.type);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 7fd42dcca3..b4aac3be6e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1343,6 +1343,7 @@ public final class RuleClass {
       throws LabelSyntaxException, InterruptedException {
     Rule rule = pkgBuilder.createRule(ruleLabel, this, location, attributeContainer);
     populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler);
+    checkAspectAllowedValues(rule, eventHandler);
     rule.populateOutputFiles(eventHandler, pkgBuilder);
     if (ast != null) {
       populateAttributeLocations(rule, ast);
@@ -1741,6 +1742,30 @@ public final class RuleClass {
     }
   }
 
+  private static void checkAspectAllowedValues(
+      Rule rule, EventHandler eventHandler) {
+    for (Attribute attrOfRule : rule.getAttributes()) {
+      for (Aspect aspect : attrOfRule.getAspects(rule)) {
+        for (Attribute attrOfAspect : aspect.getDefinition().getAttributes().values()) {
+          // By this point the AspectDefinition has been created and values assigned.
+          if (attrOfAspect.checkAllowedValues()) {
+            PredicateWithMessage<Object> allowedValues = attrOfAspect.getAllowedValues();
+            Object value = attrOfAspect.getDefaultValue(rule);
+            if (!allowedValues.apply(value)) {
+              rule.reportError(
+                  String.format(
+                      "%s: invalid value in '%s' attribute: %s",
+                      rule.getLabel(),
+                      attrOfAspect.getName(),
+                      allowedValues.getErrorReason(value)),
+                  eventHandler);
+            }
+          }
+        }
+      }
+    }
+  }
+
   @Override
   public String toString() {
     return name;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java
new file mode 100644
index 0000000000..d679273a56
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java
@@ -0,0 +1,172 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.packages;
+
+import com.google.common.base.Function;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
+import com.google.devtools.build.lib.syntax.BaseFunction;
+import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.Printer;
+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 javax.annotation.Nullable;
+
+/**
+ * A Skylark value that is a result of an 'aspect(..)' function call.
+ */
+@SkylarkModule(name = "aspect", doc = "", documented = false)
+public class SkylarkAspect implements SkylarkValue {
+  private final BaseFunction implementation;
+  private final ImmutableList<String> attributeAspects;
+  private final ImmutableList<Attribute> attributes;
+  private final ImmutableSet<String> paramAttributes;
+  private final ImmutableSet<String> fragments;
+  private final ImmutableSet<String> hostFragments;
+  private final Environment funcallEnv;
+  private SkylarkAspectClass aspectClass;
+
+  public SkylarkAspect(
+      BaseFunction implementation,
+      ImmutableList<String> attributeAspects,
+      ImmutableList<Attribute> attributes,
+      ImmutableSet<String> paramAttributes,
+      ImmutableSet<String> fragments,
+      ImmutableSet<String> hostFragments,
+      Environment funcallEnv) {
+    this.implementation = implementation;
+    this.attributeAspects = attributeAspects;
+    this.attributes = attributes;
+    this.paramAttributes = paramAttributes;
+    this.fragments = fragments;
+    this.hostFragments = hostFragments;
+    this.funcallEnv = funcallEnv;
+    ImmutableList.Builder<Pair<String, Attribute>> builder = ImmutableList.builder();
+  }
+
+  public BaseFunction getImplementation() {
+    return implementation;
+  }
+
+  public ImmutableList<String> getAttributeAspects() {
+    return attributeAspects;
+  }
+
+  public Environment getFuncallEnv() {
+    return funcallEnv;
+  }
+
+  public ImmutableList<Attribute> getAttributes() {
+    return attributes;
+  }
+
+  @Override
+  public boolean isImmutable() {
+    return implementation.isImmutable();
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.append(buffer, "Aspect:");
+    implementation.write(buffer, quotationMark);
+  }
+
+  public String getName() {
+    return getAspectClass().getName();
+  }
+
+  public SkylarkAspectClass getAspectClass() {
+    Preconditions.checkState(isExported());
+    return aspectClass;
+  }
+
+  public ImmutableSet<String> getParamAttributes() {
+    return paramAttributes;
+  }
+
+  public void export(Label extensionLabel, String name) {
+    Preconditions.checkArgument(!isExported());
+    this.aspectClass = new SkylarkAspectClass(extensionLabel, name);
+  }
+
+  public AspectDefinition getDefinition(AspectParameters aspectParams) {
+    AspectDefinition.Builder builder = new AspectDefinition.Builder(getName());
+    for (String attributeAspect : attributeAspects) {
+      builder.attributeAspect(attributeAspect, aspectClass);
+    }
+    for (Attribute attribute : attributes) {
+      Attribute attr = attribute;  // Might be reassigned.
+      if (!aspectParams.getAttribute(attr.getName()).isEmpty()) {
+        String value = aspectParams.getOnlyValueOfAttribute(attr.getName());
+        Preconditions.checkState(!Attribute.isImplicit(attr.getName()));
+        Preconditions.checkState(attr.getType() == Type.STRING);
+        Preconditions.checkArgument(aspectParams.getAttribute(attr.getName()).size() == 1,
+            String.format("Aspect %s parameter %s has %d values (must have exactly 1).",
+                          getName(),
+                          attr.getName(),
+                          aspectParams.getAttribute(attr.getName()).size()));
+        attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName());
+      }
+      builder.add(attr);
+    }
+    builder.requiresConfigurationFragmentsBySkylarkModuleName(fragments);
+    builder.requiresHostConfigurationFragmentsBySkylarkModuleName(hostFragments);
+    return builder.build();
+  }
+
+  public boolean isExported() {
+    return aspectClass != null;
+  }
+
+  public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
+    return new Function<Rule, AspectParameters>() {
+      @Nullable
+      @Override
+      public AspectParameters apply(Rule rule) {
+        AttributeMap ruleAttrs = RawAttributeMapper.of(rule);
+        AspectParameters.Builder builder = new AspectParameters.Builder();
+        for (Attribute aspectAttr : attributes) {
+          if (!Attribute.isImplicit(aspectAttr.getName())) {
+            String param = aspectAttr.getName();
+            Attribute ruleAttr = ruleAttrs.getAttributeDefinition(param);
+            if (paramAttributes.contains(aspectAttr.getName())) {
+              // These are preconditions because if they are false, RuleFunction.call() should
+              // already have generated an error.
+              Preconditions.checkArgument(ruleAttr != null,
+                  String.format("Cannot apply aspect %s to %s that does not define attribute '%s'.",
+                                getName(),
+                                rule.getTargetKind(),
+                                param));
+              Preconditions.checkArgument(ruleAttr.getType() == Type.STRING,
+                  String.format("Cannot apply aspect %s to %s with non-string attribute '%s'.",
+                                getName(),
+                                rule.getTargetKind(),
+                                param));
+            }
+            if (ruleAttr != null && ruleAttr.getType() == aspectAttr.getType()) {
+              builder.addAttribute(param, (String) ruleAttrs.get(param, ruleAttr.getType()));
+            }
+          }
+        }
+        return builder.build();
+      }
+    };
+  }
+}
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 8561dfd80e..85ab569f3e 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
@@ -22,7 +22,7 @@ import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.SkylarkLateBound;
 import com.google.devtools.build.lib.packages.BuildType;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param;
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 c8c0c86494..706a86ab87 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
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.AspectDefinition;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
@@ -57,6 +56,7 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImp
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
+import com.google.devtools.build.lib.packages.PredicateWithMessage;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
@@ -64,14 +64,12 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.packages.RuleFactory;
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
-import com.google.devtools.build.lib.packages.SkylarkAspectClass;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.TestSize;
 import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.ClassObject;
@@ -82,7 +80,6 @@ import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
-import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
@@ -415,9 +412,12 @@ public class SkylarkRuleClassFunctions {
         + "It maps from an attribute name to an attribute object "
         + "(see <a href=\"attr.html\">attr</a> module). "
         + "Aspect attributes are available to implementation function as fields of ctx parameter. "
-        + "All aspect attributes must be private, so their names must start with <code>_</code>. "
-        + "All aspect attributes must be have default values, and be of type "
-        + "<code>label</code> or <code>label_list</code>"
+        + "Implicit attributes starting with <code>_</code> must have default values, and have "
+        + "type <code>label</code> or <code>label_list</code>."
+        + "Explicit attributes must have type <code>string</code>, and must use the "
+        + "<code>values</code> restriction. If explicit attributes are present, the aspect can "
+        + "only be used with rules that have attributes of the same name and type, with valid "
+        + "values. "
       ),
       @Param(
         name = "fragments",
@@ -465,31 +465,55 @@ public class SkylarkRuleClassFunctions {
             }
           }
 
-          ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes =
+          ImmutableList<Pair<String, SkylarkAttr.Descriptor>> descriptors =
               attrObjectToAttributesList(attrs, ast);
-          for (Pair<String, Descriptor> attribute : attributes) {
-            String nativeName = attribute.getFirst();
-            if (!Attribute.isImplicit(nativeName)) {
-              throw new EvalException(
-                  ast.getLocation(),
-                  String.format(
-                      "Aspect attribute '%s' must be implicit (its name should start with '_').",
-                      nativeName));
+          ImmutableList.Builder<Attribute> attributes = ImmutableList.builder();
+          ImmutableSet.Builder<String> requiredParams = ImmutableSet.<String>builder();
+          for (Pair<String, Descriptor> descriptor : descriptors) {
+            String nativeName = descriptor.getFirst();
+            boolean hasDefault = descriptor.getSecond().getAttributeBuilder().isValueSet();
+            Attribute attribute = descriptor.second.getAttributeBuilder().build(descriptor.first);
+            if (attribute.getType() == Type.STRING
+                && ((String) attribute.getDefaultValue(null)).isEmpty()) {
+              hasDefault = false;  // isValueSet() is always true for attr.string.
             }
-
-            String skylarkName = "_" + nativeName.substring(1);
-
-            if (!attribute.getSecond().getAttributeBuilder().isValueSet()) {
+            if (!Attribute.isImplicit(nativeName)) {
+              if (!attribute.checkAllowedValues() || attribute.getType() != Type.STRING) {
+                throw new EvalException(
+                    ast.getLocation(),
+                    String.format(
+                        "Aspect parameter attribute '%s' must have type 'string' and use the "
+                        + "'values' restriction.",
+                        nativeName));
+              }
+              if (!hasDefault) {
+                requiredParams.add(nativeName);
+              } else {
+                PredicateWithMessage<Object> allowed = attribute.getAllowedValues();
+                Object defaultVal = attribute.getDefaultValue(null);
+                if (!allowed.apply(defaultVal)) {
+                  throw new EvalException(
+                      ast.getLocation(),
+                      String.format(
+                          "Aspect parameter attribute '%s' has a bad default value: %s",
+                          nativeName,
+                          allowed.getErrorReason(defaultVal)));
+                }
+              }
+            } else if (!hasDefault) {  // Implicit attribute
+              String skylarkName = "_" + nativeName.substring(1);
               throw new EvalException(
                   ast.getLocation(),
                   String.format("Aspect attribute '%s' has no default value.", skylarkName));
             }
+            attributes.add(attribute);
           }
 
           return new SkylarkAspect(
               implementation,
               attrAspects.build(),
-              attributes,
+              attributes.build(),
+              requiredParams.build(),
               ImmutableSet.copyOf(fragments.getContents(String.class, "fragments")),
               ImmutableSet.copyOf(hostFragments.getContents(String.class, "host_fragments")),
               funcallEnv);
@@ -528,6 +552,24 @@ public class SkylarkRuleClassFunctions {
           throw new EvalException(ast.getLocation(),
               "Invalid rule class hasn't been exported by a Skylark file");
         }
+
+        for (Attribute attribute : ruleClass.getAttributes()) {
+          // TODO(dslomov): If a Skylark parameter extractor is specified for this aspect, its
+          // attributes may not be required.
+          for (Map.Entry<String, ImmutableSet<String>> attrRequirements :
+               attribute.getRequiredAspectParameters().entrySet()) {
+            for (String required : attrRequirements.getValue()) {
+              if (!ruleClass.hasAttr(required, Type.STRING)) {
+                throw new EvalException(definitionLocation, String.format(
+                    "Aspect %s requires rule %s to specify attribute '%s' with type string.",
+                    attrRequirements.getKey(),
+                    ruleClass.getName(),
+                    required));
+              }
+            }
+          }
+        }
+
         PackageContext pkgContext = (PackageContext) env.lookup(PackageFactory.PKG_CONTEXT);
         BuildLangTypedAttributeValuesMap attributeValues =
             new BuildLangTypedAttributeValuesMap((Map<String, Object>) args[0]);
@@ -555,7 +597,7 @@ public class SkylarkRuleClassFunctions {
             throw new EvalException(definitionLocation,
                 "All aspects applied to rule dependencies must be top-level values");
           }
-          attributeBuilder.aspect(skylarkAspect.getAspectClass(), skylarkAspect.getDefinition());
+          attributeBuilder.aspect(skylarkAspect);
         }
 
         addAttribute(definitionLocation, builder,
@@ -848,91 +890,4 @@ public class SkylarkRuleClassFunctions {
   static {
     SkylarkSignatureProcessor.configureSkylarkFunctions(SkylarkRuleClassFunctions.class);
   }
-
-  /**
-   * A Skylark value that is a result of 'aspect(..)' function call.
-   */
-  @SkylarkModule(name = "aspect", doc = "", documented = false)
-  public static final class SkylarkAspect implements SkylarkValue {
-    private final BaseFunction implementation;
-    private final ImmutableList<String> attributeAspects;
-    private final ImmutableList<Pair<String, Descriptor>> attributes;
-    private final ImmutableSet<String> fragments;
-    private final ImmutableSet<String> hostFragments;
-    private final Environment funcallEnv;
-    private SkylarkAspectClass aspectClass;
-
-    public SkylarkAspect(
-        BaseFunction implementation,
-        ImmutableList<String> attributeAspects,
-        ImmutableList<Pair<String, Descriptor>> attributes,
-        ImmutableSet<String> fragments,
-        ImmutableSet<String> hostFragments,
-        Environment funcallEnv) {
-      this.implementation = implementation;
-      this.attributeAspects = attributeAspects;
-      this.attributes = attributes;
-      this.fragments = fragments;
-      this.hostFragments = hostFragments;
-      this.funcallEnv = funcallEnv;
-    }
-
-    public BaseFunction getImplementation() {
-      return implementation;
-    }
-
-    public ImmutableList<String> getAttributeAspects() {
-      return attributeAspects;
-    }
-
-    public Environment getFuncallEnv() {
-      return funcallEnv;
-    }
-
-    public ImmutableList<Pair<String, Descriptor>> getAttributes() {
-      return attributes;
-    }
-
-    @Override
-    public boolean isImmutable() {
-      return implementation.isImmutable();
-    }
-
-    @Override
-    public void write(Appendable buffer, char quotationMark) {
-      Printer.append(buffer, "Aspect:");
-      implementation.write(buffer, quotationMark);
-    }
-
-    public String getName() {
-      return getAspectClass().getName();
-    }
-
-    public SkylarkAspectClass getAspectClass() {
-      Preconditions.checkState(isExported());
-      return aspectClass;
-    }
-
-    void export(Label extensionLabel, String name) {
-      Preconditions.checkArgument(!isExported());
-      this.aspectClass = new SkylarkAspectClass(extensionLabel, name);
-    }
-
-    public boolean isExported() {
-      return aspectClass != null;
-    }
-
-    public AspectDefinition getDefinition() {
-      AspectDefinition.Builder builder = new AspectDefinition.Builder(getName());
-      for (String attributeAspect : attributeAspects) {
-        builder.attributeAspect(attributeAspect, aspectClass);
-      }
-      for (Pair<String, Descriptor> attribute : attributes) {
-        builder.add(attribute.second.getAttributeBuilder().build(attribute.first));
-      }
-      builder.requiresConfigurationFragmentsBySkylarkModuleName(fragments);
-      builder.requiresHostConfigurationFragmentsBySkylarkModuleName(hostFragments);
-      return builder.build();
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index d201d880cd..9bf225f1de 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -41,10 +41,10 @@ import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.packages.SkylarkAspectClass;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.DependencyEvaluationException;
@@ -144,7 +144,7 @@ public final class AspectFunction implements SkyFunction {
       aspectFactory = new SkylarkAspectFactory(skylarkAspect);
       aspect = Aspect.forSkylark(
           skylarkAspect.getAspectClass(),
-          skylarkAspect.getDefinition(),
+          skylarkAspect.getDefinition(key.getParameters()),
           key.getParameters());
     } else {
       throw new IllegalStateException();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index d82a9026cc..b0dfa3c3e6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -178,9 +178,12 @@ final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator {
       }
     }
 
-    if (!keepGoing && result.hasError()) {
+    if (result.hasError()) {
       Preconditions.checkState(errorMessage != null, "unexpected errors: %s", result.errorMap());
-      throw new TargetParsingException(errorMessage);
+      finalTargetSetEvaluator.setError();
+      if (!keepGoing) {
+        throw new TargetParsingException(errorMessage);
+      }
     }
     WalkableGraph walkableGraph = Preconditions.checkNotNull(result.getWalkableGraph(), result);
     return finalTargetSetEvaluator.build(walkableGraph);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 5500bc8b99..33b544bb34 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -23,7 +23,7 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.SkylarkProviderValidationUtil;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.AspectParameters;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.rules.SkylarkRuleContext;
 import com.google.devtools.build.lib.rules.SkylarkRuleContext.Kind;
 import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index ed93b8e659..785e7407f4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -19,7 +19,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.AspectParameters;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
 import com.google.devtools.build.lib.skyframe.AspectValue.SkylarkAspectLoadingKey;
 import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
@@ -66,12 +66,16 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction {
     try {
       skylarkAspect = AspectFunction.loadSkylarkAspect(
           env, labelLookupMap.get(extensionFile), skylarkValueName);
+      if (skylarkAspect == null) {
+        return null;
+      }
+      if (!skylarkAspect.getParamAttributes().isEmpty()) {
+        throw new AspectCreationException("Cannot instantiate parameterized aspect "
+            + skylarkAspect.getName() + " at the top level.", labelLookupMap.get(extensionFile));
+      }
     } catch (AspectCreationException e) {
       throw new LoadSkylarkAspectFunctionException(e);
     }
-    if (skylarkAspect == null) {
-      return null;
-    }
     SkyKey aspectKey =
         AspectValue.key(
             aspectLoadingKey.getTargetLabel(),
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 4d0514873f..35514449e1 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -532,6 +532,215 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void testAspectParametersUncovered() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectUncovered = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectUncovered]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent(//"ERROR /workspace/test/aspect.bzl:9:11: "
+        "Aspect //test:aspect.bzl%MyAspectUncovered requires rule my_rule to specify attribute "
+        + "'my_attr' with type string.");
+  }
+
+  @Test
+  public void testAspectParametersTypeMismatch() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectMismatch = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectMismatch]),",
+        "              'my_attr' : attr.int() },",
+       ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 4)");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent(
+        "Aspect //test:aspect.bzl%MyAspectMismatch requires rule my_rule to specify attribute "
+        + "'my_attr' with type string.");
+  }
+
+  @Test
+  public void testAspectParametersBadDefault() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectBadDefault = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a'], default='b') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadDefault]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/aspect.bzl:5:22: "
+        + "Aspect parameter attribute 'my_attr' has a bad default value: has to be one of 'a' "
+        + "instead of 'b'");
+  }
+
+  @Test
+  public void testAspectParametersBadValue() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectBadValue = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadValue]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr='b')");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(result.hasError()).isTrue();
+    } catch (Exception e) {
+      // expect to fail.
+    }
+    assertContainsEvent("ERROR /workspace/test/BUILD:2:1: //test:xxx: invalid value in 'my_attr' "
+        + "attribute: has to be one of 'a' instead of 'b'");
+  }
+
+  @Test
+  public void testAspectParameters() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspect = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa']) },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspect]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 'aaa')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
+  public void testAspectParametersOptional() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectOptParam = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['aaa'], default='aaa') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptParam]) },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
+  public void testAspectParametersOptionalOverride() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _impl(target, ctx):",
+        "   if (ctx.attr.my_attr == 'a'):",
+        "       fail('Rule is not overriding default, still has value ' + ctx.attr.my_attr)",
+        "   return struct()",
+        "def _rule_impl(ctx):",
+        "   return struct()",
+        "MyAspectOptOverride = aspect(",
+        "    implementation=_impl,",
+        "    attrs = { 'my_attr' : attr.string(values=['a', 'b'], default='a') },",
+        ")",
+        "my_rule = rule(",
+        "    implementation=_rule_impl,",
+        "    attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptOverride]),",
+        "              'my_attr' : attr.string() },",
+        ")");
+    scratch.file("test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "my_rule(name = 'xxx', my_attr = 'b')");
+
+    AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx");
+    assertThat(result.hasError()).isFalse();
+  }
+
+  @Test
   public void multipleExecutablesInTarget() throws Exception {
     scratch.file("foo/extension.bzl",
         "def _aspect_impl(target, ctx):",
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 81636b1aeb..50bd25ac56 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -32,17 +32,15 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.PredicateWithMessage;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
+import com.google.devtools.build.lib.packages.SkylarkAspect;
 import com.google.devtools.build.lib.rules.SkylarkAttr;
-import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor;
 import com.google.devtools.build.lib.rules.SkylarkFileType;
 import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions;
 import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.RuleFunction;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
 import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
-import com.google.devtools.build.lib.util.Pair;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -237,24 +235,64 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
         "   attrs = { '_extra_deps' : attr.label(default = Label('//foo/bar:baz')) }",
         ")");
     SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
-    Pair<String, Descriptor> pair = Iterables.getOnlyElement(aspect.getAttributes());
-    assertThat(pair.first).isEqualTo("$extra_deps");
-    assertThat(pair.second.getAttributeBuilder().build("$extra_deps").getDefaultValue(null))
+    Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+    assertThat(attribute.getName()).isEqualTo("$extra_deps");
+    assertThat(attribute.getDefaultValue(null))
         .isEqualTo(Label.parseAbsolute("//foo/bar:baz", false));
   }
 
   @Test
-  public void testAspectNonImplicitAttribute() throws Exception {
+  public void testAspectParameter() throws Exception {
+    evalAndExport(
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.string(values=['a', 'b']) }",
+        ")");
+    SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
+    Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes());
+    assertThat(attribute.getName()).isEqualTo("param");
+  }
+  
+  @Test
+  public void testAspectParameterRequiresValues() throws Exception {
     checkErrorContains(
-        "Aspect attribute 'extra_deps' must be implicit (its name should start with '_')",
+        "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
+        + "restriction.",
         "def _impl(target, ctx):",
         "   pass",
         "my_aspect = aspect(_impl,",
-        "   attrs = { 'extra_deps' : attr.label(default = Label('//foo/bar:baz')) }",
+        "   attrs = { 'param' : attr.string(default = 'c') }",
         ")");
   }
 
   @Test
+  public void testAspectParameterBadType() throws Exception {
+    checkErrorContains(
+        "Aspect parameter attribute 'param' must have type 'string' and use the 'values' "
+        + "restriction.",
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.label(default = Label('//foo/bar:baz')) }",
+        ")");
+  }
+
+  @Test
+  public void testAspectParameterAndExtraDeps() throws Exception {
+    evalAndExport(
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(_impl,",
+        "   attrs = { 'param' : attr.string(values=['a', 'b']),",
+        "             '_extra' : attr.label(default = Label('//foo/bar:baz')) }",
+        ")");
+    SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
+    assertThat(aspect.getAttributes()).hasSize(2);
+    assertThat(aspect.getParamAttributes()).containsExactly("param");
+  }
+
+  @Test
   public void testAspectNoDefaultValueAttribute() throws Exception {
     checkErrorContains(
         "Aspect attribute '_extra_deps' has no default value",