From 9c123bd3c5671fd27124731c3f3c8c1dbf7574c5 Mon Sep 17 00:00:00 2001 From: Roger Chapman Date: Mon, 2 Sep 2019 20:41:58 +1000 Subject: [PATCH] better error handling and error formatting --- context.go | 44 ++++++------------------- context_test.go | 30 +++++++++++++++-- errors.go | 36 +++++++++++++++++++++ errors_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++++++ v8go.cc | 53 ++++++++++++++++++++++-------- v8go.h | 8 ++++- 6 files changed, 204 insertions(+), 53 deletions(-) create mode 100644 errors.go create mode 100644 errors_test.go diff --git a/context.go b/context.go index 177d704e..585ff422 100644 --- a/context.go +++ b/context.go @@ -5,42 +5,10 @@ package v8go import "C" import ( "fmt" - "io" "runtime" "unsafe" ) -type jsErr struct { - msg string - location string - stack string -} - -func (e *jsErr) Error() string { - return e.msg -} - -func (e *jsErr) Format(s fmt.State, verb rune) { - switch verb { - case 'v': - if s.Flag('+') { - io.WriteString(s, e.msg) - if e.location != "" { - fmt.Fprintf(s, ": %s", e.location) - } - if e.stack != "" { - fmt.Fprintf(s, "\n%s", e.stack) - } - return - } - fallthrough - case 's': - io.WriteString(s, e.msg) - case 'q': - fmt.Fprintf(s, "%q", e.msg) - } -} - // Context is a global root execution environment that allows separate, // unrelated, JavaScript applications to run in a single instance of V8. type Context struct { @@ -79,7 +47,8 @@ func (c *Context) Isolate() (*Isolate, error) { } // RunScript executes the source JavaScript; origin or filename provides a -// reference for the script and used in the exception stack trace. +// reference for the script and used in the stack trace if there is an error. +// error will be of type `JSError` of not nil. func (c *Context) RunScript(source string, origin string) (*Value, error) { cSource := C.CString(source) cOrigin := C.CString(origin) @@ -106,8 +75,13 @@ func getValue(rtn C.RtnValue) *Value { } func getError(rtn C.RtnValue) error { - if rtn.error == nil { + if rtn.error.msg == nil { return nil } - return &jsErr{msg: C.GoString(rtn.error), location: "blah line 8", stack: "bad things happen"} + err := &JSError{ + Message: C.GoString(rtn.error.msg), + Location: C.GoString(rtn.error.location), + StackTrace: C.GoString(rtn.error.stack), + } + return err } diff --git a/context_test.go b/context_test.go index 693aad34..072e7e64 100644 --- a/context_test.go +++ b/context_test.go @@ -29,8 +29,32 @@ func TestContextExec(t *testing.T) { } } -func TestBadScript(t *testing.T) { +func TestJSExceptions(t *testing.T) { + t.Parallel() + + tests := [...]struct { + name string + source string + origin string + err string + }{ + {"SyntaxError", "bad js syntax", "syntax.js", "SyntaxError: Unexpected identifier"}, + {"ReferenceError", "add()", "add.js", "ReferenceError: add is not defined"}, + } + ctx, _ := v8go.NewContext(nil) - _, err := ctx.RunScript("bad script", "bad.js") - t.Errorf("error: %+v", err) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := ctx.RunScript(tt.source, tt.origin) + if err == nil { + t.Error("error expected but got ") + return + } + if err.Error() != tt.err { + t.Errorf("expected %q, got %q", tt.err, err.Error()) + } + }) + } } diff --git a/errors.go b/errors.go new file mode 100644 index 00000000..3604c090 --- /dev/null +++ b/errors.go @@ -0,0 +1,36 @@ +package v8go + +import ( + "fmt" + "io" +) + +// JSError is an error that is returned if there is are any +// JavaScript exceptions handled in the context. When used with the fmt +// verb `%+v`, will output the JavaScript stack trace, if available. +type JSError struct { + Message string + Location string + StackTrace string +} + +func (e *JSError) Error() string { + return e.Message +} + +func (e *JSError) Format(s fmt.State, verb rune) { + switch verb { + case 'v': + if s.Flag('+') { + if e.StackTrace != "" { + io.WriteString(s, e.StackTrace) + return + } + } + fallthrough + case 's': + io.WriteString(s, e.Message) + case 'q': + fmt.Fprintf(s, "%q", e.Message) + } +} diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 00000000..645f7c0e --- /dev/null +++ b/errors_test.go @@ -0,0 +1,86 @@ +package v8go_test + +import ( + "fmt" + "testing" + + "rogchap.com/v8go" +) + +func TestFormatting(t *testing.T) { + t.Parallel() + tests := [...]struct { + name string + err error + defaultVerb string + defaultVerbFlag string + stringVerb string + quoteVerb string + }{ + {"WithStack", &v8go.JSError{Message: "msg", StackTrace: "stack"}, "msg", "stack", "msg", `"msg"`}, + {"WithoutStack", &v8go.JSError{Message: "msg"}, "msg", "msg", "msg", `"msg"`}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if s := fmt.Sprintf("%v", tt.err); s != tt.defaultVerb { + t.Errorf("incorrect format for %%v: %s", s) + } + if s := fmt.Sprintf("%+v", tt.err); s != tt.defaultVerbFlag { + t.Errorf("incorrect format for %%+v: %s", s) + } + if s := fmt.Sprintf("%s", tt.err); s != tt.stringVerb { + t.Errorf("incorrect format for %%s: %s", s) + } + if s := fmt.Sprintf("%q", tt.err); s != tt.quoteVerb { + t.Errorf("incorrect format for %%q: %s", s) + } + }) + } +} + +func TestJSErrorOutput(t *testing.T) { + t.Parallel() + ctx, _ := v8go.NewContext(nil) + + math := ` + function add(a, b) { + return a + b; + } + + function addMore(a, b) { + return add(a, c); + }` + + main := ` + let a = add(3, 5); + let b = addMore(a, 6); + b; + ` + + ctx.RunScript(math, "math.js") + _, err := ctx.RunScript(main, "main.js") + if err == nil { + t.Error("expected error but got ") + return + } + e, ok := err.(*v8go.JSError) + if !ok { + t.Errorf("expected error of type JSError, got %T", err) + } + if e.Message != "ReferenceError: c is not defined" { + t.Errorf("unexpected error message: %q", e.Message) + } + if e.Location != "math.js:7:17" { + t.Errorf("unexpected error location: %q", e.Location) + } + expectedStack := `ReferenceError: c is not defined + at addMore (math.js:7:17) + at main.js:3:10` + + if e.StackTrace != expectedStack { + t.Errorf("unexpected error stack trace: %q", e.StackTrace) + } +} diff --git a/v8go.cc b/v8go.cc index 36eed14a..706986a0 100644 --- a/v8go.cc +++ b/v8go.cc @@ -26,32 +26,57 @@ typedef struct { const char* CString(String::Utf8Value& value) { if (value.length() == 0) { - return "empty exception"; + return "empty"; } return *value; } -const char* ExceptionString(TryCatch& try_catch, Isolate* iso, Local ctx) { +const char* CopyString(std::string str) { + char* data = static_cast(malloc(str.length())); + sprintf(data, "%s", str.c_str()); + return data; +} + +const char* CopyString(String::Utf8Value& value) { + if (value.length() == 0) { + return ""; + } + return CopyString(*value); +} + +RtnError ExceptionError(TryCatch& try_catch, Isolate* iso, Local ctx) { Locker locker(iso); Isolate::Scope isolate_scope(iso); HandleScope handle_scope(iso); - std::stringstream sb; + RtnError rtn = {nullptr, nullptr, nullptr}; + String::Utf8Value exception(iso, try_catch.Exception()); - sb << CString(exception); + rtn.msg = CopyString(exception); Local msg = try_catch.Message(); if (!msg.IsEmpty()) { String::Utf8Value origin(iso, msg->GetScriptOrigin().ResourceName()); - sb << std::endl - << " at " << CString(origin); + std::ostringstream sb; + sb << *origin; + Maybe line = try_catch.Message()->GetLineNumber(ctx); + if (line.IsJust()) { + sb << ":" << line.ToChecked(); + } + Maybe start = try_catch.Message()->GetStartColumn(ctx); + if (start.IsJust()) { + sb << ":" << start.ToChecked() + 1; // + 1 to match output from stack trace + } + rtn.location = CopyString(sb.str()); } - - - std::string str = sb.str(); - char* data = static_cast(malloc(str.length())); - sprintf(data, "%s", str.c_str()); - return data; + + MaybeLocal mstack = try_catch.StackTrace(ctx); + if (!mstack.IsEmpty()) { + String::Utf8Value stack(iso, mstack.ToLocalChecked()); + rtn.stack = CopyString(stack); + } + + return rtn; } extern "C" @@ -120,13 +145,13 @@ RtnValue RunScript(ContextPtr ctx_ptr, const char* source, const char* origin) { MaybeLocal