refactor(parser): byte handler for illegal bytes#2229
Merged
Conversation
Member
Author
|
Built on top of #2228. It'd be better if I was using Graphite, but I have a lot of WIP branches on my fork, so would like to get them wrapped up before migrating to Graphite. |
CodSpeed Performance ReportMerging #2229 will not alter performanceFalling back to comparing Summary
|
overlookmotel
force-pushed
the
parser-illegal-bytes
branch
from
January 31, 2024 10:50
06e2013 to
836fe0d
Compare
Member
Author
|
Now rebased on main and ready for merge if you're happy with the change. |
Member
Author
|
Who needs Graphite when you're this fast to review and merge? :) |
IWANABETHATGUY
pushed a commit
to IWANABETHATGUY/oxc
that referenced
this pull request
May 29, 2024
This adds a separate byte handler to the lexer for byte values which should never be encountered: 1. UTF-8 continuation bytes (i.e. middle of a multi-byte UTF-8 byte sequence). 2. Bytes values which are illegal in valid UTF-8 strings. At present, this function is impossible to reach, because `std::str::Chars` ensures the next byte is always the *start* of a valid UTF-8 byte sequence. But later changes I intend introducing unsafe code will make it possible (but highly undesirable!). In the meantime, I don't think it does any harm to handle this case.
camc314
added a commit
that referenced
this pull request
Jun 30, 2026
## Summary Fixes #23870. With `checkAllIndexAccess: true`, `unicorn/prefer-at` reported and auto-fixed numeric-key access on plain objects, rewriting e.g. `tokens[60]` (where `const tokens = { 60: '#666666' }`) to `tokens.at(60)`. Plain objects have no `.at()` method, so the autofix produced code that fails to type-check (`TS2339`) / throws at runtime. This skips receivers that are obviously not array-like before reporting under `checkAllIndexAccess`: - object literals — `({ 1: 1 })[1]` - non-string literals, function expressions, and class expressions - identifiers bound to a `const` whose initializer is one of the above — `const o = { 1: 1 }; o[1]` — including through TypeScript `as` / `satisfies` / `!` wrappers Strings keep `.at()`, so string receivers are still reported and fixed (`"abc"[1]` -> `"abc".at(1)`). The guard runs only under the opt-in `checkAllIndexAccess` option and short-circuits on a cheap `matches!` before any symbol-table lookup, so the default-config hot path is unchanged. Ports the receiver discrimination from eslint-plugin-unicorn ([sindresorhus/eslint-plugin-unicorn#2999](sindresorhus/eslint-plugin-unicorn#2999), issue #2229). ## Test plan - Added `pass` cases covering the exact repro plus object-literal, `const`-bound, TS `as const` / `as` / non-null receivers, function/class/non-string-literal receivers, and the addition-index branch. - Added a `fail` + `fix` case proving string-literal receivers are still reported and fixed. - `cargo test -p oxc_linter` — 1162 passed, 0 failed. Co-authored-by: Cameron <[email protected]>
camc314
added a commit
that referenced
this pull request
Jul 3, 2026
## Summary Fixes #23870. With `checkAllIndexAccess: true`, `unicorn/prefer-at` reported and auto-fixed numeric-key access on plain objects, rewriting e.g. `tokens[60]` (where `const tokens = { 60: '#666666' }`) to `tokens.at(60)`. Plain objects have no `.at()` method, so the autofix produced code that fails to type-check (`TS2339`) / throws at runtime. This skips receivers that are obviously not array-like before reporting under `checkAllIndexAccess`: - object literals — `({ 1: 1 })[1]` - non-string literals, function expressions, and class expressions - identifiers bound to a `const` whose initializer is one of the above — `const o = { 1: 1 }; o[1]` — including through TypeScript `as` / `satisfies` / `!` wrappers Strings keep `.at()`, so string receivers are still reported and fixed (`"abc"[1]` -> `"abc".at(1)`). The guard runs only under the opt-in `checkAllIndexAccess` option and short-circuits on a cheap `matches!` before any symbol-table lookup, so the default-config hot path is unchanged. Ports the receiver discrimination from eslint-plugin-unicorn ([sindresorhus/eslint-plugin-unicorn#2999](sindresorhus/eslint-plugin-unicorn#2999), issue #2229). ## Test plan - Added `pass` cases covering the exact repro plus object-literal, `const`-bound, TS `as const` / `as` / non-null receivers, function/class/non-string-literal receivers, and the addition-index branch. - Added a `fail` + `fix` case proving string-literal receivers are still reported and fixed. - `cargo test -p oxc_linter` — 1162 passed, 0 failed. Co-authored-by: Cameron <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a separate byte handler to the lexer for byte values which should never be encountered:
At present, this function is impossible to reach, because
std::str::Charsensures the next byte is always the start of a valid UTF-8 byte sequence. But later changes I intend introducing unsafe code will make it possible (but highly undesirable!). In the meantime, I don't think it does any harm to handle this case.