Refactor Skylark rules and attributes in preparation to Skylark aspects.

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

--
MOS_MIGRATED_REVID=108774581
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 23458c7..bca93ac 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
@@ -28,12 +28,16 @@
 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.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.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 org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -71,15 +75,22 @@
 
   @Test
   public void testCannotOverrideBuiltInAttribute() throws Exception {
-    checkEvalError(
-        "There is already a built-in attribute 'tags' which cannot be overridden",
-        "def impl(ctx): return",
-        "r = rule(impl, attrs = {'tags': attr.string_list()})");
+    ev.setFailFast(true);
+    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 (IllegalArgumentException | EvalException e) {
+      assertThat(e).hasMessage(
+          "There is already a built-in attribute 'tags' which cannot be overridden");
+    }
   }
 
   @Test
   public void testImplicitArgsAttribute() throws Exception {
-    eval(
+    evalAndExport(
         "def _impl(ctx):",
         "  pass",
         "exec_rule = rule(implementation = _impl, executable = True)",
@@ -89,59 +100,57 @@
   }
 
   private RuleClass getRuleClass(String name) throws Exception {
-    return ((RuleFunction) lookup(name)).getBuilder().build(name);
+    return ((RuleFunction) lookup(name)).getRuleClass();
   }
 
   private void registerDummyUserDefinedFunction() throws Exception {
     eval("def impl():\n" + "  return 0\n");
   }
 
+  private Attribute.Builder<?> evalAttributeDefinition(String... lines) throws Exception {
+    return ((SkylarkAttr.Descriptor) evalRuleClassCode(lines)).getAttributeBuilder();
+  }
+
   @Test
   public void testAttrWithOnlyType() throws Exception {
-    Object result = evalRuleClassCode("attr.string_list()");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string_list()").build("a1");
     assertEquals(Type.STRING_LIST, attr.getType());
   }
 
   @Test
   public void testOutputListAttr() throws Exception {
-    Object result = evalRuleClassCode("attr.output_list()");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.output_list()").build("a1");
     assertEquals(BuildType.OUTPUT_LIST, attr.getType());
   }
 
   @Test
   public void testIntListAttr() throws Exception {
-    Object result = evalRuleClassCode("attr.int_list()");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.int_list()").build("a1");
     assertEquals(Type.INTEGER_LIST, attr.getType());
   }
 
   @Test
   public void testOutputAttr() throws Exception {
-    Object result = evalRuleClassCode("attr.output()");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.output()").build("a1");
     assertEquals(BuildType.OUTPUT, attr.getType());
   }
 
   @Test
   public void testStringDictAttr() throws Exception {
-    Object result = evalRuleClassCode("attr.string_dict(default = {'a': 'b'})");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string_dict(default = {'a': 'b'})").build("a1");
     assertEquals(Type.STRING_DICT, attr.getType());
   }
 
   @Test
   public void testStringListDictAttr() throws Exception {
-    Object result = evalRuleClassCode("attr.string_list_dict(default = {'a': ['b', 'c']})");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string_list_dict(default = {'a': ['b', 'c']})")
+        .build("a1");
     assertEquals(Type.STRING_LIST_DICT, attr.getType());
   }
 
   @Test
   public void testAttrAllowedFileTypesAnyFile() throws Exception {
-    Object result = evalRuleClassCode("attr.label_list(allow_files = True)");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.label_list(allow_files = True)").build("a1");
     assertEquals(FileTypeSet.ANY_FILE, attr.getAllowedFileTypesPredicate());
   }
 
@@ -154,17 +163,17 @@
 
   @Test
   public void testAttrWithSkylarkFileType() throws Exception {
-    Object result = evalRuleClassCode("attr.label_list(allow_files = FileType(['.xml']))");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.label_list(allow_files = FileType(['.xml']))")
+        .build("a1");
     assertTrue(attr.getAllowedFileTypesPredicate().apply("a.xml"));
     assertFalse(attr.getAllowedFileTypesPredicate().apply("a.txt"));
   }
 
   @Test
   public void testAttrWithProviders() throws Exception {
-    Object result =
-        evalRuleClassCode("attr.label_list(allow_files = True, providers = ['a', 'b'])");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr =
+        evalAttributeDefinition("attr.label_list(allow_files = True, providers = ['a', 'b'])")
+        .build("a1");
     assertEquals(ImmutableSet.of("a", "b"), attr.getMandatoryProviders());
   }
 
@@ -191,16 +200,14 @@
   }
   @Test
   public void testAttrAllowedRuleClassesSpecificRuleClasses() throws Exception {
-    Object result =
-        evalRuleClassCode("attr.label_list(allow_rules = ['java_binary'], allow_files = True)");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a");
+    Attribute attr = evalAttributeDefinition(
+        "attr.label_list(allow_rules = ['java_binary'], allow_files = True)").build("a");
     assertTrue(attr.getAllowedRuleClassesPredicate().apply(ruleClass("java_binary")));
     assertFalse(attr.getAllowedRuleClassesPredicate().apply(ruleClass("genrule")));
   }
   @Test
   public void testAttrDefaultValue() throws Exception {
-    Object result = evalRuleClassCode("attr.string(default = 'some value')");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string(default = 'some value')").build("a1");
     assertEquals("some value", attr.getDefaultValueForTesting());
   }
 
@@ -215,16 +222,14 @@
 
   @Test
   public void testAttrMandatory() throws Exception {
-    Object result = evalRuleClassCode("attr.string(mandatory=True)");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string(mandatory=True)").build("a1");
     assertTrue(attr.isMandatory());
     assertFalse(attr.isNonEmpty());
   }
 
   @Test
   public void testAttrNonEmpty() throws Exception {
-    Object result = evalRuleClassCode("attr.string_list(non_empty=True)");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string_list(non_empty=True)").build("a1");
     assertTrue(attr.isNonEmpty());
     assertFalse(attr.isMandatory());
   }
@@ -237,15 +242,14 @@
 
   @Test
   public void testAttrCfg() throws Exception {
-    Object result = evalRuleClassCode("attr.label(cfg = HOST_CFG, allow_files = True)");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.label(cfg = HOST_CFG, allow_files = True)")
+        .build("a1");
     assertEquals(ConfigurationTransition.HOST, attr.getConfigurationTransition());
   }
 
   @Test
   public void testAttrValues() throws Exception {
-    Object result = evalRuleClassCode("attr.string(values = ['ab', 'cd'])");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.string(values = ['ab', 'cd'])").build("a1");
     PredicateWithMessage<Object> predicate = attr.getAllowedValues();
     assertThat(predicate.apply("ab")).isTrue();
     assertThat(predicate.apply("xy")).isFalse();
@@ -253,8 +257,7 @@
 
   @Test
   public void testAttrIntValues() throws Exception {
-    Object result = evalRuleClassCode("attr.int(values = [1, 2])");
-    Attribute attr = ((Attribute.Builder<?>) result).build("a1");
+    Attribute attr = evalAttributeDefinition("attr.int(values = [1, 2])").build("a1");
     PredicateWithMessage<Object> predicate = attr.getAllowedValues();
     assertThat(predicate.apply(2)).isTrue();
     assertThat(predicate.apply(3)).isFalse();
@@ -262,8 +265,8 @@
 
   @Test
   public void testRuleImplementation() throws Exception {
-    eval("def impl(ctx): return None", "rule1 = rule(impl)");
-    RuleClass c = ((RuleFunction) lookup("rule1")).getBuilder().build("rule1");
+    evalAndExport("def impl(ctx): return None", "rule1 = rule(impl)");
+    RuleClass c = ((RuleFunction) lookup("rule1")).getRuleClass();
     assertEquals("impl", c.getConfiguredTargetFunction().getName());
   }
 
@@ -277,46 +280,55 @@
         "attr.string(default=attr_value)");
   }
 
+  private static final Label FAKE_LABEL = Label.parseAbsoluteUnchecked("//fake/label.bzl");
+
   @Test
   public void testRuleAddAttribute() throws Exception {
-    eval("def impl(ctx): return None", "r1 = rule(impl, attrs={'a1': attr.string()})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    evalAndExport("def impl(ctx): return None", "r1 = rule(impl, attrs={'a1': attr.string()})");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     assertTrue(c.hasAttr("a1", Type.STRING));
   }
 
+  protected void evalAndExport(String... lines) throws Exception {
+    eval(lines);
+    SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(ev.getEnvironment(), FAKE_LABEL);
+  }
+
   @Test
   public void testOutputToGenfiles() throws Exception {
-    eval("def impl(ctx): pass", "r1 = rule(impl, output_to_genfiles=True)");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    evalAndExport("def impl(ctx): pass", "r1 = rule(impl, output_to_genfiles=True)");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     assertFalse(c.hasBinaryOutput());
   }
 
   @Test
   public void testRuleAddMultipleAttributes() throws Exception {
-    eval(
+    evalAndExport(
         "def impl(ctx): return None",
         "r1 = rule(impl,",
         "     attrs = {",
         "            'a1': attr.label_list(allow_files=True),",
         "            'a2': attr.int()",
         "})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     assertTrue(c.hasAttr("a1", BuildType.LABEL_LIST));
     assertTrue(c.hasAttr("a2", Type.INTEGER));
   }
   @Test
   public void testRuleAttributeFlag() throws Exception {
-    eval(
+    evalAndExport(
         "def impl(ctx): return None",
         "r1 = rule(impl, attrs = {'a1': attr.string(mandatory=True)})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     assertTrue(c.getAttributeByName("a1").isMandatory());
   }
 
   @Test
   public void testRuleOutputs() throws Exception {
-    eval("def impl(ctx): return None", "r1 = rule(impl, outputs = {'a': 'a.txt'})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    evalAndExport(
+        "def impl(ctx): return None",
+        "r1 = rule(impl, outputs = {'a': 'a.txt'})");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     ImplicitOutputsFunction function = c.getImplicitOutputsFunction();
     assertEquals("a.txt", Iterables.getOnlyElement(function.getImplicitOutputs(null)));
   }
@@ -351,14 +363,15 @@
     registerDummyUserDefinedFunction();
     checkErrorContains(
         "Illegal argument: "
-            + "expected <String, Builder> type for 'attrs' but got <string, string> instead",
+            + "expected <String, Descriptor> type for 'attrs' but got <string, string> instead",
         "rule(impl, attrs = {'a1': 'some text'})");
   }
 
   @Test
   public void testLabel() throws Exception {
     Object result = evalRuleClassCode("Label('//foo/foo:foo')");
-    assertEquals("//foo/foo:foo", ((Label) result).toString());
+    assertThat(result).isInstanceOf(Label.class);
+    assertEquals("//foo/foo:foo", result.toString());
   }
 
   @Test
@@ -380,19 +393,22 @@
 
   @Test
   public void testRuleLabelDefaultValue() throws Exception {
-    eval(
+    evalAndExport(
         "def impl(ctx): return None\n"
             + "r1 = rule(impl, attrs = {'a1': "
             + "attr.label(default = Label('//foo:foo'), allow_files=True)})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     Attribute a = c.getAttributeByName("a1");
-    assertEquals("//foo:foo", ((Label) a.getDefaultValueForTesting()).toString());
+    assertThat(a.getDefaultValueForTesting()).isInstanceOf(Label.class);
+    assertEquals("//foo:foo", a.getDefaultValueForTesting().toString());
   }
 
   @Test
   public void testIntDefaultValue() throws Exception {
-    eval("def impl(ctx): return None", "r1 = rule(impl, attrs = {'a1': attr.int(default = 40+2)})");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    evalAndExport(
+        "def impl(ctx): return None",
+        "r1 = rule(impl, attrs = {'a1': attr.int(default = 40+2)})");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     Attribute a = c.getAttributeByName("a1");
     assertEquals(42, a.getDefaultValueForTesting());
   }
@@ -406,8 +422,8 @@
 
   @Test
   public void testRuleInheritsBaseRuleAttributes() throws Exception {
-    eval("def impl(ctx): return None", "r1 = rule(impl)");
-    RuleClass c = ((RuleFunction) lookup("r1")).getBuilder().build("r1");
+    evalAndExport("def impl(ctx): return None", "r1 = rule(impl)");
+    RuleClass c = ((RuleFunction) lookup("r1")).getRuleClass();
     assertTrue(c.hasAttr("tags", Type.STRING_LIST));
     assertTrue(c.hasAttr("visibility", BuildType.NODEP_LABEL_LIST));
     assertTrue(c.hasAttr("deprecation", Type.STRING));