(core) Error explanations from friendly-traceback

Summary: Extend formula error messages with explanations from https://github.com/friendly-traceback/friendly-traceback. Only for Python 3.

Test Plan: Updated several Python tests. In general, these require separate branches for Python 2 and 3.

Reviewers: georgegevoian

Reviewed By: georgegevoian

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

app/server/lib/NSandbox.ts

@@ -5,15 +5,20 @@ import {arrayToString} from 'app/common/arrayToString';
 import * as marshal from 'app/common/marshal';
 import {ISandbox, ISandboxCreationOptions, ISandboxCreator} from 'app/server/lib/ISandbox';
 import log from 'app/server/lib/log';
-import {DirectProcessControl, ISandboxControl, NoProcessControl, ProcessInfo,
-        SubprocessControl} from 'app/server/lib/SandboxControl';
+import {
+  DirectProcessControl,
+  ISandboxControl,
+  NoProcessControl,
+  ProcessInfo,
+  SubprocessControl
+} from 'app/server/lib/SandboxControl';
 import * as sandboxUtil from 'app/server/lib/sandboxUtil';
 import * as shutdown from 'app/server/lib/shutdown';
 import {ChildProcess, spawn} from 'child_process';
+import * as fs from 'fs';
+import * as _ from 'lodash';
 import * as path from 'path';
 import {Stream, Writable} from 'stream';
-import * as _ from 'lodash';
-import * as fs from 'fs';
 import * as which from 'which';
 
 type SandboxMethod = (...args: any[]) => any;
@@ -570,15 +575,9 @@ function gvisor(options: ISandboxOptions): SandboxProcess {
   // Check for local virtual environments created with core's
   // install:python2 or install:python3 targets. They'll need
   // some extra sharing to make available in the sandbox.
-  // This appears to currently be incompatible with checkpoints?
-  // Shares and checkpoints interact delicately because the file
-  // handle layout/ordering needs to remain exactly the same.
-  // Fixable no doubt, but for now I just disable this convenience
-  // if checkpoints are in use.
   const venv = path.join(process.cwd(),
                          pythonVersion === '2' ? 'venv' : 'sandbox_venv3');
-  const useCheckpoint = process.env.GRIST_CHECKPOINT && !paths.importDir;
-  if (fs.existsSync(venv) && !useCheckpoint) {
+  if (fs.existsSync(venv)) {
     wrapperArgs.addMount(venv);
     wrapperArgs.push('-s', path.join(venv, 'bin', 'python'));
   }
@@ -589,7 +588,7 @@ function gvisor(options: ISandboxOptions): SandboxProcess {
   // between the checkpoint and how it gets used later).
   // If a sandbox is being used for import, it will have a special mount we can't
   // deal with easily right now. Should be possible to do in future if desired.
-  if (options.useGristEntrypoint && pythonVersion === '3' && useCheckpoint) {
+  if (options.useGristEntrypoint && pythonVersion === '3' && process.env.GRIST_CHECKPOINT && !paths.importDir) {
     if (process.env.GRIST_CHECKPOINT_MAKE) {
       const child =
         spawn(command, [...wrapperArgs.get(), '--checkpoint', process.env.GRIST_CHECKPOINT!,

sandbox/grist/codebuilder.py

@@ -1,11 +1,14 @@
 import ast
 import contextlib
 import itertools
+import linecache
 import re
 import six
 
 import astroid
 import asttokens
+
+import friendly_errors
 import textbuilder
 import logger
 log = logger.Logger(__name__, logger.INFO)
@@ -22,6 +25,13 @@ LAZY_ARG_FUNCTIONS = {
   'PEEK': slice(0, 1),
 }
 
+
+class GristSyntaxError(SyntaxError):
+  """
+  Indicates a formula is invalid in a Grist-specific way.
+  """
+
+
 def make_formula_body(formula, default_value, assoc_value=None):
   """
   Given a formula, returns a textbuilder.Builder object suitable to be the body of a function,
@@ -46,7 +56,7 @@ def make_formula_body(formula, default_value, assoc_value=None):
 
   # Parse the formula into an abstract syntax tree (AST), catching syntax errors.
   try:
-    atok = asttokens.ASTTokens(tmp_formula.get_text(), parse=True)
+    atok = asttokens.ASTTokens(tmp_formula.get_text(), parse=True, filename=code_filename)
   except SyntaxError as e:
     return textbuilder.Text(_create_syntax_error_code(tmp_formula, formula, e))
 
@@ -91,8 +101,7 @@ def make_formula_body(formula, default_value, assoc_value=None):
     message = "No `return` statement, and the last line isn't an expression."
     if isinstance(last_statement, ast.Assign):
       message += " If you want to check for equality, use `==` instead of `=`."
-    error = SyntaxError(message,
-                        ('<string>', 1, 1, ""))
+    error = GristSyntaxError(message, ('<string>', 1, 1, ""))
     return textbuilder.Text(_create_syntax_error_code(tmp_formula, formula, error))
 
   # Apply the new set of patches to the original formula to get the real output.
@@ -129,11 +138,24 @@ def _create_syntax_error_code(builder, input_text, err):
   output_offset = output_ln.line_to_offset(err.lineno, err.offset - 1 if err.offset else 0)
   input_offset = builder.map_back_offset(output_offset)
   line, col = input_ln.offset_to_line(input_offset)
-  message = err.args[0]
   input_text_line = input_text.splitlines()[line - 1]
+
+  message = err.args[0]
+  err_type = type(err)
+  if isinstance(err, GristSyntaxError):
+    # Just use SyntaxError in the final code
+    err_type = SyntaxError
+  elif six.PY3:
+    # Add explanation from friendly-traceback.
+    # Only supported in Python 3.
+    # Not helpful for Grist-specific errors.
+    # Needs to use the source code, so save it to its source cache.
+    save_to_linecache(builder.get_text())
+    message += friendly_errors.friendly_message(err)
+
   return "%s\nraise %s(%r, ('usercode', %r, %r, %r))" % (
     textbuilder.line_start_re.sub('# ', input_text.rstrip()),
-    type(err).__name__, message, line, col + 1, input_text_line)
+    err_type.__name__, message, line, col + 1, input_text_line)
 
 #----------------------------------------------------------------------
 
@@ -299,7 +321,7 @@ class InferRecAssignment(InferenceTip):
   @classmethod
   def filter(cls, node):
     if node.name == 'rec':
-      raise SyntaxError('Grist disallows assignment to the special variable "rec"',
+      raise GristSyntaxError('Grist disallows assignment to the special variable "rec"',
           ('<string>', node.lineno, node.col_offset, ""))
 
   @classmethod
@@ -315,7 +337,7 @@ class InferRecAttrAssignment(InferenceTip):
   @classmethod
   def filter(cls, node):
     if isinstance(node.expr, astroid.nodes.Name) and node.expr.name == 'rec':
-      raise SyntaxError("You can't assign a value to a column with `=`. "
+      raise GristSyntaxError("You can't assign a value to a column with `=`. "
                              "If you mean to check for equality, use `==` instead.",
           ('<string>', node.lineno, node.col_offset, ""))
 
@@ -379,3 +401,23 @@ def parse_grist_names(builder):
           parsed_names.append(make_tuple(start, end, obj.name, node.arg))
 
   return [name for name in parsed_names if name]
+
+
+code_filename = "usercode"
+
+
+def save_to_linecache(source_code):
+  """
+  Makes source code available to friendly-traceback and traceback formatting in general.
+  """
+  if six.PY3:
+    import friendly_traceback.source_cache
+
+    friendly_traceback.source_cache.cache.add(code_filename, source_code)
+  else:
+    linecache.cache[code_filename] = (
+      len(source_code),
+      None,
+      [line + '\n' for line in source_code.splitlines()],
+      code_filename,
+    )

sandbox/grist/friendly_errors.py

@@ -0,0 +1,43 @@
+def friendly_message(exc):
+  """
+  Returns a string to append to a standard error message.
+  If possible, the string contains a friendly explanation of the error.
+  Otherwise, the string is empty.
+  """
+  try:
+    if "has no column" in str(exc):
+      # Avoid the standard AttributeError explanation
+      return ""
+
+    # Imported locally because it's Python 3 only
+    from friendly_traceback.core import FriendlyTraceback
+
+    fr = FriendlyTraceback(type(exc), exc, exc.__traceback__)
+    fr.assign_generic()
+    fr.assign_cause()
+
+    generic = fr.info["generic"]  # broad explanation for the exception class
+    cause = fr.info.get("cause")  # more specific explanation
+
+    if "https://github.com" in generic:
+      # This is a placeholder message when there is no explanation,
+      # with a suggestion to report the case on GitHub.
+      return ""
+
+    # Add a blank line between the standard message and the friendly message
+    result = "\n\n" + generic
+
+    # Check for the placeholder message again in the cause
+    if cause and "https://github.com" not in cause:
+      result += "\n" + cause
+
+    result = result.rstrip()
+    if isinstance(exc, SyntaxError):
+      result += "\n\n"
+
+    return result
+  except (Exception, SystemExit):
+    # This can go wrong in many ways, it's not worth propagating the error.
+    # friendly-traceback raises SystemExit when it encounters an internal error.
+    # Note that SystemExit is not a subclass of Exception.
+    return ""

sandbox/grist/gencode.py

@@ -208,18 +208,9 @@ def _is_special_table(table_id):
 
 
 def exec_module_text(module_text):
+  mod = imp.new_module(codebuilder.code_filename)
+  codebuilder.save_to_linecache(module_text)
+  code_obj = compile(module_text, codebuilder.code_filename, "exec")
   # pylint: disable=exec-used
-  filename = "usercode"
-  mod = imp.new_module(filename)
-
-  # Ensure that source lines show in tracebacks
-  linecache.cache[filename] = (
-    len(module_text),
-    None,
-    [line + '\n' for line in module_text.splitlines()],
-    filename,
-  )
-
-  code_obj = compile(module_text, filename, "exec")
   exec(code_obj, mod.__dict__)
   return mod

sandbox/grist/objtypes.py

@@ -15,9 +15,12 @@ import traceback
 from datetime import date, datetime
 from math import isnan
 
+import six
+
+import friendly_errors
 import moment
 import records
-import six
+import depend
 
 
 class UnmarshallableError(ValueError):
@@ -306,6 +309,10 @@ class RaisedException(object):
     if include_details:
       self.details = traceback.format_exc()
       self._message = str(error) + location
+      if not (isinstance(error, (SyntaxError, depend.CircularRefError)) or error != self.error):
+        # For SyntaxError, the friendly message was already added earlier.
+        # CircularRefError and CellError are Grist-specific and have no friendly message.
+        self._message += friendly_errors.friendly_message(error)
     elif isinstance(error, InvalidTypedValue):
       self._message = error.typename
       self.details = error.value

sandbox/grist/test_codebuilder.py

@@ -72,22 +72,44 @@ class TestCodeBuilder(unittest.TestCase):
                      "'''test1'''\nreturn \"\"\"test2\"\"\"")
 
     # Test that we produce valid code when "$foo" occurs in invalid places.
+    if six.PY2:
+      raise_code = "raise SyntaxError('invalid syntax', ('usercode', 1, 5, u'foo($bar=1)'))"
+    else:
+      raise_code = ("raise SyntaxError('invalid syntax\\n\\n"
+                    "A `SyntaxError` occurs when Python cannot understand your code.\\n\\n', "
+                    "('usercode', 1, 5, 'foo($bar=1)'))")
     self.assertEqual(make_body('foo($bar=1)'),
-                     "# foo($bar=1)\n"
-                     "raise SyntaxError('invalid syntax', ('usercode', 1, 5, %s'foo($bar=1)'))"
-                     % unicode_prefix)
+                     "# foo($bar=1)\n" + raise_code)
+
+    if six.PY2:
+      raise_code = ("raise SyntaxError('invalid syntax', "
+                    "('usercode', 1, 5, u'def $bar(): return 3'))")
+    else:
+      raise_code = ("raise SyntaxError('invalid syntax\\n\\n"
+                    "A `SyntaxError` occurs when Python cannot understand your code.\\n\\n', "
+                    "('usercode', 1, 5, 'def $bar(): return 3'))")
     self.assertEqual(make_body('def $bar(): return 3'),
-                     "# def $bar(): return 3\n"
-                     "raise SyntaxError('invalid syntax', "
-                     "('usercode', 1, 5, %s'def $bar(): return 3'))"
-                     % unicode_prefix)
+                     "# def $bar(): return 3\n" + raise_code)
 
     # If $ is a syntax error, we don't want to turn it into a different syntax error.
+    if six.PY2:
+      raise_code = ("raise SyntaxError('invalid syntax', "
+                    "('usercode', 1, 17, u'$foo + (\"$%.2f\" $ ($17.5))'))")
+    else:
+      raise_code = ("raise SyntaxError('invalid syntax\\n\\n"
+                    "A `SyntaxError` occurs when Python cannot understand your code.\\n\\n', "
+                    "('usercode', 1, 17, '$foo + (\"$%.2f\" $ ($17.5))'))")
     self.assertEqual(make_body('$foo + ("$%.2f" $ ($17.5))'),
-                     '# $foo + ("$%.2f" $ ($17.5))\n'
-                     "raise SyntaxError('invalid syntax', "
-                     "('usercode', 1, 17, {}'$foo + (\"$%.2f\" $ ($17.5))'))"
-                     .format(unicode_prefix))
+                     '# $foo + ("$%.2f" $ ($17.5))\n' + raise_code)
+
+    if six.PY2:
+      raise_code = "raise SyntaxError('invalid syntax', ('usercode', 4, 10, u'  return $ bar'))"
+    else:
+      raise_code = ("raise SyntaxError('invalid syntax\\n\\n"
+                    "A `SyntaxError` occurs when Python cannot understand your code.\\n\\n"
+                    "I am guessing that you wrote `$` by mistake.\\n"
+                    "Removing it and writing `return  bar` seems to fix the error.\\n\\n', "
+                    "('usercode', 4, 10, '  return $ bar'))")
     self.assertEqual(make_body('if $foo:\n' +
                                '  return $foo\n' +
                                'else:\n' +
@@ -96,8 +118,7 @@ class TestCodeBuilder(unittest.TestCase):
                      '#   return $foo\n' +
                      '# else:\n' +
                      '#   return $ bar\n' +
-                     "raise SyntaxError('invalid syntax', ('usercode', 4, 10, %s'  return $ bar'))"
-                     % unicode_prefix)
+                     raise_code)
 
     # Check for reasonable behaviour with non-empty text and no statements.
     self.assertEqual(make_body('# comment'), '# comment\npass')

sandbox/grist/test_engine.py

@@ -270,7 +270,7 @@ class EngineTestCase(unittest.TestCase):
   def assertFormulaError(self, exc, type_, message, tracebackRegexp=None):
     self.assertIsInstance(exc, objtypes.RaisedException)
     self.assertIsInstance(exc.error, type_)
-    self.assertEqual(str(exc.error), message)
+    self.assertEqual(exc._message, message)
     if tracebackRegexp:
       self.assertRegex(exc.details, tracebackRegexp)
 

sandbox/grist/test_formula_error.py

@@ -54,12 +54,33 @@ else:
                               TypeError, 'SQRT() takes exactly 1 argument (2 given)',
                               r"TypeError: SQRT\(\) takes exactly 1 argument \(2 given\)")
     else:
-      self.assertFormulaError(self.engine.get_formula_error('Math', 'excel_formula', 3),
-                              TypeError, 'SQRT() takes 1 positional argument but 2 were given',
-                              r"TypeError: SQRT\(\) takes 1 positional argument but 2 were given")
+      self.assertFormulaError(
+        self.engine.get_formula_error('Math', 'excel_formula', 3), TypeError,
+        'SQRT() takes 1 positional argument but 2 were given\n\n'
+        'A `TypeError` is usually caused by trying\n'
+        'to combine two incompatible types of objects,\n'
+        'by calling a function with the wrong type of object,\n'
+        'or by trying to do an operation not allowed on a given type of object.\n\n'
+        'You apparently have called the function `SQRT` with\n'
+        '2 positional argument(s) while it requires 1\n'
+        'such positional argument(s).',
+        r"TypeError: SQRT\(\) takes 1 positional argument but 2 were given",
+      )
 
+    int_not_iterable_message = "'int' object is not iterable"
+    if six.PY3:
+      int_not_iterable_message += (
+        '\n\n'
+        'A `TypeError` is usually caused by trying\n'
+        'to combine two incompatible types of objects,\n'
+        'by calling a function with the wrong type of object,\n'
+        'or by trying to do an operation not allowed on a given type of object.\n\n'
+        'An iterable is an object capable of returning its members one at a time.\n'
+        'Python containers (`list, tuple, dict`, etc.) are iterables.\n'
+        'An iterable is required here.'
+      )
     self.assertFormulaError(self.engine.get_formula_error('Math', 'built_in_formula', 3),
-                            TypeError, "'int' object is not iterable",
+                            TypeError, int_not_iterable_message,
                             textwrap.dedent(
                               r"""
                                 File "usercode", line \d+, in built_in_formula
@@ -68,8 +89,21 @@ else:
                               """
                             ))
 
+    if six.PY2:
+      message = "invalid syntax (usercode, line 5)"
+    else:
+      message = textwrap.dedent(
+        """\
+        invalid syntax
+
+        A `SyntaxError` occurs when Python cannot understand your code.
+
+        I am guessing that you wrote `:` by mistake.
+        Removing it and writing `return 0` seems to fix the error.
+
+         (usercode, line 5)""")
     self.assertFormulaError(self.engine.get_formula_error('Math', 'syntax_err', 3),
-                            SyntaxError, "invalid syntax (usercode, line 5)",
+                            SyntaxError, message,
                             textwrap.dedent(
                               r"""
                                 File "usercode", line 5
@@ -88,6 +122,7 @@ else:
         IndentationError: unexpected indent
         """
       )
+      message = 'unexpected indent (usercode, line 2)'
     else:
       traceback_regex = textwrap.dedent(
         r"""
@@ -96,12 +131,21 @@ else:
         IndentationError: unexpected indent
         """
       )
+      message = textwrap.dedent(
+        """\
+        unexpected indent
+
+        An `IndentationError` occurs when a given line of code is
+        not indented (aligned vertically with other lines) as expected.
+
+        Line `2` identified above is more indented than expected.
+
+         (usercode, line 2)""")
     self.assertFormulaError(self.engine.get_formula_error('Math', 'indent_err', 3),
-                            IndentationError, 'unexpected indent (usercode, line 2)',
-                            traceback_regex)
+                            IndentationError, message, traceback_regex)
 
     self.assertFormulaError(self.engine.get_formula_error('Math', 'other_err', 3),
-                            TypeError, "'int' object is not iterable",
+                            TypeError, int_not_iterable_message,
                             textwrap.dedent(
                               r"""
                                 File "usercode", line \d+, in other_err
@@ -350,9 +394,11 @@ else:
                             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.assertFormulaError(
+      cell_error, objtypes.CellError,
+      "Table 'AttrTest' has no column 'AA'\n(in referenced cell AttrTest[1].B)",
+      r"CellError: AttributeError in referenced cell AttrTest\[1\].B",
+    )
     self.assertEqual(
       objtypes.encode_object(cell_error),
       ['E',

sandbox/grist/test_gencode.py

@@ -71,9 +71,12 @@ class TestGenCode(unittest.TestCase):
     gcode.make_module(self.schema)
     generated = gcode.get_user_text()
     if six.PY3:
-      generated = generated.replace(
-         ", 'for a in b'))",
-        ", u'for a in b'))",
+      saved_sample = saved_sample.replace(
+        "raise SyntaxError('invalid syntax', ('usercode', 1, 11, u'for a in b'))",
+        "raise SyntaxError('invalid syntax\\n\\n"
+        "A `SyntaxError` occurs when Python cannot understand your code.\\n\\n"
+        "You wrote a `for` loop but\\nforgot to add a colon `:` at the end\\n\\n', "
+        "('usercode', 1, 11, 'for a in b'))"
       )
     self.assertEqual(generated, saved_sample, "Generated code doesn't match sample:\n" +
                      "".join(difflib.unified_diff(generated.splitlines(True),

sandbox/grist/test_trigger_formulas.py

@@ -1,5 +1,8 @@
 import copy
 import time
+
+import six
+
 import logger
 import objtypes
 import testutil
@@ -669,8 +672,20 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
       ["id",  "A",  "B",  "C"],
       [1,     0,    0,    div_error(0)],
     ])
+    message = 'float division by zero'
+    if six.PY3:
+      message += """
+
+A `ZeroDivisionError` occurs when you are attempting to divide a value
+by zero either directly or by using some other mathematical operation.
+
+You are dividing by the following term
+
+    rec.A
+
+which is equal to zero."""
     self.assertFormulaError(self.engine.get_formula_error('Math', 'C', 1),
-                            ZeroDivisionError, 'float division by zero',
+                            ZeroDivisionError, message,
                             r"1/rec\.A \+ 1/rec\.B")
     self.update_record("Math", 1, A=1)
 
@@ -682,7 +697,6 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
     ])
     error = self.engine.get_formula_error('Math', 'C', 1)
     self.assertFormulaError(error, ZeroDivisionError, 'float division by zero')
-    self.assertEqual(error._message, 'float division by zero')
     self.assertEqual(error.details, objtypes.RaisedException(ZeroDivisionError()).no_traceback().details)
 
 

sandbox/requirements3.txt

@@ -1,4 +1,13 @@
-astroid==2.5.7  # this is a difference between python 2 and 3, everything else is same
+# friendly-traceback and its dependencies, for python 3 only
+friendly-traceback==0.5.46
+stack-data==0.3.0
+executing==0.8.3
+pure-eval==0.2.2
+
+# Different astroid version for python 3
+astroid==2.5.7
+
+# Everything after this is the same for python 2 and 3
 asttokens==2.0.5
 backports.functools-lru-cache==1.6.4
 chardet==4.0.0