Remove provider safety check and make all old ctx objects featureless

It's now allowed to return anything from a rule implementation function.
If a ctx object is returned it becomes featureless and cannot be used from
anywhere else (i.e. from an implementation function of another rule).

Fixes #2319

--
PiperOrigin-RevId: 151015919
MOS_MIGRATED_REVID=151015919

3 files changed, 115 insertions, 48 deletions

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 976c268e97..528fe99064 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
@@ -701,34 +701,6 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
-  public void aspectFailingReturnsUnsafeObject() throws Exception {
-    scratch.file(
-        "test/aspect.bzl",
-        "def foo():",
-        "   return 0",
-        "def _impl(target, ctx):",
-        "   return struct(x = foo)",
-        "",
-        "MyAspect = aspect(implementation=_impl)");
-    scratch.file("test/BUILD", "java_library(name = 'xxx',)");
-
-    reporter.removeHandler(failFastHandler);
-    try {
-      AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
-      assertThat(keepGoing()).isTrue();
-      assertThat(result.hasError()).isTrue();
-    } catch (ViewCreationFailedException e) {
-      // expect to fail.
-    }
-    assertContainsEvent(
-        "ERROR /workspace/test/BUILD:1:1: in //test:aspect.bzl%MyAspect aspect on java_library rule"
-        + " //test:xxx: \n"
-        + "\n"
-        + "\n"
-        + "/workspace/test/aspect.bzl:4:11: Value of provider 'x' is of an illegal type: function");
-  }
-
-  @Test
   public void aspectFailingOrphanArtifacts() throws Exception {
     scratch.file(
         "test/aspect.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 14f783e915..79231d663d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -972,26 +972,6 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
   }
 
   @Test
-  public void testProviderValidation() throws Exception {
-    // This does not have full coverage for SkylarkProviderValidationUtil, which eventually
-    // should be factored into a more general way for validating that an object is deeply
-    // Skylark-permissible. It's just a regression test for specific issues.
-    reporter.removeHandler(failFastHandler);
-    scratch.file(
-        "test/skylark/extension.bzl",
-        "def _impl(ctx):",
-        "  return struct(bad=depset([depset([])]))",
-        "my_rule = rule(implementation = _impl)");
-    scratch.file(
-        "test/skylark/BUILD",
-        "load('/test/skylark/extension', 'my_rule')",
-        "my_rule(name = 'r')");
-
-    getConfiguredTarget("//test/skylark:r");
-    assertContainsEvent("Value of provider 'bad' is of an illegal type: depset\n");
-  }
-
-  @Test
   public void testRecursionDetection() throws Exception {
     reporter.removeHandler(failFastHandler);
     scratch.file(
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index b5d7108dd7..ef609dbb83 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -42,6 +42,7 @@ import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.ArrayList;
 import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
@@ -1733,4 +1734,118 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         (Label) evalRuleContextCode(ruleContext, "ruleContext.attr.runtimes.values()[0]");
     assertThat(valueLabel).isEqualTo(Label.parseAbsolute("//jvm:runtime"));
   }
+
+  // A list of attributes and methods ctx objects have
+  private final List<String> ctxAttributes = ImmutableList.of(
+      "attr",
+      "split_attr",
+      "executable",
+      "file",
+      "files",
+      "workspace_name",
+      "label",
+      "fragments",
+      "host_fragments",
+      "configuration",
+      "host_configuration",
+      "coverage_instrumented(dep)",
+      "features",
+      "bin_dir",
+      "genfiles_dir",
+      "outputs",
+      "rule",
+      "aspect_ids",
+      "var",
+      "tokenize('foo')",
+      "expand('foo', [], Label('//test:main'))",
+      "new_file('foo.txt')",
+      "new_file(file, 'foo.txt')",
+      "check_placeholders('foo', [])",
+      "action(command = 'foo', outputs = [file])",
+      "file_action(file, 'foo')",
+      "empty_action(mnemonic = 'foo', inputs = [file])",
+      "template_action(template = file, output = file, substitutions = {})",
+      "runfiles()",
+      "resolve_command(command = 'foo')");
+
+  @Test
+  public void testFrozenRuleContextHasInaccessibleAttributes() throws Exception {
+    scratch.file("test/BUILD",
+        "load('/test/rules', 'main_rule', 'dep_rule')",
+        "dep_rule(name = 'dep')",
+        "main_rule(name = 'main', deps = [':dep'])");
+    scratch.file("test/rules.bzl");
+
+    for (String attribute : ctxAttributes) {
+      scratch.overwriteFile("test/rules.bzl",
+          "def _main_impl(ctx):",
+          "  dep = ctx.attr.deps[0]",
+          "  file = ctx.outputs.file",
+          "  foo = dep.dep_ctx." + attribute,
+          "main_rule = rule(",
+          "  implementation = _main_impl,",
+          "  attrs = {",
+          "    'deps': attr.label_list()",
+          "  },",
+          "  outputs = {'file': 'output.txt'},",
+          ")",
+          "def _dep_impl(ctx):",
+          "  return struct(dep_ctx = ctx)",
+          "dep_rule = rule(implementation = _dep_impl)");
+      invalidatePackages();
+      try {
+        getConfiguredTarget("//test:main");
+        fail("Should have been unable to access dep_ctx." + attribute);
+      } catch (AssertionError e) {
+        assertThat(e.getMessage()).contains("cannot access field or method '"
+            + attribute.split("\\(")[0]
+            + "' of rule context for '//test:dep' outside of its own rule implementation function");
+      }
+    }
+  }
+
+  @Test
+  public void testFrozenRuleContextForAspectsHasInaccessibleAttributes() throws Exception {
+    List<String> attributes = new ArrayList<>();
+    attributes.addAll(ctxAttributes);
+    attributes.addAll(ImmutableList.of(
+        "rule.attr",
+        "rule.executable",
+        "rule.file",
+        "rule.files",
+        "rule.kind"));
+    scratch.file("test/BUILD",
+        "load('/test/rules', 'my_rule')",
+        "my_rule(name = 'dep')",
+        "my_rule(name = 'mid', deps = [':dep'])",
+        "my_rule(name = 'main', deps = [':mid'])");
+    scratch.file("test/rules.bzl");
+    for (String attribute : attributes) {
+       scratch.overwriteFile("test/rules.bzl",
+          "def _rule_impl(ctx):",
+          "  pass",
+          "def _aspect_impl(target, ctx):",
+          "  if ctx.rule.attr.deps:",
+          "    dep = ctx.rule.attr.deps[0]",
+          "    file = ctx.new_file('file.txt')",
+          "    foo = dep." + (attribute.startsWith("rule.") ? "" : "ctx.") + attribute,
+          "  return struct(ctx = ctx, rule=ctx.rule)",
+          "MyAspect = aspect(implementation=_aspect_impl)",
+          "my_rule = rule(",
+          "  implementation = _rule_impl,",
+          "  attrs = {",
+          "    'deps': attr.label_list(aspects = [MyAspect])",
+          "  },",
+          ")");
+      invalidatePackages();
+      try {
+        getConfiguredTarget("//test:main");
+        fail("Should have been unable to access dep." + attribute);
+      } catch (AssertionError e) {
+        assertThat(e.getMessage()).contains("cannot access field or method '"
+            + attribute.split("\\(")[0]
+            + "' of rule context for '//test:dep' outside of its own rule implementation function");
+      }
+    }
+  }
 }