Better error messages for non-exported values.

RELNOTES: None.
PiperOrigin-RevId: 152793682

2 files changed, 87 insertions, 2 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 3f2b066575..2bbfc00681 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
@@ -160,7 +160,7 @@ public final class SkylarkAttr {
 
   private static Attribute.Builder<?> createAttribute(
       Type<?> type, SkylarkDict<String, Object> arguments, FuncallExpression ast, Environment env)
-      throws EvalException, ConversionException {
+      throws EvalException {
     // We use an empty name now so that we can set it later.
     // This trick makes sense only in the context of Skylark (builtin rules should not use it).
     Attribute.Builder<?> builder = Attribute.attr("", type);
@@ -283,6 +283,11 @@ public final class SkylarkAttr {
       List<SkylarkAspect> aspects =
           ((SkylarkList<?>) obj).getContents(SkylarkAspect.class, "aspects");
       for (SkylarkAspect aspect : aspects) {
+        if (!aspect.isExported()) {
+          throw new EvalException(
+              ast.getLocation(),
+              "Aspects should be top-level values in extension files that define them.");
+        }
         builder.aspect(aspect, ast.getLocation());
       }
     }
@@ -339,7 +344,7 @@ public final class SkylarkAttr {
         ClassObjectConstructor constructor = (ClassObjectConstructor) obj;
         if (!constructor.isExported()) {
           throw new EvalException(location,
-              "Providers should be assigned to top-level values in modules");
+              "Providers should be top-level values in extension files that define them.");
         }
         result.add(SkylarkProviderIdentifier.forKey(constructor.getKey()));
       }
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 004443b0b5..76c80b6cf9 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
@@ -534,6 +534,86 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void aspectsNonExported() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _aspect_impl(target, ctx):",
+        "   return []",
+        "",
+        "def _rule_impl(ctx):",
+        "   pass",
+        "",
+        "def mk_aspect():",
+        "   return aspect(implementation=_aspect_impl)",
+        "my_rule = rule(",
+        "   implementation=_rule_impl,",
+        "   attrs = { 'attr' : attr.label_list(aspects = [mk_aspect()]) },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:aspect.bzl', 'my_rule')",
+        "java_library(",
+        "     name = 'yyy',",
+        ")",
+        "my_rule(",
+        "     name = 'xxx',",
+        "     attr = [':yyy'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult analysisResult = update("//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(analysisResult.hasError()).isTrue();
+    } catch (ViewCreationFailedException | TargetParsingException e) {
+      // expected
+    }
+
+    assertContainsEvent("ERROR /workspace/test/aspect.bzl:11:23");
+    assertContainsEvent("Aspects should be top-level values in extension files that define them.");
+  }
+
+  @Test
+  public void providerNonExported() throws Exception {
+    scratch.file(
+        "test/rule.bzl",
+        "def mk_provider():",
+        "   return provider()",
+        "def _rule_impl(ctx):",
+        "   pass",
+        "my_rule = rule(",
+        "   implementation=_rule_impl,",
+        "   attrs = { 'attr' : attr.label_list(providers = [mk_provider()]) },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:rule.bzl', 'my_rule')",
+        "java_library(",
+        "     name = 'yyy',",
+        ")",
+        "my_rule(",
+        "     name = 'xxx',",
+        "     attr = [':yyy'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    try {
+      AnalysisResult analysisResult = update("//test:xxx");
+      assertThat(keepGoing()).isTrue();
+      assertThat(analysisResult.hasError()).isTrue();
+    } catch (ViewCreationFailedException | TargetParsingException e) {
+      // expected
+    }
+
+    assertContainsEvent("ERROR /workspace/test/rule.bzl:7:23");
+    assertContainsEvent(
+        "Providers should be top-level values in extension files that define them.");
+  }
+
+
+  @Test
   public void aspectOnLabelAttr() throws Exception {
     scratch.file(
         "test/aspect.bzl",