From 49cb51bac57874f581a1acb7437f27b5e2ed88b4 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 11 Aug 2022 13:03:45 +0200 Subject: [PATCH] (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 | 23 +++++---- sandbox/grist/codebuilder.py | 58 +++++++++++++++++++--- sandbox/grist/friendly_errors.py | 43 ++++++++++++++++ sandbox/grist/gencode.py | 15 ++---- sandbox/grist/objtypes.py | 9 +++- sandbox/grist/test_codebuilder.py | 47 +++++++++++++----- sandbox/grist/test_engine.py | 2 +- sandbox/grist/test_formula_error.py | 68 +++++++++++++++++++++----- sandbox/grist/test_gencode.py | 9 ++-- sandbox/grist/test_trigger_formulas.py | 18 ++++++- sandbox/requirements3.txt | 11 ++++- 11 files changed, 239 insertions(+), 64 deletions(-) create mode 100644 sandbox/grist/friendly_errors.py diff --git a/app/server/lib/NSandbox.ts b/app/server/lib/NSandbox.ts index 5c73b2c0..5d023da9 100644 --- a/app/server/lib/NSandbox.ts +++ b/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!, diff --git a/sandbox/grist/codebuilder.py b/sandbox/grist/codebuilder.py index ca5324d2..9c1accf6 100644 --- a/sandbox/grist/codebuilder.py +++ b/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, - ('', 1, 1, "")) + error = GristSyntaxError(message, ('', 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"', ('', node.lineno, node.col_offset, "")) @classmethod @@ -315,8 +337,8 @@ 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 `=`. " - "If you mean to check for equality, use `==` instead.", + raise GristSyntaxError("You can't assign a value to a column with `=`. " + "If you mean to check for equality, use `==` instead.", ('', node.lineno, node.col_offset, "")) @classmethod @@ -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, + ) diff --git a/sandbox/grist/friendly_errors.py b/sandbox/grist/friendly_errors.py new file mode 100644 index 00000000..a736b6ab --- /dev/null +++ b/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 "" diff --git a/sandbox/grist/gencode.py b/sandbox/grist/gencode.py index 65f5ed7d..ed6a47ba 100644 --- a/sandbox/grist/gencode.py +++ b/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 diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py index 36cc038e..cfd9d261 100644 --- a/sandbox/grist/objtypes.py +++ b/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 diff --git a/sandbox/grist/test_codebuilder.py b/sandbox/grist/test_codebuilder.py index 377e5bb1..2ce55fca 100644 --- a/sandbox/grist/test_codebuilder.py +++ b/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') diff --git a/sandbox/grist/test_engine.py b/sandbox/grist/test_engine.py index 79a721df..cddbc9be 100644 --- a/sandbox/grist/test_engine.py +++ b/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) diff --git a/sandbox/grist/test_formula_error.py b/sandbox/grist/test_formula_error.py index d6f82707..3e91b7bb 100644 --- a/sandbox/grist/test_formula_error.py +++ b/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', diff --git a/sandbox/grist/test_gencode.py b/sandbox/grist/test_gencode.py index 218e4128..dff010c9 100644 --- a/sandbox/grist/test_gencode.py +++ b/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), diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index 23885ce1..ae52a2fb 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/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) diff --git a/sandbox/requirements3.txt b/sandbox/requirements3.txt index b673a446..38fb0a18 100644 --- a/sandbox/requirements3.txt +++ b/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