(core) Render unmarshallable values as non-errors, using their repr() strings.

Summary:
- Instead of sending an "UnmarshallableError" as an exception, introduce an
  "Unmarshallable" type of value, represented as ['U', repr(value)]
- Unmarshallable values are rendered using a bluish text color, no longer a
  pink background.
- Factor out ErrorDom to be simpler and cleaner.
- Add GristObjCode enum, and simplify related helpers.
- Use safe_repr() for when repr() itself fails
- Handle conversion errors using safe_repr() when str() fails

Test Plan: Added a test case based on a fixture covering a bunch of cases.

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D2584

app/common/ValueFormatter.ts

@@ -22,6 +22,7 @@ export function formatObject(args: [string, ...any[]]): string {
     case 'd': return moment.tz(objArgs[0] * 1000, 'UTC').format("YYYY-MM-DD");
     case 'R': return `${objArgs[0]}[${objArgs[1]}]`;
     case 'E': return gristTypes.formatError(args);
+    case 'U': return String(args[1]);
     case 'P': return PENDING_DATA_PLACEHOLDER;
   }
   return objType + "(" + JSON.stringify(objArgs).slice(1, -1) + ")";

app/common/gristTypes.ts

@@ -7,7 +7,15 @@ export type GristType = 'Any' | 'Attachments' | 'Blob' | 'Bool' | 'Choice' | 'Da
   'Id' | 'Int' | 'ManualSortPos' | 'Numeric' | 'PositionNumber' | 'Ref' | 'RefList' | 'Text';
 
 // Letter codes for CellValue types encoded as [code, args...] tuples.
-export type GristObjType = 'L' | 'D' | 'd' | 'R' | 'E' | 'P';
+export const enum GristObjCode {
+  List            = 'L',
+  DateTime        = 'D',
+  Date            = 'd',
+  Reference       = 'R',
+  Exception       = 'E',
+  Pending         = 'P',
+  Unmarshallable  = 'U',
+}
 
 export const MANUALSORT = 'manualSort';
 
@@ -50,11 +58,19 @@ export function isObject(value: CellValue): value is [string, any?] {
   return Array.isArray(value);
 }
 
+/**
+ * Returns GristObjCode of the value if the value is an object, or null otherwise.
+ * The return type includes any string, since we should not assume we can only get valid codes.
+ */
+export function getObjCode(value: CellValue): GristObjCode|string|null {
+  return Array.isArray(value) ? value[0] : null;
+}
+
 /**
  * Returns whether a value (as received in a DocAction) represents a raised exception.
  */
 export function isRaisedException(value: CellValue): boolean {
-  return Array.isArray(value) && value[0] === 'E';
+  return getObjCode(value) === GristObjCode.Exception;
 }
 
 /**
@@ -62,21 +78,14 @@ export function isRaisedException(value: CellValue): boolean {
  * which is a valid value for list types in grist.
  */
 export function isListOrNull(value: CellValue): boolean {
-  return value === null || (Array.isArray(value) && value[0] === 'L');
+  return value === null || (Array.isArray(value) && value[0] === GristObjCode.List);
 }
 
 /**
  * Returns whether a value (as received in a DocAction) represents an empty list.
  */
 export function isEmptyList(value: CellValue): boolean {
-  return Array.isArray(value) && value.length === 1 && value[0] === 'L';
+  return Array.isArray(value) && value.length === 1 && value[0] === GristObjCode.List;
-}
-
-/**
- * Returns whether a value (as received in a DocAction) represents a "Pending" value.
- */
-export function isPending(value: CellValue): boolean {
-  return Array.isArray(value) && value[0] === 'P';
 }
 
 /**
@@ -97,8 +106,11 @@ function isNumber(v: CellValue) { return typeof v === 'number' || typeof v === '
 function isNumberOrNull(v: CellValue) { return isNumber(v) || v === null; }
 function isBoolean(v: CellValue) { return typeof v === 'boolean' || v === 1 || v === 0; }
 
+// These values are not regular cell values, even in a column of type Any.
+const abnormalValueTypes: string[] = [GristObjCode.Exception, GristObjCode.Pending, GristObjCode.Unmarshallable];
+
 function isNormalValue(value: CellValue) {
-  return !(Array.isArray(value) && (value[0] === 'E' || value[0] === 'P'));
+  return !abnormalValueTypes.includes(getObjCode(value)!);
 }
 
 /**

sandbox/grist/objtypes.py

@@ -7,11 +7,11 @@ Non-primitive values are represented in actions as [type_name, args...].
   objtypes.encode_object(obj)   - returns a marshallable list representation.
   objtypes.decode_object(val)   - returns an object represented by the [name, args...] argument.
 
-If an object cannot be encoded or decoded, a RaisedError exception is encoded or returned instead.
+If an object cannot be encoded or decoded, an "UnmarshallableValue" is returned instead
-In a formula, this would cause an exception to be raised.
+of the form ['U', repr(obj)].
 """
-import marshal
 import exceptions
+import marshal
 import traceback
 from datetime import date, datetime
 
@@ -123,29 +123,35 @@ def deregister_converter(name):
   if prev:
     del _registered_converters_by_type[prev[0]]
 
+def safe_repr(obj):
+  """
+  Like repr(obj) but falls back to a simpler "<type-name>" string when repr() itself fails.
+  """
+  try:
+    return repr(obj)
+  except Exception:
+    return '<' + type(obj).__name__ + '>'
+
 
 def encode_object(obj):
   """
   Given an object, returns [typename, args...] array of marshallable values, which should be
   sufficient to reconstruct `obj`. Given a primitive object, returns it unchanged.
 
-  If obj failed to encode, yields an encoding for RaisedException(UnmarshallableError, message).
+  If obj failed to encode, yields an encoding for an UnmarshallableValue object, containing
-  I.e. on reading this back, and using the value, we'll get UnmarshallableError exception.
+  the repr(obj) string.
   """
   try:
     t = type(obj)
-    try:
     converter = (
         _registered_converters_by_type.get(t) or
         _registered_converters_by_type[getattr(t, '_objtypes_converter_type', t)])
-    except KeyError:
-      raise UnmarshallableError("No converter for type %s" % type(obj))
     return converter(obj)
 
   except Exception as e:
-    # Don't risk calling encode_object recursively; instead encode a RaisedException error
+    # We either don't know how to convert the value, or failed during the conversion. Instead we
-    # manually with arguments that ought not fail.
+    # return an "UnmarshallableValue" object, with repr() of the value to show to the user.
-    return ["E", "UnmarshallableError", str(e), repr(obj)]
+    return ['U', safe_repr(obj)]
 
 
 def decode_object(value):
@@ -189,10 +195,7 @@ class SelfConverter(object):
 
 def _encode_obj_impl(converter, name):
   def inner(obj):
-    try:
     args = converter.encode_args(obj)
-    except Exception:
-      raise UnmarshallableError("Encoding of %s failed" % name)
 
     for arg in args:
       check_marshallable(arg)
@@ -245,7 +248,11 @@ class RaisedException(object):
 
   @classmethod
   def decode_args(cls, *args):
-    return cls(decode_object(args))
+    # Decoding of a RaisedException is currently only used in tests.
+    name = args[0]
+    exc_type = getattr(exceptions, name)
+    assert isinstance(exc_type, type) and issubclass(exc_type, BaseException)
+    return cls(exc_type(*args[1:]))
 
   def __eq__(self, other):
     return isinstance(other, type(self)) and self.encode_args() == other.encode_args()
@@ -254,29 +261,6 @@ class RaisedException(object):
     return not self.__eq__(other)
 
 
-class ExceptionConverter(object):
-  """
-  Converter for any type derived from BaseException. On encoding it returns the exception object's
-  .args attribute, and uses them on decoding as constructor arguments to instantiate the error.
-  """
-  @classmethod
-  def encode_args(cls, obj):
-    return list(getattr(obj, 'args', ()))
-
-  @classmethod
-  def decode_args(cls, type_, args):
-    return type_(*args)
-
-
-# Register all Exceptions as valid types that can be handled by Grist.
-for _, my_type in exceptions.__dict__.iteritems():
-  if isinstance(my_type, type) and issubclass(my_type, BaseException):
-    register_converter(ExceptionConverter, my_type)
-
-# Register the special exceptions we defined.
-register_converter(ExceptionConverter, UnmarshallableError)
-register_converter(ExceptionConverter, ConversionError)
-
 # Register the special wrapper class for raised exceptions with a custom short name.
 register_converter(SelfConverter, RaisedException, "E")
 

sandbox/grist/usertypes.py

@@ -171,10 +171,15 @@ class BaseColumnType(object):
       return self.do_convert(value_to_convert)
     except Exception as e:
       # If conversion failed, return a string to serve as alttext.
+      try:
         if isinstance(value_to_convert, six.text_type):
           # str() will fail for a non-ascii unicode object, which needs an explicit encoding.
           return value_to_convert.encode('utf8')
         return str(value_to_convert)
+      except Exception:
+        # If converting to string failed, we should still produce something.
+        return objtypes.safe_repr(value_to_convert)
+
 
   # This is a user-facing method, hence the camel-case naming, as for `lookupRecords` and such.
   @classmethod