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

Don't check line in JS_EvalThis2 #909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ABBAPOH
Copy link
Contributor

@ABBAPOH ABBAPOH commented Feb 12, 2025

The check was added in a later version of pull request an attempt to be on the safe side of things; but it break one Qbs test. Apparently, 0 is a valid value in Qbs code. Which might be a bug, but currently the code relies on that and when using 1 instead of 0, the line in the stack trace is not correct.
Remove the check for now.

@ABBAPOH ABBAPOH changed the title Don't check line Don't check line in JS_EvalThis2 Feb 12, 2025
@bnoordhuis
Copy link
Contributor

I feel we should treat this as a fire drill on how to deal with bugs and backwards incompatible changes.

Landing this pull request as-is breaks backwards compatibility (not really because it hasn't been released yet but bear with me); that leaves three options:

  1. Do nothing. Books start on the first line; so does code. Line 0 doesn't make sense

  2. Add a new eval flag JS_EVAL_ZERO_MEANS_ZERO, actual name TBD

  3. Increment JS_EVAL_OPTIONS_VERSION and update the code so v1 keeps working as before but in v2 .line = 0 is interpreted literally; an options struct is supposed to be ergonomic however and this is kind of anathema to that

I'm curious though... before commit 78bdbef the first line number was always 1. What changed that makes that test fail?

@ABBAPOH
Copy link
Contributor Author

ABBAPOH commented Feb 17, 2025

I'm curious though

It is 1 in quickjs code, but Qbs passes 0 (and even -1, heh) in our patched version. While this seems to be a bug in the first place (especially the -1 value which stands for invalid/empty line (code location)), it actually provides the correct result in tests, so we kinda rely on this behaviour for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants