Skip to content

refactor(parser): byte handler for illegal bytes#2229

Merged
Boshen merged 1 commit into
oxc-project:mainfrom
overlookmotel:parser-illegal-bytes
Jan 31, 2024
Merged

refactor(parser): byte handler for illegal bytes#2229
Boshen merged 1 commit into
oxc-project:mainfrom
overlookmotel:parser-illegal-bytes

Conversation

@overlookmotel

@overlookmotel overlookmotel commented Jan 31, 2024

Copy link
Copy Markdown
Member

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.

@github-actions github-actions Bot added the A-parser Area - Parser label Jan 31, 2024
@overlookmotel

Copy link
Copy Markdown
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-hq

codspeed-hq Bot commented Jan 31, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #2229 will not alter performance

⚠️ No base runs were found

Falling back to comparing overlookmotel:parser-illegal-bytes (836fe0d) with main (9c6c17b)

Summary

✅ 17 untouched benchmarks

@overlookmotel

Copy link
Copy Markdown
Member Author

Now rebased on main and ready for merge if you're happy with the change.

@Boshen
Boshen merged commit 5279e89 into oxc-project:main Jan 31, 2024
@overlookmotel
overlookmotel deleted the parser-illegal-bytes branch January 31, 2024 11:07
@overlookmotel

Copy link
Copy Markdown
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants