From 0e5c2bee59edbff517a19d9c986df9f18d70827d Mon Sep 17 00:00:00 2001
From: Alex Hall <alex.mojaki@gmail.com>
Date: Fri, 30 Jul 2021 20:51:56 +0200
Subject: [PATCH] (core) Add CellError to explain when a formula refers to a
 cell with an error value

Summary:
get_cell_value wraps RaisedException with CellError to expand the error message for the user.

This is still pretty conceptual, the comments explain some things to think about, but it works and is an improvement.

Test Plan: Updated Python unit tests

Reviewers: dsagal, paulfitz

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D2928
---
 sandbox/grist/column.py             |  7 +++++-
 sandbox/grist/functions/info.py     |  4 ++++
 sandbox/grist/objtypes.py           | 33 +++++++++++++++++++++++++----
 sandbox/grist/test_formula_error.py | 15 ++++++++++---
 4 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py
index 8ad31ac5..c3e477cd 100644
--- a/sandbox/grist/column.py
+++ b/sandbox/grist/column.py
@@ -147,7 +147,12 @@ class BaseColumn(object):
     """
     raw = self.raw_get(row_id)
     if isinstance(raw, objtypes.RaisedException):
-      raise raw.error
+      if isinstance(raw.error, depend.CircularRefError):
+        # Wrapping a CircularRefError in a CellError is often redundant, but
+        # TODO a CellError is still useful if the calling cell is not involved in the cycle
+        raise raw.error
+      else:
+        raise objtypes.CellError(self.table_id, self.col_id, row_id, raw.error)
     if self.type_obj.is_right_type(raw):
       return self._make_rich_value(raw)
     return usertypes.AltText(str(raw), self.type_obj.typename())
diff --git a/sandbox/grist/functions/info.py b/sandbox/grist/functions/info.py
index 7b605ec7..ae8883ad 100644
--- a/sandbox/grist/functions/info.py
+++ b/sandbox/grist/functions/info.py
@@ -12,6 +12,7 @@ import six
 import column
 from functions import date      # pylint: disable=import-error
 from functions.unimplemented import unimplemented
+from objtypes import CellError
 from usertypes import AltText   # pylint: disable=import-error
 from records import Record, RecordSet
 
@@ -597,6 +598,9 @@ def _prepare_record_dict(record, dates_as_iso=False, expand_refs=0):
       result[col_id] = val
     except Exception as e:
       result[col_id] = None
+      while isinstance(e, CellError):
+        # The extra information from CellError is redundant here
+        e = e.error  # pylint: disable=no-member
       errors[col_id] = "%s: %s" % (type(e).__name__, str(e))
 
   if errors:
diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py
index fbda33c8..569fb0c3 100644
--- a/sandbox/grist/objtypes.py
+++ b/sandbox/grist/objtypes.py
@@ -263,11 +263,18 @@ class RaisedException(object):
     # and to DB (for when we store formula results). There are two concerns: one is that it's
     # potentially quite verbose; the other is that it's makes the tests more annoying (again b/c
     # verbose).
+    error = self.error
+    location = ""
+    while isinstance(error, CellError):
+      if not location:
+        location = "\n(in referenced cell {error.location})".format(error=error)
+      error = error.error
+    name = type(error).__name__
     if self.details:
-      return [type(self.error).__name__, str(self.error), self.details]
-    if isinstance(self.error, InvalidTypedValue):
-      return [type(self.error).__name__, self.error.typename, self.error.value]
-    return [type(self.error).__name__]
+      return [name, str(error) + location, self.details]
+    if isinstance(error, InvalidTypedValue):
+      return [name, error.typename, error.value]
+    return [name]
 
   @classmethod
   def decode_args(cls, *args):
@@ -275,6 +282,24 @@ class RaisedException(object):
     return cls(None, encoded_error=list(args))
 
 
+class CellError(Exception):
+  def __init__(self, table_id, col_id, row_id, error):
+    super(CellError, self).__init__(table_id, col_id, row_id, error)
+    self.table_id = table_id
+    self.col_id = col_id
+    self.row_id = row_id
+    self.error = error
+
+  def __str__(self):
+    return (
+      "{self.error.__class__.__name__} in referenced cell {self.location}"
+    ).format(self=self)
+
+  @property
+  def location(self):
+    return "{self.table_id}[{self.row_id}].{self.col_id}".format(self=self)
+
+
 class RecordList(list):
   """
   Just like list but allows setting custom attributes, which we use for remembering _group_by and
diff --git a/sandbox/grist/test_formula_error.py b/sandbox/grist/test_formula_error.py
index 791568f7..3116eb3b 100644
--- a/sandbox/grist/test_formula_error.py
+++ b/sandbox/grist/test_formula_error.py
@@ -330,9 +330,18 @@ else:
     self.assertFormulaError(self.engine.get_formula_error('AttrTest', 'B', 1),
                             AttributeError, "Table 'AttrTest' has no column 'AA'",
                             r"AttributeError: Table 'AttrTest' has no column 'AA'")
-    self.assertFormulaError(self.engine.get_formula_error('AttrTest', 'C', 1),
-                            AttributeError, "Table 'AttrTest' has no column 'AA'",
-                            r"AttributeError: Table 'AttrTest' has no column 'AA'")
+    cell_error = self.engine.get_formula_error('AttrTest', 'C', 1)
+    self.assertFormulaError(cell_error,
+                            objtypes.CellError, "AttributeError in referenced cell AttrTest[1].B",
+                            r"CellError: AttributeError in referenced cell AttrTest\[1\].B")
+    self.assertEqual(
+      objtypes.encode_object(cell_error),
+      ['E',
+       'AttributeError',
+       "Table 'AttrTest' has no column 'AA'\n"
+       "(in referenced cell AttrTest[1].B)",
+       cell_error.details]
+    )
 
   def test_cumulative_formula(self):
     formula = ("Table1.lookupOne(A=$A-1).Principal + Table1.lookupOne(A=$A-1).Interest " +