Skip to content

fix(isolated-declarations): preserve unresolved parameter types - #25909

Merged
graphite-app[bot] merged 1 commit into
mainfrom
codex/fix-isolated-declarations-indexed-access
Aug 19, 2026
Merged

graphite-app[bot] merged 1 commit into
mainfrom
codex/fix-isolated-declarations-indexed-access

Conversation

@Dunqing

@Dunqing Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member

#25292 made isolated declaration emit report a false TS9025 for a defaulted parameter whose annotation is an indexed access type. This breaks Vite's UserConfig["base"] case when Rolldown upgrades Oxc, even though the indexed property already includes undefined.

Oxc's isolated declaration emitter has no type checker, so it cannot safely resolve indexed accesses, type references, conditional types, or similar annotations. This change uses a syntax-only classification: widen or diagnose types whose syntax is conclusive, and preserve checker-dependent types rather than emit an unstable error. The union/intersection rules retain the distinct behavior of any, never, unknown, and void, and an explicit undefined union member is never duplicated.

Regression coverage includes the Rolldown/Vite case and a compact matrix of the relevant type algebra. The implementation and tests were reduced to one commit after review.

Validated with the isolated-declarations tests, strict Clippy, the all-features workspace suite, lint, docs, AST generation, and TypeScript 5.6.1-rc/6.0.3 type-algebra checks. The full local just ready run stopped only at a runner-dependent source-map snapshot because the repository expects Node 26.5.0 and this machine has 25.8.1; the remaining suite passed with that and the known terminal-color snapshot group skipped.

Follow-up to #25292. Related Rolldown failure: https://github.com/rolldown/rolldown/actions/runs/32158377691/job/95782148042?pr=10707

Disclosure: this change was prepared and verified with AI assistance (Codex).

@github-actions github-actions Bot added the A-isolated-declarations Area - Isolated Declarations label Aug 19, 2026
@codspeed

codspeed Bot commented Aug 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 45 untouched benchmarks
⏩ 36 skipped benchmarks1


Comparing codex/fix-isolated-declarations-indexed-access (8b19972) with main (83cb516)

Open in CodSpeed

Footnotes

  1. 36 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing added the run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it label Aug 19, 2026
@oxc-guard

oxc-guard Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

@Dunqing

Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@codex review

@oxc-guard oxc-guard Bot removed the run-monitor-oxc Add to a PR to dispatch oxc-project/monitor-oxc CI against it label Aug 19, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: daf7d55700

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_isolated_declarations/src/function.rs Outdated
@Dunqing

Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 393da97680

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_isolated_declarations/src/function.rs Outdated
@Dunqing

Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2144ea357a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_isolated_declarations/src/function.rs Outdated
@Dunqing

Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 235a1015aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Dunqing
Dunqing force-pushed the codex/fix-isolated-declarations-indexed-access branch from 235a101 to 8b19972 Compare August 19, 2026 07:30
@Dunqing
Dunqing marked this pull request as ready for review August 19, 2026 07:34
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Aug 19, 2026

Dunqing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Merge activity

#25292 made isolated declaration emit report a false TS9025 for a defaulted parameter whose annotation is an indexed access type. This breaks Vite's `UserConfig["base"]` case when Rolldown upgrades Oxc, even though the indexed property already includes `undefined`.

Oxc's isolated declaration emitter has no type checker, so it cannot safely resolve indexed accesses, type references, conditional types, or similar annotations. This change uses a syntax-only classification: widen or diagnose types whose syntax is conclusive, and preserve checker-dependent types rather than emit an unstable error. The union/intersection rules retain the distinct behavior of `any`, `never`, `unknown`, and `void`, and an explicit `undefined` union member is never duplicated.

Regression coverage includes the Rolldown/Vite case and a compact matrix of the relevant type algebra. The implementation and tests were reduced to one commit after review.

Validated with the isolated-declarations tests, strict Clippy, the all-features workspace suite, lint, docs, AST generation, and TypeScript 5.6.1-rc/6.0.3 type-algebra checks. The full local `just ready` run stopped only at a runner-dependent source-map snapshot because the repository expects Node 26.5.0 and this machine has 25.8.1; the remaining suite passed with that and the known terminal-color snapshot group skipped.

Follow-up to #25292. Related Rolldown failure: https://github.com/rolldown/rolldown/actions/runs/32158377691/job/95782148042?pr=10707

Disclosure: this change was prepared and verified with AI assistance (Codex).
@graphite-app
graphite-app Bot force-pushed the codex/fix-isolated-declarations-indexed-access branch from 8b19972 to 134fb22 Compare August 19, 2026 07:36
@graphite-app
graphite-app Bot merged commit 134fb22 into main Aug 19, 2026
31 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Aug 19, 2026
@graphite-app
graphite-app Bot deleted the codex/fix-isolated-declarations-indexed-access branch August 19, 2026 07:41
graphite-app Bot pushed a commit that referenced this pull request Aug 19, 2026
### 🚀 Features

- 83cb516 minifier: Improve negation of expressions in boolean context (#25755) (Armano)

### 🐛 Bug Fixes

- 134fb22 isolated-declarations: Preserve unresolved parameter types (#25909) (Dunqing)
- 526c2b3 codegen: Remove deprecated test APIs (#25889) (camc314)
- 344a9b2 codegen: Align enum template sourcemaps (#25872) (camc314)

### ⚡ Performance

- 5ef0c93 minifier: Unwrap all unary exprs without creating dummy expressions (#25891) (Armano)
- 8783524 packages/codegen: Remove loop from `growMappingBuffer` (#25886) (overlookmotel)
- b6b14c0 packages/codegen: Remove redundant branch (#25885) (overlookmotel)
- be6adf0 packages/sourcemap: Make `location` passed to `recordSourceMapping` always a number (#25884) (overlookmotel)
- f720f15 packages/codegen: Take named-mapping only code off hot path in `recordSourceMapping` (#25883) (overlookmotel)
- a734af5 packages/codegen: Remove redundant setting of `last` in `printImportAttributes` and `printIf` (#25882) (overlookmotel)
- c6e44c6 packages/codegen: Combine 2 writes in `printJSXElement` (#25881) (overlookmotel)
- 8801662 packages/codegen: Remove redundant mapping from `printParams` (#25880) (overlookmotel)

### 📚 Documentation

- 5ad330e packages/codegen: Reformat comments (#25876) (overlookmotel)
shulaoda added a commit to rolldown/rolldown that referenced this pull request Aug 19, 2026
Rolldown needs Oxc 0.146.0 to pick up the isolated-declarations fix for
unresolved parameter types while retaining the 0.145.0 property-mangler
migration.

### Regression fix

Oxc 0.146.0 includes
[oxc-project/oxc#25909](oxc-project/oxc#25909),
which preserves unresolved parameter types during isolated declaration
generation. This removes the declaration-generation failure observed in
Rolldown's Vite compatibility job.

The Rust workspace dependencies, `Cargo.lock`, and npm catalog packages
(`@oxc-project/runtime`, `@oxc-project/types`, `oxc-parser`, and
`oxc-transform`) now use 0.146.0.

### Breaking changes

Both come from oxc's new property mangler:

- `oxc::minifier::MinifierOptions` gained a required `mangle_properties:
Option<ManglePropertiesOptions>` field. Set to `None` in all three
initializers in `minify_options.rs` — the bundler does not plumb
property mangling through.
- `oxc_minify_napi::MinifyOptions` gained `mangleProps`, which leaked
into rolldown's public `MinifyOptions` (`Omit<BindingMinifyOptions,
'module' | 'sourcemap'>`) and broke the `IsSchemaSubType` assertion in
`validator.ts`. Added `'mangleProps'` to the `Omit`, since
`normalize_binding_options` only reads `mangle`/`compress`/`codegen` and
would silently drop the option.

### Snapshot churn

- **Class bindings are now preserved by DCE** — `(class { static { foo }
})` becomes `var A_keep = class { static { foo } };`. This accounts for
most of the changed snapshots.
- **`@__KEY__` / `#__KEY__` annotations are now emitted** by codegen
(`mangle_props_key_comment*`, `preserve_key_comment`) — groundwork for
the new property mangler.
- **Better template-literal DCE** — `dce_template_literal` drops `var
alsoKeep;` and `${alsoKeep}`, which is provably `undefined` and so has
no `ToString` side effect.

The esbuild compatibility metric is unchanged: `stats.md` stays at 688
passed / 87.42%, and every `summary/*.md` regenerated byte-identical, so
none of these codegen changes move a case across the pass/fail line.

### Transitive updates

`oxc-browserslist` 3.0.11 → 5.0.1, `oxc_sourcemap` 8.1.0 → 8.1.2,
`oxc-miette` 4.0.0 dropped, `lazy-regex` 3.6.1 added (pulled in by the
property mangler).

Verified after rebasing onto current `main`: the full Rust workspace
suite, parser and transformer package smoke tests, native Rolldown debug
binding build, Vite monorepo build and declaration generation, plus 934
unit, 1,128 serve, 690 bundled-serve, and 923 build tests.

Co-authored-by: Dunqing <dengqing0821@gmail.com>
Co-authored-by: Jerry Zhao <165626830+shulaoda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-isolated-declarations Area - Isolated Declarations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Sponsor
SponsoredKunjungi sekarang
Promo