respond to review comments

src/python/pants/engine/objects.py

@@ -161,6 +161,8 @@ class Collection(object):
   which may be consumed e.g. over FFI from the engine.
 
   Python consumers of a Collection should prefer to use its standard iteration API.
+
+  Note that elements of a Collection are type-checked upon construction.
   """
 
   @memoized_classmethod

src/python/pants/util/objects.py

@@ -276,16 +276,14 @@ class TypeConstraint(AbstractClass):
   :class:`SubclassesOf`.
   """
 
-  def __init__(self, variance_symbol, description):
+  def __init__(self, description):
     """Creates a type constraint centered around the given types.
 
     The type constraint is satisfied as a whole if satisfied for at least one of the given types.
 
-    :param type *types: The focus of this type constraint.
-    :param str description: A description for this constraint if the list of types is too long.
+    :param str description: A concise, readable description of what the type constraint represents.
+                            Used directly as the __str__ implementation.
     """
-    assert(variance_symbol)
-    self._variance_symbol = variance_symbol
     self._description = description
 
   @abstractmethod
@@ -295,30 +293,39 @@ def satisfied_by(self, obj):
     :rtype: bool
     """
 
-  def validate_satisfied_by(self, obj):
-    """Return some version of `obj` if the object satisfies this type constraint, or raise.
+  def make_type_constraint_error(self, obj, constraint):
+    return TypeConstraintError(
+      "value {!r} (with type {!r}) must satisfy this type constraint: {!r}."
+      .format(obj, type(obj).__name__, constraint))
 
-    # TODO: consider disallowing overriding this too?
+  # TODO: disallow overriding this method with some form of mixin/decorator along with datatype
+  # __eq__!
+  def validate_satisfied_by(self, obj):
+    """Return `obj` if the object satisfies this type constraint, or raise.
 
     :raises: `TypeConstraintError` if `obj` does not satisfy the constraint.
     """
 
     if self.satisfied_by(obj):
       return obj
 
-    raise TypeConstraintError(
-      "value {!r} (with type {!r}) must satisfy this type constraint: {!r}."
-      .format(obj, type(obj).__name__, self))
+    raise self.make_type_constraint_error(obj, self)
 
   def __ne__(self, other):
     return not (self == other)
 
   def __str__(self):
-    return '{}{}'.format(self._variance_symbol, self._description)
+    return self._description
 
 
-class BasicTypeConstraint(TypeConstraint):
-  """A `TypeConstraint` predicated only on the object's type."""
+class TypeOnlyConstraint(TypeConstraint):
+  """A `TypeConstraint` predicated only on the object's type.
+
+  `TypeConstraint` subclasses may override `.satisfied_by()` to perform arbitrary validation on the
+  object itself -- however, this class implements `.satisfied_by()` with a guarantee that it will
+  only act on the object's `type` via `.satisfied_by_type()`. This kind of type checking is faster
+  and easier to understand than the more complex validation allowed by `.satisfied_by()`.
+  """
 
   # TODO: make an @abstract_classproperty decorator to do this boilerplate!
   @classproperty
@@ -328,12 +335,12 @@ def _variance_symbol(cls):
                               .format(cls.__name__))
 
   def __init__(self, *types):
-    """Creates a type constraint centered around the given types.
+    """Creates a type constraint based on some logic to match the given types.
 
-    The type constraint is satisfied as a whole if satisfied for at least one of the given types.
+    NB: A `TypeOnlyConstraint` implementation should ensure that the type constraint is satisfied as
+    a whole if satisfied for at least one of the given `types`.
 
-    :param type *types: The focus of this type constraint.
-    :param str description: A description for this constraint if the list of types is too long.
+    :param type *types: The types this constraint will match in some way.
     """
 
     if not types:
@@ -342,13 +349,12 @@ def __init__(self, *types):
       raise TypeError('Supplied types must be types. {!r}'.format(types))
 
     if len(types) == 1:
-      constrained_type = types[0].__name__
+      type_list = types[0].__name__
     else:
-      constrained_type = '({})'.format(', '.join(t.__name__ for t in types))
+      type_list = ' or '.join(t.__name__ for t in types)
+    description = '{}({})'.format(type(self).__name__, type_list)
 
-    super(BasicTypeConstraint, self).__init__(
-      variance_symbol=self._variance_symbol,
-      description=constrained_type)
+    super(TypeOnlyConstraint, self).__init__(description=description)
 
     # NB: This is made into a tuple so that we can use self._types in issubclass() and others!
     self._types = tuple(types)
@@ -382,20 +388,16 @@ def __repr__(self):
               constrained_type=constrained_type))
 
 
-class SuperclassesOf(BasicTypeConstraint):
+class SuperclassesOf(TypeOnlyConstraint):
   """Objects of the exact type as well as any super-types are allowed."""
 
-  _variance_symbol = '-'
-
   def satisfied_by_type(self, obj_type):
     return any(issubclass(t, obj_type) for t in self._types)
 
 
-class Exactly(BasicTypeConstraint):
+class Exactly(TypeOnlyConstraint):
   """Only objects of the exact type are allowed."""
 
-  _variance_symbol = '='
-
   def satisfied_by_type(self, obj_type):
     return obj_type in self._types
 
@@ -406,44 +408,59 @@ def graph_str(self):
       return repr(self)
 
 
-class SubclassesOf(BasicTypeConstraint):
+class SubclassesOf(TypeOnlyConstraint):
   """Objects of the exact type as well as any sub-types are allowed."""
 
-  _variance_symbol = '+'
-
   def satisfied_by_type(self, obj_type):
     return issubclass(obj_type, self._types)
 
 
 class TypedCollection(TypeConstraint):
-  """A `TypeConstraint` which accepts a BasicTypeConstraint and validates a collection."""
+  """A `TypeConstraint` which accepts a TypeOnlyConstraint and validates a collection."""
 
-  @classmethod
-  def _generate_variance_symbol(cls, constraint):
-    return '[{}]'.format(constraint._variance_symbol)
+  _iterable_constraint = SubclassesOf(Iterable)
 
   def __init__(self, constraint):
-    """Create a TypeConstraint which validates each member of a collection with `constraint`.
+    """Create a `TypeConstraint` which validates each member of a collection with `constraint`.
 
-    :param BasicTypeConstraint constraint: the TypeConstraint to apply to each element. This is
-                                           currently required to be a BasicTypeConstraint to avoid
-                                           complex prototypal type relationships.
+    :param TypeOnlyConstraint constraint: the `TypeConstraint` to apply to each element. This is
+                                          currently required to be a `TypeOnlyConstraint` to avoid
+                                          complex prototypal type relationships.
     """
 
-    if not isinstance(constraint, BasicTypeConstraint):
+    if not isinstance(constraint, TypeOnlyConstraint):
       raise TypeError("constraint for collection must be a {}! was: {}"
-                      .format(BasicTypeConstraint.__name__, constraint))
+                      .format(TypeOnlyConstraint.__name__, constraint))
+
+    description = '{}({})'.format(type(self).__name__, constraint)
+
     self._constraint = constraint
 
-    super(TypedCollection, self).__init__(
-      variance_symbol=self._generate_variance_symbol(constraint),
-      description=constraint._description)
+    super(TypedCollection, self).__init__(description=description)
 
+  # TODO: consider making this a private method of TypeConstraint, as it now duplicates the logic in
+  # self.validate_satisfied_by()!
   def satisfied_by(self, obj):
-    if isinstance(obj, Iterable):
+    if self._iterable_constraint.satisfied_by(obj):
       return all(self._constraint.satisfied_by(el) for el in obj)
     return False
 
+  def make_collection_type_constraint_error(self, base_obj, el):
+    base_error = self.make_type_constraint_error(el, self._constraint)
+    return TypeConstraintError("in wrapped constraint {!r} matching iterable object {!r}: {}"
+                               .format(self, base_obj, base_error))
+
+  def validate_satisfied_by(self, obj):
+    if self._iterable_constraint.satisfied_by(obj):
+      for el in obj:
+        if not self._constraint.satisfied_by(el):
+          raise self.make_collection_type_constraint_error(obj, el)
+      return obj
+
+    base_iterable_error = self.make_type_constraint_error(obj, self._iterable_constraint)
+    raise TypeConstraintError(
+      "in wrapped constraint {!r}: {}".format(self, base_iterable_error))
+
   def __hash__(self):
     return hash((type(self), self._constraint))
 

tests/python/pants_test/engine/test_objects.py

@@ -4,10 +4,28 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+import re
+
+from future.utils import PY3, text_type
+
 from pants.engine.objects import Collection
+from pants.util.objects import TypeCheckError
 from pants_test.test_base import TestBase
 
 
 class CollectionTest(TestBase):
   def test_collection_iteration(self):
     self.assertEqual([1, 2], [x for x in Collection.of(int)([1, 2])])
+
+  def test_element_typechecking(self):
+    IntColl = Collection.of(int)
+    with self.assertRaisesRegexp(TypeCheckError, re.escape("""\
+field 'dependencies' was invalid: in wrapped constraint TypedCollection(Exactly(int)) matching iterable object [3, {u}'hello']: value {u}'hello' (with type 'unicode') must satisfy this type constraint: Exactly(int)."""
+                                                           .format(u='' if PY3 else 'u'))):
+      IntColl([3, "hello"])
+
+    IntOrStringColl = Collection.of(int, text_type)
+    self.assertEqual([3, "hello"], [x for x in IntOrStringColl([3, "hello"])])
+    with self.assertRaisesRegexp(TypeCheckError, re.escape("""\
+field 'dependencies' was invalid: in wrapped constraint TypedCollection(Exactly(int, unicode)) matching iterable object [{}]: value {} (with type 'dict') must satisfy this type constraint: Exactly(int, unicode).""")):
+      IntOrStringColl([{}])