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=113394826

8 files changed, 34 insertions, 25 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 717a132003..6fedceee06 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(
-        "Expected a list of aspects for 'aspects'",
+        "Illegal argument: expected type aspect for 'aspects' element but got type int instead",
         "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 2fb7db04af..ae3de80459 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,7 +22,6 @@ 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;
@@ -33,6 +32,7 @@ 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;
@@ -544,7 +544,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(ImmutableMap.class);
+    assertThat(result).isInstanceOf(SkylarkDict.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 e83651cff0..68178db8ec 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
@@ -37,6 +37,7 @@ 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;
@@ -68,7 +69,8 @@ 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'")}
+    optionalNamedOnly = {@Param(name = "optional_key", doc = "", defaultValue = "'x'")},
+    useEnvironment = true
   )
   private BuiltinFunction mockFunc;
 
@@ -121,8 +123,10 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
         new BuiltinFunction("mock") {
           @SuppressWarnings("unused")
           public Object invoke(
-              Object mandatory, Object optional, Object mandatoryKey, Object optionalKey) {
+              Object mandatory, Object optional, Object mandatoryKey, Object optionalKey,
+              Environment env) {
             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 4989cbd900..cd0129dd70 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,8 +26,6 @@ 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;
 
 /**
@@ -37,8 +35,8 @@ import java.util.TreeMap;
 @RunWith(JUnit4.class)
 public class EvalUtilsTest {
 
-  private static Map<Object, Object> makeDict() {
-    return new LinkedHashMap<>();
+  private static SkylarkDict<Object, Object> makeDict() {
+    return SkylarkDict.<Object, Object>of(null);
   }
 
   @Test
@@ -79,7 +77,7 @@ public class EvalUtilsTest {
     map.put("test", 7);
     map.put(-1, 2);
     map.put("4", 6);
-    map.put(2.0, 1);
+    map.put(true, 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 7a1033838e..4396608f66 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 kwargs;
+        return SkylarkDict.copyOf(env, 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 a894ddfe64..7bbe8eafa7 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(newElements: Iterable) is not applicable for arguments (string): "
-                + "'newElements' is string, but should be Iterable",
+            "Method set.union(new_elements: Iterable) is not applicable for arguments (string): "
+                + "'new_elements' 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(newElements: Iterable) is not applicable for arguments (string): "
-                + "'newElements' is string, but should be Iterable",
+            "Method set.union(new_elements: Iterable) is not applicable for arguments (string): "
+                + "'new_elements' 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 6aae5c1b78..7ee189cd9e 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,6 +132,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
     }
   }
 
+  @SkylarkModule(name = "MockClassObject", doc = "", documented = false)
   static final class MockClassObject implements ClassObject {
     @Override
     public Object getValue(String name) {
@@ -835,11 +836,11 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   }
 
   @Test
-  public void testDictAssignmentAsLValueNoSideEffects() throws Exception {
+  public void testDictAssignmentAsLValueSideEffects() throws Exception {
     new SkylarkTest().setUp("def func(d):",
         "  d['b'] = 2",
         "d = {'a' : 1}",
-        "func(d)").testLookup("d", ImmutableMap.of("a", 1));
+        "func(d)").testLookup("d", SkylarkDict.of(null, "a", 1, "b", 2));
   }
 
   @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 4c6eb6ac63..c44e5c918b 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,8 +322,14 @@ public class ValidationTest extends EvaluationTestCase {
 
     // Also for ClassObject
     assertThat(SkylarkType.of(ClassObject.SkylarkClassObject.class)).isEqualTo(SkylarkType.STRUCT);
-    // TODO(bazel-team): fix that?
-    assertThat(SkylarkType.of(ClassObject.class)).isNotEqualTo(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");
+    }
 
     // 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?
@@ -342,17 +348,17 @@ public class ValidationTest extends EvaluationTestCase {
     assertThat(SkylarkType.LIST.includes(combo1)).isTrue();
 
     SkylarkType union1 =
-        SkylarkType.Union.of(SkylarkType.MAP, SkylarkType.LIST, SkylarkType.STRUCT);
-    assertThat(union1.includes(SkylarkType.MAP)).isTrue();
+        SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST, SkylarkType.STRUCT);
+    assertThat(union1.includes(SkylarkType.DICT)).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.MAP, SkylarkType.STRING, SkylarkType.INT);
+            SkylarkType.LIST, SkylarkType.DICT, SkylarkType.STRING, SkylarkType.INT);
     SkylarkType inter1 = SkylarkType.intersection(union1, union2);
-    assertThat(inter1.includes(SkylarkType.MAP)).isTrue();
+    assertThat(inter1.includes(SkylarkType.DICT)).isTrue();
     assertThat(inter1.includes(SkylarkType.LIST)).isTrue();
     assertThat(inter1.includes(combo1)).isTrue();
     assertThat(inter1.includes(SkylarkType.INT)).isFalse();