Rollback of commit f941d56acfad5f8c819c81b494f806ea74ea7fd8.

*** Reason for rollback ***

Breaks many targets, see []

*** Original change description ***

Reinstate mutable SkylarkDict

Add <String, Object> annotation to optionMap invocation in SkylarkAttr,
to make JDK 1.7 happy.

Give the visible name "aspect" to class SkylarkAspect.

--
MOS_MIGRATED_REVID=113543873

8 files changed, 25 insertions, 34 deletions

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 6fedceee06..717a132003 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
@@ -199,7 +199,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   @Test
   public void testLabelListWithAspectsError() throws Exception {
     checkErrorContains(
-        "Illegal argument: expected type aspect for 'aspects' element but got type int instead",
+        "Expected a list of aspects for 'aspects'",
         "def _impl(target, ctx):",
         "   pass",
         "my_aspect = aspect(implementation = _impl)",
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 98cfe8dea5..e46c55fddf 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
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertSame;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
@@ -32,7 +33,6 @@ import com.google.devtools.build.lib.rules.SkylarkRuleContext;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.rules.python.PythonSourcesProvider;
 import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
-import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import com.google.devtools.build.lib.testutil.TestConstants;
@@ -559,7 +559,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
   public void testSkylarkRuleContextGetDefaultShellEnv() throws Exception {
     SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
     Object result = evalRuleContextCode(ruleContext, "ruleContext.configuration.default_shell_env");
-    assertThat(result).isInstanceOf(SkylarkDict.class);
+    assertThat(result).isInstanceOf(ImmutableMap.class);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 10c7a6c685..961a2cb5ae 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
-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.Printer;
@@ -70,8 +69,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
     mandatoryPositionals = {@Param(name = "mandatory", doc = "")},
     optionalPositionals = {@Param(name = "optional", doc = "")},
     mandatoryNamedOnly = {@Param(name = "mandatory_key", doc = "")},
-    optionalNamedOnly = {@Param(name = "optional_key", doc = "", defaultValue = "'x'")},
-    useEnvironment = true
+    optionalNamedOnly = {@Param(name = "optional_key", doc = "", defaultValue = "'x'")}
   )
   private BuiltinFunction mockFunc;
 
@@ -124,10 +122,8 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
         new BuiltinFunction("mock") {
           @SuppressWarnings("unused")
           public Object invoke(
-              Object mandatory, Object optional, Object mandatoryKey, Object optionalKey,
-              Environment env) {
+              Object mandatory, Object optional, Object mandatoryKey, Object optionalKey) {
             return EvalUtils.optionMap(
-                env,
                 "mandatory",
                 mandatory,
                 "optional",
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
index cd0129dd70..4989cbd900 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
@@ -26,6 +26,8 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
+import java.util.LinkedHashMap;
+import java.util.Map;
 import java.util.TreeMap;
 
 /**
@@ -35,8 +37,8 @@ import java.util.TreeMap;
 @RunWith(JUnit4.class)
 public class EvalUtilsTest {
 
-  private static SkylarkDict<Object, Object> makeDict() {
-    return SkylarkDict.<Object, Object>of(null);
+  private static Map<Object, Object> makeDict() {
+    return new LinkedHashMap<>();
   }
 
   @Test
@@ -77,7 +79,7 @@ public class EvalUtilsTest {
     map.put("test", 7);
     map.put(-1, 2);
     map.put("4", 6);
-    map.put(true, 1);
+    map.put(2.0, 1);
     map.put(Runtime.NONE, 0);
 
     int expected = 0;
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 4396608f66..7a1033838e 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -259,7 +259,7 @@ public class EvaluationTest extends EvaluationTestCase {
           final Map<String, Object> kwargs,
           FuncallExpression ast,
           Environment env) {
-        return SkylarkDict.copyOf(env, kwargs);
+        return kwargs;
       }
     };
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index 7bbe8eafa7..a894ddfe64 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -418,8 +418,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
   public void testBuiltinFunctionErrorMessage() throws Exception {
     new BothModesTest()
         .testIfErrorContains(
-            "Method set.union(new_elements: Iterable) is not applicable for arguments (string): "
-                + "'new_elements' is string, but should be Iterable",
+            "Method set.union(newElements: Iterable) is not applicable for arguments (string): "
+                + "'newElements' is string, but should be Iterable",
             "set([]).union('a')")
         .testIfErrorContains(
             "Method string.startswith(sub: string, start: int, end: int or NoneType) is not "
@@ -1347,8 +1347,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
     new BothModesTest()
         .testIfErrorContains("insufficient arguments received by union", "set(['a']).union()")
         .testIfErrorContains(
-            "Method set.union(new_elements: Iterable) is not applicable for arguments (string): "
-                + "'new_elements' is string, but should be Iterable",
+            "Method set.union(newElements: Iterable) is not applicable for arguments (string): "
+                + "'newElements' is string, but should be Iterable",
             "set(['a']).union('b')");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 7ee189cd9e..6aae5c1b78 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -132,7 +132,6 @@ public class SkylarkEvaluationTest extends EvaluationTest {
     }
   }
 
-  @SkylarkModule(name = "MockClassObject", doc = "", documented = false)
   static final class MockClassObject implements ClassObject {
     @Override
     public Object getValue(String name) {
@@ -836,11 +835,11 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   }
 
   @Test
-  public void testDictAssignmentAsLValueSideEffects() throws Exception {
+  public void testDictAssignmentAsLValueNoSideEffects() throws Exception {
     new SkylarkTest().setUp("def func(d):",
         "  d['b'] = 2",
         "d = {'a' : 1}",
-        "func(d)").testLookup("d", SkylarkDict.of(null, "a", 1, "b", 2));
+        "func(d)").testLookup("d", ImmutableMap.of("a", 1));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
index c44e5c918b..4c6eb6ac63 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -284,7 +284,7 @@ public class ValidationTest extends EvaluationTestCase {
 
   @Test
   public void testParentWithSkylarkModule() throws Exception {
-    Class<?> emptyTupleClass = Tuple.empty().getClass();
+    Class<?> emptyTupleClass = Tuple.EMPTY.getClass();
     Class<?> tupleClass = Tuple.of(1, "a", "b").getClass();
     Class<?> mutableListClass = new MutableList(Tuple.of(1, 2, 3), env).getClass();
 
@@ -311,7 +311,7 @@ public class ValidationTest extends EvaluationTestCase {
   @Test
   public void testSkylarkTypeEquivalence() throws Exception {
     // All subclasses of SkylarkList are made equivalent
-    Class<?> emptyTupleClass = Tuple.empty().getClass();
+    Class<?> emptyTupleClass = Tuple.EMPTY.getClass();
     Class<?> tupleClass = Tuple.of(1, "a", "b").getClass();
     Class<?> mutableListClass = new MutableList(Tuple.of(1, 2, 3), env).getClass();
 
@@ -322,14 +322,8 @@ public class ValidationTest extends EvaluationTestCase {
 
     // Also for ClassObject
     assertThat(SkylarkType.of(ClassObject.SkylarkClassObject.class)).isEqualTo(SkylarkType.STRUCT);
-    try {
-      SkylarkType.of(ClassObject.class);
-      throw new Exception("foo");
-    } catch (Exception e) {
-      assertThat(e.getMessage()).contains(
-          "interface com.google.devtools.build.lib.syntax.ClassObject "
-          + "is not allowed as a Skylark value");
-    }
+    // TODO(bazel-team): fix that?
+    assertThat(SkylarkType.of(ClassObject.class)).isNotEqualTo(SkylarkType.STRUCT);
 
     // Also test for these bazel classes, to avoid some regression.
     // TODO(bazel-team): move to some other place to remove dependency of syntax tests on Artifact?
@@ -348,17 +342,17 @@ public class ValidationTest extends EvaluationTestCase {
     assertThat(SkylarkType.LIST.includes(combo1)).isTrue();
 
     SkylarkType union1 =
-        SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST, SkylarkType.STRUCT);
-    assertThat(union1.includes(SkylarkType.DICT)).isTrue();
+        SkylarkType.Union.of(SkylarkType.MAP, SkylarkType.LIST, SkylarkType.STRUCT);
+    assertThat(union1.includes(SkylarkType.MAP)).isTrue();
     assertThat(union1.includes(SkylarkType.STRUCT)).isTrue();
     assertThat(union1.includes(combo1)).isTrue();
     assertThat(union1.includes(SkylarkType.STRING)).isFalse();
 
     SkylarkType union2 =
         SkylarkType.Union.of(
-            SkylarkType.LIST, SkylarkType.DICT, SkylarkType.STRING, SkylarkType.INT);
+            SkylarkType.LIST, SkylarkType.MAP, SkylarkType.STRING, SkylarkType.INT);
     SkylarkType inter1 = SkylarkType.intersection(union1, union2);
-    assertThat(inter1.includes(SkylarkType.DICT)).isTrue();
+    assertThat(inter1.includes(SkylarkType.MAP)).isTrue();
     assertThat(inter1.includes(SkylarkType.LIST)).isTrue();
     assertThat(inter1.includes(combo1)).isTrue();
     assertThat(inter1.includes(SkylarkType.INT)).isFalse();