search

quickjs-ng / quickjs

QuickJS, the Next Generation: a mighty JavaScript engine
https://quickjs-ng.github.io/quickjs/
MIT License
1.01k stars 86 forks source link

[rfc] include stack trace when stringifying exception object #630

Open bnoordhuis opened 2 hours ago

bnoordhuis commented 2 hours ago

JS_ToCStringLen2 (and therefore JS_ToCString and JS_ToCStringLen) currently special-cases exception objects:

https://github.com/quickjs-ng/quickjs/blob/d2bca87c64e41162e3da0ffe0a19858a81b09db4/quickjs.c#L4091-L4106

That is, it stringifies obj.message and returns that. That only tells you what though, not where.

I want to include obj.stack as well to make debugging easier but I'm wondering if that could break existing code somehow. The most visible change is that the string is going to contain newlines where before it (usually) didn't.

saghul commented 2 hours ago

I think the challenge here is that everyone likely borrowed the code from libc that prints the stack: https://github.com/quickjs-ng/quickjs/blob/d2bca87c64e41162e3da0ffe0a19858a81b09db4/quickjs-libc.c#L4063 I know I did: https://github.com/saghul/txiki.js/blob/9c65cd39b1bfe98e5b2b286113f2de29087b732d/src/utils.c#L147

Those would need to be adjusted, which I really don't mind.

I'm not sure I understand this part:

    // Stringification can fail when there is an exception pending,
    // e.g. a stack overflow InternalError. Special-case exception
    // objects to make debugging easier, look up the .message property
    // and stringify that.

Are the 2 sentences related? I guess no, then if we are to special-case exception objects we might as well stringify the stack trace?

But at the same time that is somewhat inconsistent with new Error('boo!').toString() since it doesn't incldue the stack...

All of that to say I don't think I have a strong opinion 😅

bnoordhuis commented 1 hour ago

Are the 2 sentences related? I guess no, then if we are to special-case exception objects we might as well stringify the stack trace?

They're related in the sense that we ought to call the .toString method but we don't, we take a shortcut to avoid invoking JS code. And since we're doing that for the sake of debuggability, I figured we might as well go the whole way.

bnoordhuis commented 1 hour ago

Oh, and a semi-related thing: JS_ToCString and friends can fail for a variety of reasons and return NULL. I'd like to change it to return a (statically allocated) empty string because I always forget to add the NULL check.