add stack traces to unhashable TypeErrors in the engine (#7532)

### Problem

- When we provide an unhashable type such as `list` to a `datatype` field of an object that goes through the v2 engine, the engine will tell you there is a TypeError, but won't tell you which object it's from.
- The unhashable type error happens late (long after the datatype is constructed) and without an appropriate stacktrace. It should be easy to declare hashable collection fields (in this case, just tuples) and have them fail early (at construction).

### Solution

- Add the name of the type of the object passed to `extern_identify()` to the `TypeError` raised if one of its fields are unhashable.
- Make a `HashableTypedCollection` type constructor.
- Capture exceptions raised in CFFI extern methods using the `onerror` handler and re-raise those with their tracebacks so the `TypeError`s are actually catchable.

### Result

Unhashable type errors for datatypes going through the engine happen less, and errors happen earlier and with more context!

src/python/pants/base/specs.py

@@ -224,6 +224,9 @@ def matches_target_address_pair(self, address, target):
     return self._target_tag_matches(target) and not self._excluded_by_pattern(address)
 
 
+# TODO: we can't yet express a type in terms of itself, because datatype field types are evaluated
+# eagerly (so we can't yet type-check the individual items of a tuple provided as the `dependencies`
+# field).
 class Specs(datatype([('dependencies', tuple), ('matcher', SpecsMatcher)])):
   """A collection of Specs representing Spec subclasses, and a SpecsMatcher to filter results."""
 

src/python/pants/engine/isolated_process.py

@@ -10,7 +10,7 @@
 
 from pants.engine.fs import Digest
 from pants.engine.rules import RootRule, rule
-from pants.util.objects import Exactly, datatype, string_list, string_optional, string_type
+from pants.util.objects import Exactly, datatype, hashable_string_list, string_optional, string_type
 
 
 logger = logging.getLogger(__name__)
@@ -19,12 +19,12 @@
 
 
 class ExecuteProcessRequest(datatype([
-  ('argv', string_list),
+  ('argv', hashable_string_list),
   ('input_files', Digest),
   ('description', string_type),
-  ('env', string_list),
-  ('output_files', string_list),
-  ('output_directories', string_list),
+  ('env', hashable_string_list),
+  ('output_files', hashable_string_list),
+  ('output_directories', hashable_string_list),
   # NB: timeout_seconds covers the whole remote operation including queuing and setup.
   ('timeout_seconds', Exactly(float, int)),
   ('jdk_home', string_optional),

src/python/pants/engine/native.py

@@ -24,7 +24,7 @@
 from pants.util.dirutil import read_file, safe_mkdir, safe_mkdtemp
 from pants.util.memo import memoized_classproperty, memoized_property
 from pants.util.meta import Singleton
-from pants.util.objects import datatype
+from pants.util.objects import SubclassesOf, datatype
 
 
 logger = logging.getLogger(__name__)
@@ -250,11 +250,22 @@ def format_cffi_externs(cls):
       + '\n'.join(extern_decls)
       + '\n}\n')
 
-  def register_cffi_externs(self):
-    """Registers the @_extern_decl methods with our ffi instance."""
+  def register_cffi_externs(self, native):
+    """Registers the @_extern_decl methods with our ffi instance.
+
+    Also establishes an `onerror` handler for each extern method which stores any exception in the
+    `native` object so that it can be retrieved later. See
+    https://cffi.readthedocs.io/en/latest/using.html#extern-python-reference for more info.
+    """
+    native.reset_cffi_extern_method_runtime_exceptions()
+
+    def exc_handler(exc_type, exc_value, traceback):
+      error_info = native.CFFIExternMethodRuntimeErrorInfo(exc_type, exc_value, traceback)
+      native.add_cffi_extern_method_runtime_exception(error_info)
+
     for field_name, _ in self._extern_fields.items():
       bound_method = getattr(self, field_name)
-      self._ffi.def_extern()(bound_method)
+      self._ffi.def_extern(onerror=exc_handler)(bound_method)
 
   def to_py_str(self, msg_ptr, msg_len):
     return bytes(self._ffi.buffer(msg_ptr, msg_len)).decode('utf-8')
@@ -562,6 +573,36 @@ def from_key(self, key):
 class Native(Singleton):
   """Encapsulates fetching a platform specific version of the native portion of the engine."""
 
+  _errors_during_execution = None
+
+  class CFFIExternMethodRuntimeErrorInfo(datatype([
+      ('exc_type', type),
+      ('exc_value', SubclassesOf(Exception)),
+      'traceback',
+  ])):
+    """Encapsulates an exception raised when a CFFI extern is called so that it can be displayed.
+
+    When an exception is raised in the body of a CFFI extern, the `onerror` handler is used to
+    capture it, storing the exception info as an instance of `CFFIExternMethodRuntimeErrorInfo` with
+    `.add_cffi_extern_method_runtime_exception()`. The scheduler will then check whether any
+    exceptions were stored by calling `.cffi_extern_method_runtime_exceptions()` after specific
+    calls to the native library which may raise. `.reset_cffi_extern_method_runtime_exceptions()`
+    should be called after the stored exception has been handled or before it is re-raised.
+
+    Some ways that exceptions in CFFI extern methods can be handled are described in
+    https://cffi.readthedocs.io/en/latest/using.html#extern-python-reference.
+    """
+
+  def reset_cffi_extern_method_runtime_exceptions(self):
+    self._errors_during_execution = []
+
+  def cffi_extern_method_runtime_exceptions(self):
+    return self._errors_during_execution
+
+  def add_cffi_extern_method_runtime_exception(self, error_info):
+    assert isinstance(error_info, self.CFFIExternMethodRuntimeErrorInfo)
+    self._errors_during_execution.append(error_info)
+
   class BinaryLocationError(Exception): pass
 
   @memoized_property
@@ -588,7 +629,7 @@ def binary(self):
   def lib(self):
     """Load and return the native engine module."""
     lib = self.ffi.dlopen(self.binary)
-    _FFISpecification(self.ffi, lib).register_cffi_externs()
+    _FFISpecification(self.ffi, lib).register_cffi_externs(self)
     return lib
 
   @memoized_property

src/python/pants/engine/scheduler.py

@@ -7,8 +7,11 @@
 import logging
 import multiprocessing
 import os
+import sys
 import time
+import traceback
 from builtins import object, open, str, zip
+from textwrap import dedent
 from types import GeneratorType
 
 from pants.base.exiter import PANTS_FAILED_EXIT_CODE
@@ -506,7 +509,56 @@ def product_request(self, product, subjects):
     :param list subjects: A list of subjects or Params instances for the request.
     :returns: A list of the requested products, with length match len(subjects).
     """
-    request = self.execution_request([product], subjects)
+    request = None
+    raised_exception = None
+    try:
+      request = self.execution_request([product], subjects)
+    except:                     # noqa: T803
+      # If there are any exceptions during CFFI extern method calls, we want to return an error with
+      # them and whatever failure results from it. This typically results from unhashable types.
+      if self._scheduler._native.cffi_extern_method_runtime_exceptions():
+        raised_exception = sys.exc_info()[0:3]
+      else:
+        # Otherwise, this is likely an exception coming from somewhere else, and we don't want to
+        # swallow that, so re-raise.
+        raise
+
+    # We still want to raise whenever there are any exceptions in any CFFI extern methods, even if
+    # that didn't lead to an exception in generating the execution request for some reason, so we
+    # check the extern exceptions list again.
+    internal_errors = self._scheduler._native.cffi_extern_method_runtime_exceptions()
+    if internal_errors:
+      error_tracebacks = [
+        ''.join(
+          traceback.format_exception(etype=error_info.exc_type,
+                                     value=error_info.exc_value,
+                                     tb=error_info.traceback))
+        for error_info in internal_errors
+      ]
+
+      raised_exception_message = None
+      if raised_exception:
+        exc_type, exc_value, tb = raised_exception
+        raised_exception_message = dedent("""\
+          The engine execution request raised this error, which is probably due to the errors in the
+          CFFI extern methods listed above, as CFFI externs return None upon error:
+          {}
+        """).format(''.join(traceback.format_exception(etype=exc_type, value=exc_value, tb=tb)))
+
+      # Zero out the errors raised in CFFI callbacks in case this one is caught and pants doesn't
+      # exit.
+      self._scheduler._native.reset_cffi_extern_method_runtime_exceptions()
+
+      raise ExecutionError(dedent("""\
+        {error_description} raised in CFFI extern methods:
+        {joined_tracebacks}{raised_exception_message}
+        """).format(
+          error_description=pluralize(len(internal_errors), 'Exception'),
+          joined_tracebacks='\n+++++++++\n'.join(formatted_tb for formatted_tb in error_tracebacks),
+          raised_exception_message=(
+            '\n\n{}'.format(raised_exception_message) if raised_exception_message else '')
+        ))
+
     returns, throws = self.execute(request)
 
     # Throw handling.

src/python/pants/util/objects.py

@@ -14,6 +14,7 @@
 from pants.util.collections_abc_backport import Iterable, OrderedDict, namedtuple
 from pants.util.memo import memoized_classproperty
 from pants.util.meta import AbstractClass, classproperty
+from pants.util.strutil import pluralize
 
 
 class TypeCheckError(TypeError):
@@ -122,8 +123,9 @@ def __new__(cls, *args, **kwargs):
             "field '{}' was invalid: {}".format(field_name, e))
       if type_failure_msgs:
         raise cls.make_type_error(
-          'error(s) type checking constructor arguments:\n{}'
-          .format('\n'.join(type_failure_msgs)))
+          '{} type checking constructor arguments:\n{}'
+          .format(pluralize(len(type_failure_msgs), 'error'),
+                  '\n'.join(type_failure_msgs)))
 
       return this_object
 
@@ -147,7 +149,21 @@ def __ne__(self, other):
     # explicitly implemented, otherwise Python will raise "unhashable type". See
     # https://docs.python.org/3/reference/datamodel.html#object.__hash__.
     def __hash__(self):
-      return super(DataType, self).__hash__()
+      try:
+        return super(DataType, self).__hash__()
+      except TypeError:
+        # If any fields are unhashable, we want to be able to specify which ones in the error
+        # message, but we don't want to slow down the normal __hash__ code path, so we take the time
+        # to break it down by field if we know the __hash__ fails for some reason.
+        for field_name, value in self._asdict().items():
+          try:
+            hash(value)
+          except TypeError as e:
+            raise TypeError("For datatype object {} (type '{}'): in field '{}': {}"
+                            .format(self, type(self).__name__, field_name, e))
+        # If the error doesn't seem to be with hashing any of the fields, just re-raise the
+        # original error.
+        raise
 
     # NB: As datatype is not iterable, we need to override both __iter__ and all of the
     # namedtuple methods that expect self to be iterable.
@@ -524,10 +540,25 @@ def satisfied_by_type(self, obj_type):
 class TypedCollection(TypeConstraint):
   """A `TypeConstraint` which accepts a TypeOnlyConstraint and validates a collection."""
 
-  _iterable_constraint = SubclassesOf(Iterable)
-  # strings in Python are considered iterables of substrings, but we only want to allow explicit
-  # collection types.
-  _exclude_iterable_constraint = _string_type_constraint
+  @memoized_classproperty
+  def iterable_constraint(cls):
+    """Define what kind of collection inputs are accepted by this type constraint.
+
+    :rtype: TypeConstraint
+    """
+    return SubclassesOf(Iterable)
+
+  # TODO: extend TypeConstraint to specify includes and excludes in a single constraint!
+  @classproperty
+  def exclude_iterable_constraint(cls):
+    """Define what collection inputs are *not* accepted by this type constraint.
+
+    Strings in Python are considered iterables of substrings, but we only want to allow explicit
+    collection types.
+
+    :rtype: TypeConstraint
+    """
+    return _string_type_constraint
 
   def __init__(self, constraint):
     """Create a `TypeConstraint` which validates each member of a collection with `constraint`.
@@ -548,8 +579,8 @@ def __init__(self, constraint):
     super(TypedCollection, self).__init__(description=description)
 
   def _is_iterable(self, obj):
-    return (self._iterable_constraint.satisfied_by(obj)
-            and not self._exclude_iterable_constraint.satisfied_by(obj))
+    return (self.iterable_constraint.satisfied_by(obj)
+            and not self.exclude_iterable_constraint.satisfied_by(obj))
 
   # TODO: consider making this a private method of TypeConstraint, as it now duplicates the logic in
   # self.validate_satisfied_by()!
@@ -564,10 +595,10 @@ def make_collection_type_constraint_error(self, base_obj, el):
 
   def validate_satisfied_by(self, obj):
     if not self._is_iterable(obj):
-      base_iterable_error = self.make_type_constraint_error(obj, self._iterable_constraint)
+      base_iterable_error = self.make_type_constraint_error(obj, self.iterable_constraint)
       raise TypeConstraintError(
         "in wrapped constraint {}: {}\nNote that objects matching {} are not considered iterable."
-        .format(self, base_iterable_error, self._exclude_iterable_constraint))
+        .format(self, base_iterable_error, self.exclude_iterable_constraint))
     for el in obj:
       if not self._constraint.satisfied_by(el):
         raise self.make_collection_type_constraint_error(obj, el)
@@ -586,6 +617,16 @@ def __repr__(self):
 
 
 # TODO(#6742): Useful type constraints for datatype fields before we start using mypy type hints!
+hashable_collection_constraint = Exactly(tuple)
+
+
+class HashableTypedCollection(TypedCollection):
+  iterable_constraint = hashable_collection_constraint
+
+
 string_type = Exactly(text_type)
 string_list = TypedCollection(string_type)
 string_optional = Exactly(text_type, type(None))
+
+
+hashable_string_list = HashableTypedCollection(string_type)

tests/python/pants_test/engine/BUILD

@@ -203,6 +203,7 @@ python_tests(
   coverage=['pants.engine.nodes', 'pants.engine.scheduler'],
   dependencies=[
     '3rdparty/python:future',
+    '3rdparty/python:mock',
     ':util',
     'src/python/pants/base:cmd_line_spec_parser',
     'src/python/pants/build_graph',