From 1009e8aab8312158cb67256d5c44b6bbc66c06e3 Mon Sep 17 00:00:00 2001 From: Jon Brandvein Date: Thu, 4 Aug 2016 15:08:20 +0000 Subject: Check that deeply nested values in sets are immutable -- MOS_MIGRATED_REVID=129331086 --- .../devtools/build/lib/syntax/ClassObject.java | 46 ++++++++++++++++------ .../devtools/build/lib/syntax/SkylarkList.java | 4 +- .../build/lib/syntax/SkylarkNestedSet.java | 14 ++++--- .../devtools/build/lib/syntax/EvalUtilsTest.java | 39 ++++++++++++++++-- .../build/lib/syntax/SkylarkNestedSetTest.java | 17 +++++++- 5 files changed, 94 insertions(+), 26 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java index 638936d9c3..79c19610e0 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java @@ -19,10 +19,10 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.common.collect.Sets.SetView; -import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; +import com.google.devtools.build.lib.skylarkinterface.SkylarkValue; import com.google.devtools.build.lib.util.Preconditions; import java.io.Serializable; @@ -56,7 +56,6 @@ public interface ClassObject { /** An implementation class of ClassObject for structs created in Skylark code. */ // TODO(bazel-team): maybe move the SkylarkModule annotation to the ClassObject interface? - @Immutable @SkylarkModule( name = "struct", category = SkylarkModuleCategory.BUILTIN, @@ -65,7 +64,7 @@ public interface ClassObject { + "See the global struct function " + "for more details." ) - public class SkylarkClassObject implements ClassObject, Serializable { + public class SkylarkClassObject implements ClassObject, SkylarkValue, Serializable { /** Error message to use when errorMessage argument is null. */ private static final String DEFAULT_ERROR_MESSAGE = "'struct' object has no attribute '%s'"; @@ -73,6 +72,16 @@ public interface ClassObject { private final Location creationLoc; private final String errorMessage; + /** + * Primarily for testing purposes where no location is available and the default + * errorMessage suffices. + */ + public SkylarkClassObject(Map values) { + this.values = copyValues(values); + this.creationLoc = null; + this.errorMessage = DEFAULT_ERROR_MESSAGE; + } + /** * Creates a built-in struct (i.e. without creation loc). The errorMessage has to have * exactly one '%s' parameter to substitute the struct field name. @@ -144,27 +153,40 @@ public interface ClassObject { return String.format(errorMessage, name) + "\n" + suffix; } + @Override + public boolean isImmutable() { + for (Object item : values.values()) { + if (!EvalUtils.isImmutable(item)) { + return false; + } + } + return true; + } + /** * Convert the object to string using Skylark syntax. The output tries to be * reversible (but there is no guarantee, it depends on the actual values). */ @Override - public String toString() { - StringBuilder builder = new StringBuilder(); + public void write(Appendable buffer, char quotationMark) { boolean first = true; - builder.append("struct("); + Printer.append(buffer, "struct("); // Sort by key to ensure deterministic output. for (String key : Ordering.natural().sortedCopy(values.keySet())) { if (!first) { - builder.append(", "); + Printer.append(buffer, ", "); } first = false; - builder.append(key); - builder.append(" = "); - Printer.write(builder, values.get(key)); + Printer.append(buffer, key); + Printer.append(buffer, " = "); + Printer.write(buffer, values.get(key), quotationMark); } - builder.append(")"); - return builder.toString(); + Printer.append(buffer, ")"); + } + + @Override + public String toString() { + return Printer.repr(this); } } } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java index debe96856f..51be3e9b2d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.syntax; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; @@ -268,7 +267,7 @@ public abstract class SkylarkList extends MutableCollection implements Lis } /** - * Builds a Skylark list (actually immutable) from a variable number of arguments. + * Builds a Skylark list from a variable number of arguments. * @param env an Environment from which to inherit Mutability, or null for immutable * @param contents the contents of the list * @return a Skylark list containing the specified arguments as elements. @@ -421,7 +420,6 @@ public abstract class SkylarkList extends MutableCollection implements Lis + "('a', 'b', 'c', 'd')[3:0:-1] # ('d', 'c', 'b')" + "Tuples are immutable, therefore x[1] = \"a\" is not supported." ) - @Immutable public static final class Tuple extends SkylarkList { private final ImmutableList contents; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java index bd4be21685..0f8a3d31bc 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java @@ -125,6 +125,7 @@ public final class SkylarkNestedSet implements Iterable, SkylarkValue { // TODO(bazel-team): we should check ImmutableList here but it screws up genrule at line 43 for (Object object : (SkylarkList) item) { contentType = checkType(contentType, SkylarkType.of(object.getClass()), loc); + checkImmutable(object, loc); items.add(object); } } else { @@ -186,15 +187,11 @@ public final class SkylarkNestedSet implements Iterable, SkylarkValue { private static SkylarkType checkType(SkylarkType builderType, SkylarkType itemType, Location loc) throws EvalException { if (SkylarkType.intersection( - SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST, SkylarkType.STRUCT), + SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST), itemType) != SkylarkType.BOTTOM) { throw new EvalException( loc, String.format("sets cannot contain items of type '%s'", itemType)); } - if (!EvalUtils.isImmutable(itemType.getType())) { - throw new EvalException( - loc, String.format("sets cannot contain items of type '%s' (mutable type)", itemType)); - } SkylarkType newType = SkylarkType.intersection(builderType, itemType); if (newType == SkylarkType.BOTTOM) { throw new EvalException( @@ -204,6 +201,13 @@ public final class SkylarkNestedSet implements Iterable, SkylarkValue { return newType; } + private static void checkImmutable(Object o, Location loc) throws EvalException { + if (!EvalUtils.isImmutable(o)) { + throw new EvalException( + loc, "sets cannot contain mutable items"); + } + } + /** * Returns the NestedSet embedded in this SkylarkNestedSet if it is of the parameter type. */ 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 95423b6544..26763ea796 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 @@ -19,16 +19,16 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.util.EvaluationTestCase; - +import java.util.TreeMap; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import java.util.TreeMap; - /** * Test properties of the evaluator's datatypes and utility functions * without actually creating any parse trees. @@ -44,6 +44,21 @@ public class EvalUtilsTest extends EvaluationTestCase { return SkylarkDict.of(env, 1, 1, 2, 2); } + private static SkylarkClassObject makeStruct(String field, Object value) { + return new SkylarkClassObject(ImmutableMap.of(field, value)); + } + + private static SkylarkClassObject makeBigStruct(Environment env) { + // struct(a=[struct(x={1:1}), ()], b=(), c={2:2}) + return new SkylarkClassObject(ImmutableMap.of( + "a", MutableList.of(env, + new SkylarkClassObject(ImmutableMap.of( + "x", SkylarkDict.of(env, 1, 1))), + Tuple.of()), + "b", Tuple.of(), + "c", SkylarkDict.of(env, 2, 2))); + } + @Test public void testEmptyStringToIterable() throws Exception { assertThat(EvalUtils.toIterable("", null)).isEmpty(); @@ -65,10 +80,15 @@ public class EvalUtilsTest extends EvaluationTestCase { } @Test - public void testDatatypeMutability() throws Exception { + public void testDatatypeMutabilityPrimitive() throws Exception { assertTrue(EvalUtils.isImmutable("foo")); assertTrue(EvalUtils.isImmutable(3)); + } + + @Test + public void testDatatypeMutabilityShallow() throws Exception { assertTrue(EvalUtils.isImmutable(Tuple.of(1, 2, 3))); + assertTrue(EvalUtils.isImmutable(makeStruct("a", 1))); // Mutability depends on the environment. assertTrue(EvalUtils.isImmutable(makeList(null))); @@ -77,6 +97,17 @@ public class EvalUtilsTest extends EvaluationTestCase { assertFalse(EvalUtils.isImmutable(makeDict(env))); } + @Test + public void testDatatypeMutabilityDeep() throws Exception { + assertTrue(EvalUtils.isImmutable(Tuple.of(makeList(null)))); + assertTrue(EvalUtils.isImmutable(makeStruct("a", makeList(null)))); + assertTrue(EvalUtils.isImmutable(makeBigStruct(null))); + + assertFalse(EvalUtils.isImmutable(Tuple.of(makeList(env)))); + assertFalse(EvalUtils.isImmutable(makeStruct("a", makeList(env)))); + assertFalse(EvalUtils.isImmutable(makeBigStruct(env))); + } + @Test public void testComparatorWithDifferentTypes() throws Exception { TreeMap map = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR); diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java index dae2667d54..5c6535e8c9 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.util.EvaluationTestCase; @@ -26,6 +27,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -154,8 +156,19 @@ public class SkylarkNestedSetTest extends EvaluationTestCase { } @Test - public void testNsetBadCompositeItem() throws Exception { - checkEvalError("sets cannot contain items of type 'struct'", "set([struct(a='a')])"); + public void testNsetGoodCompositeItem() throws Exception { + eval("def func():", + " return set([struct(a='a')])", + "s = func()"); + Collection result = get("s").toCollection(); + assertThat(result).hasSize(1); + assertThat(result.iterator().next()).isInstanceOf(SkylarkClassObject.class); + } + + @Test + public void testNsetBadMutableItem() throws Exception { + checkEvalError("sets cannot contain mutable items", "set([([],)])"); + checkEvalError("sets cannot contain mutable items", "set([struct(a=[])])"); } @Test -- cgit v1.2.3