Skip to content

Improve column number tracking #406

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

Closed
wants to merge 1 commit into from

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented May 15, 2024

  • simplify column number tracking using a pointer to the beginning of line
    instead of eol + mark.
  • add js_parse_error_pos to report syntax errors with correct column number
    for token parsing errors. This makes the syntax error reports much more precise.
  • add JSSourcePos type to keep track of token source position
  • add emit_op_pos to set the precise source position in code generation
    this is work in progress: it should be the only way to generate source positions
    and we should get rid of s->last_line_num and s->last_col_num.
  • runtime errors on calls report the column number of calling function or method name.
  • runtime errors on new expressions report the column number of the neẁ keyword.
  • update tests/test_builtin,js with more informative messages
  • improve assert() and tests/test_language.js tests
  • update v8.txt for updated column numbers in remaining errors

@chqrlie chqrlie force-pushed the improve-column-numbers branch from 58d8f73 to 1a220e7 Compare May 15, 2024 05:54
@saghul saghul requested a review from bnoordhuis May 15, 2024 07:13
@saghul
Copy link
Contributor

saghul commented May 15, 2024

I'll defer this review to @bnoordhuis who worked on adding column numbers :-P

@chqrlie chqrlie force-pushed the improve-column-numbers branch 5 times, most recently from cac9960 to cfcc36d Compare May 18, 2024 23:02
- simplify column number tracking using a pointer to the beginning of line
  instead of `eol` + `mark`.
- add `js_parse_error_pos` to report syntax errors with exact source position
  for token parsing errors. This makes the syntax error reports much more precise.
  eg: exact position of UTF-8 encoding error, invalid escape sequence, etc.
- add `JSSourcePos` type to use single opaque object for token source position
- add `emit_pos` to set the precise source position in code generation
- change `emit_op` to no longer emit source positions from `s->last_line_num` and `s->last_col_num`.
- remove `last_line_num` and `last_col_num` `JSParserState` members
- runtime errors on calls report the column number of calling function or method name.
- runtime errors on `new` expressions report the column number of the `neẁ` keyword.
- do not show source position in backtrace if debug information is missing
- fix spurious parsing bugs when `js_parse_skip_parens_token` could not reparse
  the current token because of stack overflow detection.
- `js_parse_save_pos` now saves the current token and `js_parse_seek_back` always
  restores the token, hence never fails, while `js_parse_seek_token` reparses the
  saved token. This is needed to handle the weird semantics of `"\1"; "use strict";`
- simplify html comment detection
- update **tests/test_builtin,js** with more informative messages
- improve `assert()` and **tests/test_language.js** tests
- update **v8.txt** for updated column numbers in remaining errors
@chqrlie chqrlie force-pushed the improve-column-numbers branch from cfcc36d to 5946221 Compare May 27, 2024 09:31
@chqrlie chqrlie mentioned this pull request Jun 7, 2024
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a rebase, but it should be good to go otherwise. Sorry for the delay @chqrlie !

@@ -399,6 +399,26 @@ found:
=== json-errors.js
Failure:
expected:
"Unexpected end of JSON input"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these new?

@TooTallNate
Copy link
Contributor

Is this expected to improve the issue in #236?

@chqrlie
Copy link
Collaborator Author

chqrlie commented Jul 4, 2024

Is this expected to improve the issue in #236?

Yes it should. Let me try this test after I fix the conflicts.

@ammarahm-ed
Copy link
Contributor

Hi, I am working on a project that is blocked due to this, can we possibly get this merged anytime soon.

@saghul
Copy link
Contributor

saghul commented Nov 4, 2024

@chqrlie are you planning on picking this back up? It likely needs a rebase at least 😅

@bnoordhuis
Copy link
Contributor

I fixed this just now in #660. Please file a new issue if anything still doesn't work as expected.

@bnoordhuis bnoordhuis closed this Nov 7, 2024
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.

5 participants