diff options
Diffstat (limited to 'src')
5 files changed, 24 insertions, 19 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java index 9448c32b2b..5d44265578 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java @@ -212,10 +212,10 @@ public abstract class EvalUtils { } else if (c.equals(Void.TYPE) || c.equals(Environment.NoneType.class)) { return "None"; } else if (List.class.isAssignableFrom(c)) { - // TODO(bazel-team): for better debugging, we should distinguish "java tuple" and "java list" - // from "tuple" and "list" -- or better yet, only use one set of pure data structures - // everywhere and eliminate all calls to .append and .extend from the code base. - return isTuple(c) ? "tuple" : "list"; + // NB: the capital here is a subtle way to distinguish java Tuple and java List + // from native SkylarkList tuple and list. + // TODO(bazel-team): refactor SkylarkList and use it everywhere. + return isTuple(c) ? "Tuple" : "List"; } else if (GlobList.class.isAssignableFrom(c)) { return "glob list"; } else if (Map.class.isAssignableFrom(c)) { diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java index 2272be68a2..f6cde1241a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java @@ -136,7 +136,7 @@ public abstract class SkylarkType { } public boolean includes(SkylarkType other) { - return intersection(this, other) == other; + return intersection(this, other).equals(other); } public boolean includes(Class<?> other) { @@ -149,9 +149,6 @@ public abstract class SkylarkType { private final class Empty { }; // Empty type, used as basis for Bottom - private static Map<Class<?>, Simple> simpleCache = // cache used by Simple - new HashMap<Class<?>, Simple>(); - // Notable types /** A singleton for the TOP type, that at analysis time means that any type is possible. */ @@ -250,12 +247,12 @@ public abstract class SkylarkType { public static class Simple extends SkylarkType { private final Class<?> type; - public Simple(Class<?> type) { + private Simple(Class<?> type) { this.type = type; } @Override public boolean contains(Object value) { - return type.isAssignableFrom(value.getClass()); + return value != null && type.isInstance(value); } @Override public Class<?> getType() { return type; @@ -273,7 +270,15 @@ public abstract class SkylarkType { @Override public boolean canBeCastTo(Class<?> type) { return this.type == type || super.canBeCastTo(type); } - public static Simple of(Class<?> type) { + private static HashMap<Class<?>, Simple> simpleCache = new HashMap<>(); + + /** + * The public way to create a Simple type + * @param type a Class + * @return the Simple type that contains exactly the instances of that Class + */ + // NB: synchronized to avoid race conditions filling that cache. + public static synchronized Simple of(Class<?> type) { Simple cached = simpleCache.get(type); if (cached != null) { return cached; @@ -308,14 +313,14 @@ public abstract class SkylarkType { // and in practice actually one of SkylarkList or SkylarkNestedSet private final SkylarkType genericType; // actually always a Simple, for now. private final SkylarkType argType; // not always Simple - public Combination(SkylarkType genericType, SkylarkType argType) { + private Combination(SkylarkType genericType, SkylarkType argType) { this.genericType = genericType; this.argType = argType; } public boolean contains(Object value) { // The empty collection is member of compatible types - if (!genericType.contains(value)) { + if (value == null || !genericType.contains(value)) { return false; } else { SkylarkType valueArgType = getGenericArgType(value); diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java index e32005065c..3d463f3018 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java @@ -97,7 +97,7 @@ public class BuildFileASTTest { assertFalse(buildfile.exec(env, reporter)); Event e = JunitTestUtils.assertContainsEvent(collector, - "unsupported operand type(s) for +: 'int' and 'list'"); + "unsupported operand type(s) for +: 'int' and 'List'"); assertEquals(4, e.getLocation().getStartLineAndColumn().getLine()); } 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 7aad74fba9..9da4ed2f81 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 @@ -61,8 +61,8 @@ public class EvalUtilsTest { public void testDataTypeNames() throws Exception { assertEquals("string", EvalUtils.getDataTypeName("foo")); assertEquals("int", EvalUtils.getDataTypeName(3)); - assertEquals("tuple", EvalUtils.getDataTypeName(makeTuple(1, 2, 3))); - assertEquals("list", EvalUtils.getDataTypeName(makeList(1, 2, 3))); + assertEquals("Tuple", EvalUtils.getDataTypeName(makeTuple(1, 2, 3))); + assertEquals("List", EvalUtils.getDataTypeName(makeList(1, 2, 3))); assertEquals("dict", EvalUtils.getDataTypeName(makeDict())); assertEquals("FilesetEntry", EvalUtils.getDataTypeName(makeFilesetEntry())); assertEquals("None", EvalUtils.getDataTypeName(Environment.NONE)); 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 f33b500c32..c31b17b161 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 @@ -302,7 +302,7 @@ public class EvaluationTest extends AbstractEvaluationTestCase { assertTrue(EvalUtils.isImmutable(x)); checkEvalError("(1,2) + [3,4]", // list + tuple - "can only concatenate list (not \"tuple\") to list"); + "can only concatenate List (not \"Tuple\") to List"); } @SuppressWarnings("unchecked") @@ -380,8 +380,8 @@ public class EvaluationTest extends AbstractEvaluationTestCase { public void testListConcatenation() throws Exception { assertEquals(Arrays.asList(1, 2, 3, 4), eval("[1, 2] + [3, 4]", env)); assertEquals(ImmutableList.of(1, 2, 3, 4), eval("(1, 2) + (3, 4)", env)); - checkEvalError("[1, 2] + (3, 4)", "can only concatenate tuple (not \"list\") to tuple"); - checkEvalError("(1, 2) + [3, 4]", "can only concatenate list (not \"tuple\") to list"); + checkEvalError("[1, 2] + (3, 4)", "can only concatenate Tuple (not \"List\") to Tuple"); + checkEvalError("(1, 2) + [3, 4]", "can only concatenate List (not \"Tuple\") to List"); } @Test |