Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve column number tracking #406

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants