Skip to content
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.

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

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

Closed
bnoordhuis opened this issue Oct 27, 2024 · 5 comments
Closed

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

bnoordhuis opened this issue Oct 27, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@bnoordhuis
Copy link
Contributor

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.

@bnoordhuis bnoordhuis added the enhancement New feature or request label Oct 27, 2024
@saghul
Copy link
Contributor

saghul commented Oct 27, 2024

I think the challenge here is that everyone likely borrowed the code from libc that prints the stack:

void js_std_dump_error1(JSContext *ctx, JSValue exception_val)
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@chqrlie
Copy link
Collaborator

chqrlie commented Oct 27, 2024

Note that

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.

NULL is returned when the conversion to string generates an exception.
There are 2 special cases:

  • if the argument conversion to a string generated an exception but the argument is an instance of Error and has amessage property that is a string. (the case you want to change).
  • memory allocation is required for the final UTF-8 conversion and failed (an exception should be generated in this case).

Checking for NULL is required so the exception can be propagated to the caller. Returning a pointer to a static char array whose address could be tested by JS_FreeCString() is feasible but makes the exception test cumbersome and more likely to be missing.

I guess the API should have been different, taking the pointer by reference and returning a status that can be checked directly. Changing that seems overkill.

Your proposition, including the stack trace in the special that emulates Error.toString(), has a very limited scope: it would only occur when the conversion of an Error object itself generates an exception, eg: in case of a pending stack overflow.

What is your use case ?

@bnoordhuis
Copy link
Contributor Author

bnoordhuis commented Oct 27, 2024

I left a comment (#632 (comment)) in another issue just now that outlines what I'm looking for: an infallible to-string method that's free of side effects (no JS calls) and returns something that's (edit: maximally!) useful to human eyes.

I'll close this issue, let's continue the discussion in the other one.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants