Export exportable values as we go instead of at the end of evaluation.

This simplifies implementation of attr.* functions and helps make
declared provider implementation simpler.

--
PiperOrigin-RevId: 148867326
MOS_MIGRATED_REVID=148867326

8 files changed, 275 insertions, 111 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 b28b08e07c..4b8c90da63 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
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
@@ -274,6 +273,18 @@ public final class SkylarkAttr {
             "cfg must be either 'data', 'host', or 'target'.");
       }
     }
+
+    if (containsNonNoneKey(arguments, ASPECTS_ARG)) {
+      Object obj = arguments.get(ASPECTS_ARG);
+      SkylarkType.checkType(obj, SkylarkList.class, ASPECTS_ARG);
+
+      List<SkylarkAspect> aspects =
+          ((SkylarkList<?>) obj).getContents(SkylarkAspect.class, "aspects");
+      for (SkylarkAspect aspect : aspects) {
+        builder.aspect(aspect, ast.getLocation());
+      }
+    }
+
     return builder;
   }
 
@@ -630,12 +641,13 @@ public final class SkylarkAttr {
                     SINGLE_FILE_ARG,
                     singleFile,
                     CONFIGURATION_ARG,
-                    cfg),
+                    cfg,
+                    ASPECTS_ARG,
+                    aspects
+                    ),
                 ast,
                 env);
-            ImmutableList<SkylarkAspect> skylarkAspects =
-                ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
-            return new Descriptor(attribute, skylarkAspects);
+            return new Descriptor(attribute);
           } catch (EvalException e) {
             throw new EvalException(ast.getLocation(), e.getMessage(), e);
           }
@@ -908,13 +920,14 @@ public final class SkylarkAttr {
                   ALLOW_EMPTY_ARG,
                   allowEmpty,
                   CONFIGURATION_ARG,
-                  cfg);
+                  cfg,
+                  ASPECTS_ARG,
+                  aspects
+                  );
           try {
             Attribute.Builder<?> attribute =
                 createAttribute(BuildType.LABEL_LIST, kwargs, ast, env);
-            ImmutableList<SkylarkAspect> skylarkAspects =
-                ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
-            return new Descriptor(attribute, skylarkAspects);
+            return new Descriptor(attribute);
           } catch (EvalException e) {
             throw new EvalException(ast.getLocation(), e.getMessage(), e);
           }
@@ -1060,13 +1073,13 @@ public final class SkylarkAttr {
                   ALLOW_EMPTY_ARG,
                   allowEmpty,
                   CONFIGURATION_ARG,
-                  cfg);
+                  cfg,
+                  ASPECTS_ARG,
+                  aspects);
           try {
             Attribute.Builder<?> attribute =
                 createAttribute(BuildType.LABEL_KEYED_STRING_DICT, kwargs, ast, env);
-            ImmutableList<SkylarkAspect> skylarkAspects =
-                ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
-            return new Descriptor(attribute, skylarkAspects);
+            return new Descriptor(attribute);
           } catch (EvalException e) {
             throw new EvalException(ast.getLocation(), e.getMessage(), e);
           }
@@ -1425,33 +1438,17 @@ public final class SkylarkAttr {
   )
   public static final class Descriptor {
     private final Attribute.Builder<?> attributeBuilder;
-    private final ImmutableList<SkylarkAspect> aspects;
 
     /**
      * This lock guards {@code attributeBuilder} field.
      *
      * {@link Attribute.Builder} class is not thread-safe for concurrent modification.
-     * This class, together with its enclosing {@link SkylarkAttr} class, do not let
-     * anyone else access the {@code attributeBuilder}, however {@link #exportAspects(Location)}
-     * method actually modifies the {@code attributeBuilder}. Therefore all read- and write-accesses
-     * to {@code attributeBuilder} are protected with this lock.
-     *
-     * For example, {@link #hasDefault()} method only reads from {@link #attributeBuilder},
-     * but we have no guarantee that it is safe to do so concurrently with adding aspects
-     * in {@link #exportAspects(Location)}.
      */
     private final Object lock = new Object();
-    boolean exported;
-
-    private Descriptor(Attribute.Builder<?> attributeBuilder) {
-      this(attributeBuilder, ImmutableList.<SkylarkAspect>of());
-    }
 
     private Descriptor(
-        Attribute.Builder<?> attributeBuilder, ImmutableList<SkylarkAspect> aspects) {
+        Attribute.Builder<?> attributeBuilder) {
       this.attributeBuilder = attributeBuilder;
-      this.aspects = aspects;
-      exported = false;
     }
 
     public boolean hasDefault() {
@@ -1471,27 +1468,6 @@ public final class SkylarkAttr {
         return attributeBuilder.build(name);
       }
     }
-
-    public ImmutableList<SkylarkAspect> getAspects() {
-      return aspects;
-    }
-
-    public void exportAspects(Location definitionLocation) throws EvalException {
-      synchronized (lock) {
-        if (exported) {
-          // Only export an attribute definiton once.
-          return;
-        }
-        for (SkylarkAspect skylarkAspect : getAspects()) {
-          if (!skylarkAspect.isExported()) {
-            throw new EvalException(definitionLocation,
-                "All aspects applied to rule dependencies must be top-level values");
-          }
-          attributeBuilder.aspect(skylarkAspect, definitionLocation);
-        }
-        exported = true;
-      }
-    }
   }
 
   static {
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 3cc9decfe6..5a01e6fd88 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
@@ -94,7 +94,6 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.ExecutionException;
 
 /**
@@ -684,7 +683,6 @@ public class SkylarkRuleClassFunctions {
       }
       for (Pair<String, SkylarkAttr.Descriptor> attribute : attributes) {
         SkylarkAttr.Descriptor descriptor = attribute.getSecond();
-        descriptor.exportAspects(definitionLocation);
 
         addAttribute(definitionLocation, builder,
             descriptor.build(attribute.getFirst()));
@@ -720,25 +718,6 @@ public class SkylarkRuleClassFunctions {
           SkylarkAspect.class,
           RuleFunction.class);
 
-  public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel)
-      throws EvalException {
-    Set<String> globalNames = env.getGlobals().getDirectVariableNames();
-
-    for (Class<? extends SkylarkExportable> exportable : EXPORTABLES) {
-      for (String name : globalNames) {
-        Object value = env.lookup(name);
-        if (value == null) {
-          throw new AssertionError(String.format("No such variable: '%s'", name));
-        }
-        if (exportable.isInstance(value)) {
-          if (!exportable.cast(value).isExported()) {
-            exportable.cast(value).export(skylarkLabel, name);
-          }
-        }
-      }
-    }
-  }
-
   @SkylarkSignature(
     name = "Label",
     doc =
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 fb68cb2fe6..08c6cf5395 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
@@ -27,18 +27,21 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
-import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions;
+import com.google.devtools.build.lib.packages.SkylarkExportable;
 import com.google.devtools.build.lib.skyframe.SkylarkImportLookupValue.SkylarkImportLookupKey;
+import com.google.devtools.build.lib.syntax.AssignmentStatement;
 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.syntax.SkylarkImport;
+import com.google.devtools.build.lib.syntax.Statement;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -367,12 +370,7 @@ public class SkylarkImportLookupFunction implements SkyFunction {
               .createSkylarkRuleClassEnvironment(
                   extensionLabel, mutability, eventHandler, ast.getContentHashCode(), importMap)
               .setupOverride("native", packageFactory.getNativeModule(inWorkspace));
-      ast.exec(extensionEnv, eventHandler);
-      try {
-        SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel);
-      } catch (EvalException e) {
-        eventHandler.handle(Event.error(e.getLocation(), e.getMessage()));
-      }
+      execAndExport(ast, extensionLabel, eventHandler, extensionEnv);
 
       Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
       if (eventHandler.hasErrors()) {
@@ -382,6 +380,39 @@ public class SkylarkImportLookupFunction implements SkyFunction {
     }
   }
 
+  public static void execAndExport(BuildFileAST ast, Label extensionLabel,
+      EventHandler eventHandler,
+      com.google.devtools.build.lib.syntax.Environment extensionEnv) throws InterruptedException {
+    ImmutableList<Statement> statements = ast.getStatements();
+    for (Statement statement : statements) {
+      ast.execTopLevelStatement(statement, extensionEnv, eventHandler);
+      possiblyExport(statement, extensionLabel, eventHandler, extensionEnv);
+    }
+  }
+
+  private static void possiblyExport(Statement statement, Label extensionLabel,
+      EventHandler eventHandler,
+      com.google.devtools.build.lib.syntax.Environment extensionEnv) {
+    if (!(statement instanceof AssignmentStatement)) {
+      return;
+    }
+    AssignmentStatement assignmentStatement = (AssignmentStatement) statement;
+    ImmutableSet<String> boundNames = assignmentStatement.getLValue().boundNames();
+    for (String name : boundNames) {
+      Object lookup = extensionEnv.lookup(name);
+      if (lookup instanceof SkylarkExportable) {
+        try {
+          SkylarkExportable exportable = (SkylarkExportable) lookup;
+          if (!exportable.isExported()) {
+            exportable.export(extensionLabel, name);
+          }
+        } catch (EvalException e) {
+          eventHandler.handle(Event.error(e.getLocation(), e.getMessage()));
+        }
+      }
+    }
+  }
+
   @Override
   public String extractTag(SkyKey skyKey) {
     return null;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index a0f6a33367..b1f079a14b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -194,27 +194,52 @@ public class BuildFileAST extends ASTNode {
   public boolean exec(Environment env, EventHandler eventHandler) throws InterruptedException {
     boolean ok = true;
     for (Statement stmt : stmts) {
-      try {
-        stmt.exec(env);
-      } catch (EvalException e) {
+      if (!execTopLevelStatement(stmt, env, eventHandler)) {
         ok = false;
-        // Do not report errors caused by a previous parsing error, as it has already been
-        // reported.
-        if (e.isDueToIncompleteAST()) {
-          continue;
-        }
-        // When the exception is raised from another file, report first the location in the
-        // BUILD file (as it is the most probable cause for the error).
-        Location exnLoc = e.getLocation();
-        Location nodeLoc = stmt.getLocation();
-        eventHandler.handle(Event.error(
-            (exnLoc == null || !nodeLoc.getPath().equals(exnLoc.getPath())) ? nodeLoc : exnLoc,
-            e.getMessage()));
       }
     }
     return ok;
   }
 
+  /**
+   * Executes tol-level statement of this build file in a given Environment.
+   *
+   * <p>If, for any reason, execution of a statement cannot be completed, an {@link EvalException}
+   * is thrown by {@link Statement#exec(Environment)}. This exception is caught here and reported
+   * through reporter. In effect, there is a
+   * "try/except" block around every top level statement. Such exceptions are not ignored, though:
+   * they are visible via the return value. Rules declared in a package containing any error
+   * (including loading-phase semantical errors that cannot be checked here) must also be considered
+   * "in error".
+   *
+   * <p>Note that this method will not affect the value of {@link #containsErrors()}; that refers
+   * only to lexer/parser errors.
+   *
+   * @return true if no error occurred during execution.
+   */
+
+  public boolean execTopLevelStatement(Statement stmt, Environment env,
+      EventHandler eventHandler) throws InterruptedException {
+    try {
+      stmt.exec(env);
+      return true;
+    } catch (EvalException e) {
+      // Do not report errors caused by a previous parsing error, as it has already been
+      // reported.
+      if (e.isDueToIncompleteAST()) {
+        return false;
+      }
+      // When the exception is raised from another file, report first the location in the
+      // BUILD file (as it is the most probable cause for the error).
+      Location exnLoc = e.getLocation();
+      Location nodeLoc = stmt.getLocation();
+      eventHandler.handle(Event.error(
+          (exnLoc == null || !nodeLoc.getPath().equals(exnLoc.getPath())) ? nodeLoc : exnLoc,
+          e.getMessage()));
+      return false;
+    }
+  }
+
   @Override
   public String toString() {
     return "BuildFileAST" + getStatements();
@@ -348,10 +373,20 @@ public class BuildFileAST extends ASTNode {
   @Nullable
   public static Object eval(Environment env, String... input)
       throws EvalException, InterruptedException {
+    BuildFileAST ast = parseAndValidateSkylarkString(env, input);
+    return ast.eval(env);
+  }
+
+  /**
+   * Parses and validates the lines from input and return the the AST
+   * In case of error during validation, it throws an EvalException.
+   */
+  public static BuildFileAST parseAndValidateSkylarkString(Environment env, String[] input)
+      throws EvalException {
     BuildFileAST ast = parseSkylarkString(env.getEventHandler(), input);
     ValidationEnvironment valid = new ValidationEnvironment(env);
     valid.validateAst(ast.getStatements());
-    return ast.eval(env);
+    return ast;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index 22d686bf4e..ecc72073fe 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.io.Serializable;
@@ -73,6 +74,35 @@ public class LValue implements Serializable {
     }
   }
 
+  /**
+   *  Returns all names bound by this LValue.
+   *
+   *  Examples:
+   *  <ul>
+   *  <li><{@code x = ...} binds x.</li>
+   *  <li><{@code x, [y,z] = ..} binds x, y, z.</li>
+   *  <li><{@code x[5] = ..} does not bind any names.</li>
+   *  </ul>
+   */
+  public ImmutableSet<String> boundNames() {
+    ImmutableSet.Builder<String> result = ImmutableSet.builder();
+    collectBoundNames(expr, result);
+    return result.build();
+  }
+
+  private static void collectBoundNames(Expression lhs, ImmutableSet.Builder<String> result) {
+    if (lhs instanceof Identifier) {
+      result.add(((Identifier) lhs).getName());
+      return;
+    }
+    if (lhs instanceof ListLiteral) {
+      ListLiteral variables = (ListLiteral) lhs;
+      for (Expression expression : variables.getElements()) {
+        collectBoundNames(expression, result);
+      }
+    }
+  }
+
   private static void doAssign(
       Environment env, Location loc, Expression lhs, Object result)
       throws EvalException, InterruptedException {
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 37d4cea027..e79e9c1ecf 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
@@ -37,17 +37,18 @@ import com.google.devtools.build.lib.packages.RequiredProviders;
 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.packages.SkylarkAspectClass;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
 import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.rules.SkylarkAttr;
 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.skyframe.SkylarkImportLookupFunction;
 import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.ClassObject;
 import com.google.devtools.build.lib.syntax.Environment;
-import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
@@ -96,36 +97,37 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
 
   @Test
   public void testCannotOverrideBuiltInAttribute() throws Exception {
-    ev.setFailFast(true);
+    ev.setFailFast(false);
     try {
       evalAndExport(
           "def impl(ctx): return", "r = rule(impl, attrs = {'tags': attr.string_list()})");
       Assert.fail("Expected error '"
           + "There is already a built-in attribute 'tags' which cannot be overridden"
           + "' but got no error");
-    } catch (EvalException e) {
-      assertThat(e).hasMessage(
+    } catch (AssertionError e) {
+      assertThat(e).hasMessageThat().contains(
           "There is already a built-in attribute 'tags' which cannot be overridden");
     }
   }
 
   @Test
   public void testCannotOverrideBuiltInAttributeName() throws Exception {
-    ev.setFailFast(true);
+    ev.setFailFast(false);
     try {
       evalAndExport(
           "def impl(ctx): return", "r = rule(impl, attrs = {'name': attr.string()})");
       Assert.fail("Expected error '"
           + "There is already a built-in attribute 'name' which cannot be overridden"
           + "' but got no error");
-    } catch (EvalException e) {
-      assertThat(e).hasMessage(
+    } catch (AssertionError e) {
+      assertThat(e).hasMessageThat().contains(
           "There is already a built-in attribute 'name' which cannot be overridden");
     }
   }
 
   @Test
   public void testImplicitArgsAttribute() throws Exception {
+    ev.setFailFast(false);
     evalAndExport(
         "def _impl(ctx):",
         "  pass",
@@ -263,18 +265,32 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
 
   @Test
   public void testLabelListWithAspects() throws Exception {
-    SkylarkAttr.Descriptor attr =
-        (SkylarkAttr.Descriptor) evalRuleClassCode(
+    evalAndExport(
             "def _impl(target, ctx):",
             "   pass",
             "my_aspect = aspect(implementation = _impl)",
-            "attr.label_list(aspects = [my_aspect])");
-    Object aspect = ev.lookup("my_aspect");
+            "a = attr.label_list(aspects = [my_aspect])");
+    SkylarkAttr.Descriptor attr = (SkylarkAttr.Descriptor) ev.lookup("a");
+            SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
     assertThat(aspect).isNotNull();
-    assertThat(attr.getAspects()).containsExactly(aspect);
+    assertThat(attr.build("xxx").getAspectClasses()).containsExactly(aspect.getAspectClass());
   }
 
   @Test
+  public void testLabelWithAspects() throws Exception {
+    evalAndExport(
+        "def _impl(target, ctx):",
+        "   pass",
+        "my_aspect = aspect(implementation = _impl)",
+        "a = attr.label(aspects = [my_aspect])");
+    SkylarkAttr.Descriptor attr = (SkylarkAttr.Descriptor) ev.lookup("a");
+    SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect");
+    assertThat(aspect).isNotNull();
+    assertThat(attr.build("xxx").getAspectClasses()).containsExactly(aspect.getAspectClass());
+  }
+
+
+  @Test
   public void testLabelListWithAspectsError() throws Exception {
     checkErrorContains(
         "expected type 'Aspect' for 'aspects' element but got type 'int' instead",
@@ -492,8 +508,10 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   }
 
   protected void evalAndExport(String... lines) throws Exception {
-    eval(lines);
-    SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(ev.getEnvironment(), FAKE_LABEL);
+    BuildFileAST buildFileAST = BuildFileAST.parseAndValidateSkylarkString(
+        ev.getEnvironment(), lines);
+    SkylarkImportLookupFunction.execAndExport(
+        buildFileAST, FAKE_LABEL, ev.getEventHandler(), ev.getEnvironment());
   }
 
   @Test
@@ -1221,6 +1239,44 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
     assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isFalse();
   }
 
+  @Test
+  public void fancyExports() throws Exception {
+    evalAndExport(
+        "def _impla(target, ctx): pass",
+        "p, (a, p1) = [",
+        "   provider(),",
+        "   [ aspect(_impla),",
+        "     provider() ]",
+        "]"
+    );
+    SkylarkClassObjectConstructor p = (SkylarkClassObjectConstructor) lookup("p");
+    SkylarkAspect a = (SkylarkAspect) lookup("a");
+    SkylarkClassObjectConstructor p1 = (SkylarkClassObjectConstructor) lookup("p1");
+    assertThat(p.getPrintableName()).isEqualTo("p");
+    assertThat(p.getKey())
+        .isEqualTo(new SkylarkClassObjectConstructor.SkylarkKey(FAKE_LABEL, "p"));
+    assertThat(p1.getPrintableName()).isEqualTo("p1");
+    assertThat(p1.getKey())
+        .isEqualTo(new SkylarkClassObjectConstructor.SkylarkKey(FAKE_LABEL, "p1"));
+    assertThat(a.getAspectClass()).isEqualTo(
+        new SkylarkAspectClass(FAKE_LABEL, "a")
+    );
+  }
+
+  @Test
+  public void multipleTopLevels() throws Exception {
+    evalAndExport(
+        "p = provider()",
+        "p1 = p"
+    );
+    SkylarkClassObjectConstructor p = (SkylarkClassObjectConstructor) lookup("p");
+    SkylarkClassObjectConstructor p1 = (SkylarkClassObjectConstructor) lookup("p1");
+    assertThat(p).isEqualTo(p1);
+    assertThat(p.getKey())
+        .isEqualTo(new SkylarkClassObjectConstructor.SkylarkKey(FAKE_LABEL, "p"));
+    assertThat(p1.getKey())
+        .isEqualTo(new SkylarkClassObjectConstructor.SkylarkKey(FAKE_LABEL, "p"));
+  }
 
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java
new file mode 100644
index 0000000000..69bede701a
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java
@@ -0,0 +1,57 @@
+// Copyright 2006 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.syntax;
+
+import com.google.common.truth.Truth;
+import java.util.Arrays;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** A test for {@link LValue#boundNames()}. */
+@RunWith(JUnit4.class)
+public class LValueBoundNamesTest {
+
+  @Test
+  public void simpleAssignment() {
+    assertBoundNames("x = 1", "x");
+  }
+
+  @Test
+  public void listAssignment() {
+    assertBoundNames("x, y = 1", "x", "y");
+  }
+
+  @Test
+  public void complexListAssignment() {
+    assertBoundNames("x, [y] = 1", "x", "y");
+  }
+
+  @Test
+  public void arrayElementAssignment() {
+    assertBoundNames("x[1] = 1");
+  }
+
+  @Test
+  public void complexListAssignment2() {
+    assertBoundNames("[[x], y], [z, w[1]] = 1", "x", "y", "z");
+  }
+
+  private static void assertBoundNames(String assignement, String... boundNames) {
+    BuildFileAST buildFileAST = BuildFileAST
+        .parseSkylarkString(Environment.FAIL_FAST_HANDLER, assignement);
+    LValue lValue = ((AssignmentStatement) buildFileAST.getStatements().get(0)).getLValue();
+    Truth.assertThat(lValue.boundNames()).containsExactlyElementsIn(Arrays.asList(boundNames));
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index 3a04c67764..d76e2ebc62 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -125,7 +125,7 @@ public class EvaluationTestCase {
     setMode(TestMode.BUILD);
   }
 
-  protected EventHandler getEventHandler() {
+  public EventHandler getEventHandler() {
     return eventCollectionApparatus.reporter();
   }