Skip to content

Output pretty error if possible#399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fantix merged 5 commits into from
Nov 18, 2022
Merged

Output pretty error if possible #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b05b319
Output pretty error if possible
fantix Nov 17, 2022
17c6997
OS-specific line separator
fantix Nov 17, 2022
0595db9
Safe lazy color initialization
fantix Nov 17, 2022
89399c1
Able to turn off error hint by EDGEDB_ERROR_HINT=off
fantix Nov 17, 2022
0a5acae
CRF: use consistent env var values
fantix Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions edgedb/color.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
import os
import sys
import warnings

COLOR = None


class Color:
HEADER = ""
BLUE = ""
CYAN = ""
GREEN = ""
WARNING = ""
FAIL = ""
ENDC = ""
BOLD = ""
UNDERLINE = ""


def get_color() -> Color:
global COLOR

if COLOR is None:
COLOR = Color()
if type(USE_COLOR) is bool:
use_color = USE_COLOR
else:
try:
use_color = USE_COLOR()
except Exception:
use_color = False
if use_color:
COLOR.HEADER = '\033[95m'
COLOR.BLUE = '\033[94m'
COLOR.CYAN = '\033[96m'
COLOR.GREEN = '\033[92m'
COLOR.WARNING = '\033[93m'
COLOR.FAIL = '\033[91m'
COLOR.ENDC = '\033[0m'
COLOR.BOLD = '\033[1m'
COLOR.UNDERLINE = '\033[4m'

return COLOR


try:
USE_COLOR = {
"default": lambda: sys.stderr.isatty(),
"auto": lambda: sys.stderr.isatty(),
"enabled": True,
"disabled": False,
}[
os.getenv("EDGEDB_COLOR_OUTPUT", "default")
]
except KeyError:
warnings.warn(
"EDGEDB_COLOR_OUTPUT can only be one of: "
"default, auto, enabled or disabled"
)
USE_COLOR = False
138 changes: 134 additions & 4 deletions edgedb/errors/_base.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,12 @@
#


import io
import os
import traceback
import unicodedata
import warnings

__all__ = (
'EdgeDBError', 'EdgeDBMessage',
)
Expand DownExpand Up@@ -79,6 +85,7 @@ class EdgeDBErrorMeta(Meta):
class EdgeDBError(Exception, metaclass=EdgeDBErrorMeta):

_code = None
_query = None
tags = frozenset()

def __init__(self, *args, **kwargs):
Expand All@@ -93,15 +100,25 @@ def _position(self):
# not a stable API method
return int(self._read_str_field(FIELD_POSITION_START, -1))

@property
def _position_start(self):
# not a stable API method
return int(self._read_str_field(FIELD_CHARACTER_START, -1))

@property
def _position_end(self):
# not a stable API method
return int(self._read_str_field(FIELD_CHARACTER_END, -1))

@property
def _line(self):
# not a stable API method
return int(self._read_str_field(FIELD_LINE, -1))
return int(self._read_str_field(FIELD_LINE_START, -1))

@property
def _col(self):
# not a stable API method
return int(self._read_str_field(FIELD_COLUMN, -1))
return int(self._read_str_field(FIELD_COLUMN_START, -1))

@property
def _hint(self):
Expand All@@ -127,6 +144,35 @@ def _from_code(code, *args, **kwargs):
exc._code = code
return exc

def __str__(self):
msg = super().__str__()
if SHOW_HINT and self._query and self._position_start >= 0:
try:
return _format_error(
msg,
self._query,
self._position_start,
max(1, self._position_end - self._position_start),
self._line if self._line > 0 else "?",
self._col if self._col > 0 else "?",
self._hint or "error",
)
except Exception:
return "".join(
(
msg,
LINESEP,
LINESEP,
"During formatting of the above exception, "
"another exception occurred:",
LINESEP,
LINESEP,
traceback.format_exc(),
)
)
else:
return msg


def _lookup_cls(code: int, *, meta: type, default: type):
try:
Expand DownExpand Up@@ -180,15 +226,83 @@ def _severity_name(severity):
return 'PANIC'


def _format_error(msg, query, start, offset, line, col, hint):
c = get_color()
rv = io.StringIO()
rv.write(f"{c.BOLD}{msg}{c.ENDC}{LINESEP}")
lines = query.splitlines(keepends=True)
num_len = len(str(len(lines)))
rv.write(f"{c.BLUE}{'':>{num_len}} ┌─{c.ENDC} query:{line}:{col}{LINESEP}")
rv.write(f"{c.BLUE}{'':>{num_len}} │ {c.ENDC}{LINESEP}")
for num, line in enumerate(lines):
length = len(line)
line = line.rstrip() # we'll use our own line separator
if start >= length:
# skip lines before the error
start -= length
continue

if start >= 0:
# Error starts in current line, write the line before the error
first_half = repr(line[:start])[1:-1]
line = line[start:]
length -= start
rv.write(f"{c.BLUE}{num + 1:>{num_len}} │ {c.ENDC}{first_half}")
start = _unicode_width(first_half)
else:
# Multi-line error continues
rv.write(f"{c.BLUE}{num + 1:>{num_len}} │ {c.FAIL}│ {c.ENDC}")

if offset > length:
# Error is ending beyond current line
line = repr(line)[1:-1]
rv.write(f"{c.FAIL}{line}{c.ENDC}{LINESEP}")
if start >= 0:
# Multi-line error starts
rv.write(f"{c.BLUE}{'':>{num_len}} │ "
f"{c.FAIL}╭─{'─' * start}^{c.ENDC}{LINESEP}")
offset -= length
start = -1 # mark multi-line
else:
# Error is ending within current line
first_half = repr(line[:offset])[1:-1]
line = repr(line[offset:])[1:-1]
rv.write(f"{c.FAIL}{first_half}{c.ENDC}{line}{LINESEP}")
size = _unicode_width(first_half)
if start >= 0:
# Mark single-line error
rv.write(f"{c.BLUE}{'':>{num_len}} │ {' ' * start}"
f"{c.FAIL}{'^' * size} {hint}{c.ENDC}")
else:
# End of multi-line error
rv.write(f"{c.BLUE}{'':>{num_len}} │ "
f"{c.FAIL}╰─{'─' * (size - 1)}^ {hint}{c.ENDC}")
break
return rv.getvalue()


def _unicode_width(text):

@msullivanmsullivanNov 18, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is an approximation that will overestimate the length of strings using combining characters that don't normalize away.
This means we can get stuff like

cannot redefine property 'x' of object type 'std::FreeObject' as scalar type 'std::str'
┌─ query:2:26
│ 2 │ select { x := 1 } { x := 'f̷͈͎͒̕ǫ̴̏͌ö̶̱̘' };
│ ^^^^^^^^^^^^^^^^ error

https://zalgo.org/ has a great generator for pathological unicode strings.

Probably this is almost always fine.

@msullivanmsullivanNov 18, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CLI's error formatter does seem to handle this one right. I'm not sure exactly what algorithm it uses.
I think to some extent anything we do is an approximation because it depends on assumptions about how the terminal will render it.

Unicode does have a notion of "grapheme clusters", and there are python libraries for handling them (https://github.com/alvinlindstam/grapheme). I'm not sure that this is the best approach either, though, since the width of those clusters can vary and I don't know if there is good way to guess that well. (Consider 🇨🇦, which depending on your terminal will show as either a flag or the letters CA, but either way will probably take two characters, but is only one grapheme cluster.)

Maybe the best approach on our side would be basically just to drop combining characters.

There is a potentially infinitely deep rabbit hole here.

@tailhooktailhookNov 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust uses de facto standard in Rust: https://crates.io/crates/unicode-width
Which uses Unicode Standard Annex 11 for calculating width.

Note: unicode width != number of graphemes. As width of a grapheme can be two chars.

And you're right that in generic case it's impossible to calculate width. Because some characters can have width not just depending on the terminal but also on the specific font (if there is a character for a specific set of emojis). unicode-width's frontpage gives an example.

Dropping Zero-width Joiners

  1. I've tested three terminals I have around (xfce4, alacritty, wezterm), and all of them do match width with unicode width, even though some of them do not render some of the characters (i.e. flags are only supported in wezterm). Disclaimer: alacritty and wezterm are implemented in Rust so this explains it a bit, although xfce4 is not and has much longer history.
  2. Copying from any of the three terminal's I've tried does discard zero-width joiner by itself
  3. Rendering in Firefox on my machine gives invalid width regardless of if there is a zero-width joiner (i.e. it somehow draws emojis 1.5 of character width 🤷 )
  4. Rendering in Chrome on my machine gives good results when zero-width joiner is dropped (like normally copying from terminal), but doesn't if I somehow manage to keep that character (i.e. piping output to xsel -b manually).

Note: zalgo-generated stuff looks like plain foo in all three terminals (although those unicode tricks are present in the output)

The summary of this: dropping zero-width joiners is safe to do, although looks unnecessary in my setup. If that's not true for some popular terminals or on other systems it could be done.

Fixup

For my examples this function works:

def unicode_width(s):
return sum(0 if unicodedata.category(c) in ('Mn', 'Cf') else
2 if unicodedata.east_asian_width(c) == "W" else 1
for c in s)

But I'm not sure if this is good enough. The libraries doing this (rustic unicode-width, and pythonic urwid) have their own width tables.

@fantixfantixNov 23, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in #404

return sum(
2 if unicodedata.east_asian_width(c) == "W" else 1
for c in unicodedata.normalize("NFC", text)
)


FIELD_HINT = 0x_00_01
FIELD_DETAILS = 0x_00_02
FIELD_SERVER_TRACEBACK = 0x_01_01

# XXX: Subject to be changed/deprecated.
FIELD_POSITION_START = 0x_FF_F1
FIELD_POSITION_END = 0x_FF_F2
FIELD_LINE = 0x_FF_F3
FIELD_COLUMN = 0x_FF_F4
FIELD_LINE_START = 0x_FF_F3
FIELD_COLUMN_START = 0x_FF_F4
FIELD_UTF16_COLUMN_START = 0x_FF_F5
FIELD_LINE_END = 0x_FF_F6
FIELD_COLUMN_END = 0x_FF_F7
FIELD_UTF16_COLUMN_END = 0x_FF_F8
FIELD_CHARACTER_START = 0x_FF_F9
FIELD_CHARACTER_END = 0x_FF_FA


EDGE_SEVERITY_DEBUG = 20
Expand All@@ -198,3 +312,19 @@ def _severity_name(severity):
EDGE_SEVERITY_ERROR = 120
EDGE_SEVERITY_FATAL = 200
EDGE_SEVERITY_PANIC = 255


LINESEP = os.linesep

try:
SHOW_HINT = {"default": True, "enabled": True, "disabled": False}[
os.getenv("EDGEDB_ERROR_HINT", "default")
]
except KeyError:
warnings.warn(
"EDGEDB_ERROR_HINT can only be one of: default, enabled or disabled"
)
SHOW_HINT = False


from edgedb.color import get_color
2 changes: 2 additions & 0 deletions edgedb/protocol/protocol.pyx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -305,6 +305,7 @@ cdef class SansIOProtocol:

elif mtype == ERROR_RESPONSE_MSG:
exc = self.parse_error_message()
exc._query = query
exc = self._amend_parse_error(
exc, output_format, expect_one, required_one)

Expand DownExpand Up@@ -435,6 +436,7 @@ cdef class SansIOProtocol:

elif mtype == ERROR_RESPONSE_MSG:
exc = self.parse_error_message()
exc._query = query
if exc.get_code() == parameter_type_mismatch_code:
if not isinstance(in_dc, NullCodec):
buf = WriteBuffer.new()
Expand Down