feat: improve error report

[gengjiawen]

fix: bellard/quickjs#93

currently for input

var dog = {};
dog.bark = function() {
  throw new Error('woof woof');
};
function makeStack() {
  try {
    dog.bark();
  } catch (error) {
    print(error.stack)
  }
}
makeStack();

Engine behaviour

v8:
Error: woof woof
    at dog.bark (/tmp/3df242e:3:9)
    at makeStack (/tmp/3df242e:7:9)
    at /tmp/3df242e:12:1

quickjs-ng:
    at <anonymous> (/tmp/59832ce:3:13)
    at makeStack (/tmp/59832ce:7:5)
    at <eval> (/tmp/59832ce:12:1)

this PR:

quickjs-ng:
    at bark (/tmp/59832ce:3:13)
    at makeStack (/tmp/59832ce:7:5)
    at <eval> (/tmp/59832ce:12:1)

[bnoordhuis]

1. I don't think JS_SetPropertyInternal2 is the right place for this because then we're adding a property as a side effect of adding another property.

A better place is when constructing the backtrace, maybe? (But we're already calling get_func_name there so why isn't that working?)

2. Going through JS_ToCString and JS_FreeCString is kind of inefficient. It's better to split up get_func_name into two parts, one that looks up the JSValue, and one that turns it into a C string.

[bnoordhuis]

Minor style issues: alignment + star leans right; applies to the closing brace too

Correct me if I'm wrong but you want to set the name if it's not empty, right? Also, get_func_name can return NULL

Suggested change

	const char* name = get_func_name(ctx, val);
	if (strcmp(name, "") == 0) {
	const char *name = get_func_name(ctx, val);
	if (name && *name) {

[gengjiawen]

I don't think JS_SetPropertyInternal2 is the right place for this because then we're adding a property as a side effect of adding another property.

It's anonymous function by definition function() {throw new Error('woof woof');}; It use the object property name as a fallback for easy debugging.

[gengjiawen]

Correct me if I'm wrong but you want to set the name if it's not empty, right?

Yeap. quickjs now already works when dog.bark value is named function

var dog = {};
dog.bark = function c2() {
  throw new Error('woof woof');
};
function makeStack() {
  try {
    dog.bark();
  } catch (error) {
    print(error.stack)
  }
}
makeStack();

You can see it here http://k2.gengjiawen.com:8001/

[bnoordhuis]

Yes, I understand that. I think in general it should be possible to recover that information from the JSStackFrame when the backtrace is built.

(I worked on something similar recently for constructors, although I haven't finished that yet.)

The problem I see with the approach in this PR is that it produces the wrong result in this example, right?

var a = {foo: function() { throw Error("fail") }}
var b = {bar: a.foo}
b.bar() // "foo" in stack trace

edit: cross-posted

[gengjiawen]

Yes, I understand that. I think in general it should be possible to recover that information from the JSStackFrame when the backtrace is built.

Yeap. Is there enough context when construct backtrace since this request the property info ? From what I see looks like quickjs not saving enough context for now.

The problem I see with the approach in this PR is that it produces the wrong result in this example, right?

Personally I think it's acceptable. the function is foo when defined.
v8 for reference

/tmp/6bfa5ab:1: Error: fail
var a = {foo: function() { throw Error("fail") }}
                           ^
Error: fail
    at Object.foo [as bar] (/tmp/6bfa5ab:1:34)
    at /tmp/6bfa5ab:3:3

[gengjiawen]

Also, get_func_name can return NULL

Is this why the segment fault in ASAN ?

[bnoordhuis]

Is there enough context when construct backtrace

The answer is a qualified "yes". I had it sort of working for constructors but in general no. I'll grant that making JS_CallInternal record that information is probably a lot more involved than your current approach.

That said, I still don't think JS_SetPropertyInternal2 is the proper place for that because that makes the .name property magically appear as an observable side effect of another operation.

After thinking about it some more, I think the right place is in the parser. We already parse something like this correctly:

var f = function() {} // f.name === "f"

And also:

var o = {f: function() {}} // o.f.name === "f"

But not:

var o = {}
o.f = function() {} // o.f.name === ""