Skip to content

fix(link_stage): restore inline let-else in exports-kind filter#9237

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-27-chore_revert_refactor
Apr 27, 2026
Merged

fix(link_stage): restore inline let-else in exports-kind filter#9237
graphite-app[bot] merged 1 commit intomainfrom
04-27-chore_revert_refactor

Conversation

@IWANABETHATGUY
Copy link
Copy Markdown
Member

@IWANABETHATGUY IWANABETHATGUY commented Apr 27, 2026

Reverts the iterator restructuring from #9205 in determine_module_exports_kind, restoring the original filter_map(rec.resolved_module) + inline let Module::Normal(importee) = ... else { return; } pattern. The clarified SAFETY and promotion comments introduced in that PR are kept intact — only the structural refactor is rolled back.

We're rolling back the structural change because it appears to have introduced a regression that we have not yet been able to reduce to a reliable repro. Restoring the prior shape lets us unblock users while we continue investigating the root cause; once we have a minimal reproduction we can revisit the refactor with a regression test in place.

Copy link
Copy Markdown
Member Author


How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 27, 2026

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit d295f42
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69ef374820cc3f000882094e
😎 Deploy Preview https://deploy-preview-9237--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@IWANABETHATGUY IWANABETHATGUY changed the title chore: revert refactor refactor(link_stage): restore inline let-else in exports-kind filter Apr 27, 2026
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review April 27, 2026 06:18
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 27, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing 04-27-chore_revert_refactor (d295f42) with main (9f42ff3)2

Open in CodSpeed

Footnotes

  1. 10 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.

  2. No successful run was found on main (d295f42) during the generation of this report, so 9f42ff3 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@sapphi-red sapphi-red force-pushed the 04-27-chore_revert_refactor branch from 65b56dc to acab0b9 Compare April 27, 2026 10:06
@sapphi-red
Copy link
Copy Markdown
Member

I added a test case. You can run it with cargo test -p rolldown --test integration --release -- cjs_require_esm_none_exports_kind. Note that you need --release as this only happens with release build.

The output of it without fix:

//#region \0rolldown/runtime.js
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __commonJSMin = (cb, mod) => () => (mod || (cb((mod = { exports: {} }).exports, mod), cb = null), mod.exports);
var __copyProps = (to, from, except, desc) => {
	if (from && typeof from === "object" || typeof from === "function") for (var keys = __getOwnPropNames(from), i = 0, n = keys.length, key; i < n; i++) {
		key = keys[i];
		if (!__hasOwnProp.call(to, key) && key !== except) __defProp(to, key, {
			get: ((k) => from[k]).bind(null, key),
			enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable
		});
	}
	return to;
};
var __toESM = (mod, isNodeMode, target) => (target = mod != null ? __create(__getProtoOf(mod)) : {}, __copyProps(isNodeMode || !mod || !mod.__esModule ? __defProp(target, "default", {
	value: mod,
	enumerable: true
}) : target, mod));
//#endregion
//#region aligner.js
var require_aligner = /* @__PURE__ */ __commonJSMin((() => {
	var Aligner = function() {};
	Aligner.prototype.align = function() {};
}));
//#endregion
//#region default_aligner.js
var require_default_aligner = /* @__PURE__ */ __commonJSMin(((exports, module) => {
	var Aligner = (require_aligner(), __toCommonJS(aligner_exports));
	var DefaultAligner = function() {};
	DefaultAligner.prototype = Object.create(Aligner);
	module.exports = DefaultAligner;
}));
require_aligner();
var import_default_aligner = /* @__PURE__ */ __toESM(require_default_aligner());
console.log(import_default_aligner.default);
//#endregion

(references __toCommonJS and aligner_exports which are not declared.

The output with the fix:

//#region \0rolldown/runtime.js
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __esmMin = (fn, res) => () => (fn && (res = fn(fn = 0)), res);
var __commonJSMin = (cb, mod) => () => (mod || (cb((mod = { exports: {} }).exports, mod), cb = null), mod.exports);
var __exportAll = (all, no_symbols) => {
	let target = {};
	for (var name in all) __defProp(target, name, {
		get: all[name],
		enumerable: true
	});
	if (!no_symbols) __defProp(target, Symbol.toStringTag, { value: "Module" });
	return target;
};
var __copyProps = (to, from, except, desc) => {
	if (from && typeof from === "object" || typeof from === "function") for (var keys = __getOwnPropNames(from), i = 0, n = keys.length, key; i < n; i++) {
		key = keys[i];
		if (!__hasOwnProp.call(to, key) && key !== except) __defProp(to, key, {
			get: ((k) => from[k]).bind(null, key),
			enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable
		});
	}
	return to;
};
var __toESM = (mod, isNodeMode, target) => (target = mod != null ? __create(__getProtoOf(mod)) : {}, __copyProps(isNodeMode || !mod || !mod.__esModule ? __defProp(target, "default", {
	value: mod,
	enumerable: true
}) : target, mod));
var __toCommonJS = (mod) => __hasOwnProp.call(mod, "module.exports") ? mod["module.exports"] : __copyProps(__defProp({}, "__esModule", { value: true }), mod);
//#endregion
//#region aligner.js
var aligner_exports = /* @__PURE__ */ __exportAll({});
var Aligner;
var init_aligner = __esmMin((() => {
	Aligner = function() {};
	Aligner.prototype.align = function() {};
}));
//#endregion
//#region default_aligner.js
var require_default_aligner = /* @__PURE__ */ __commonJSMin(((exports, module) => {
	var Aligner = (init_aligner(), __toCommonJS(aligner_exports));
	var DefaultAligner = function() {};
	DefaultAligner.prototype = Object.create(Aligner);
	module.exports = DefaultAligner;
}));
//#endregion
//#region main.js
init_aligner();
var import_default_aligner = /* @__PURE__ */ __toESM(require_default_aligner());
console.log(import_default_aligner.default);
//#endregion

The explanation by claude:

In the buggy code, the filter_map closure holds &self.module_table for the entire iterator chain lifetime. LLVM sees this immutable reference and applies optimizations that exploit the immutability guarantee — effectively caching or eliding reads of exports_kind from self.module_table. The unsafe mutation via addr_of!(*importee).cast_mut() violates this guarantee, causing stale reads. The fix moves as_normal() back into for_each, so filter_map no longer captures &self.module_table. This changes the LLVM IR structure such that the specific optimization exploiting the immutable reference is not applied. The unsafe mutations of exports_kind take effect as intended.

Note: the fundamental UB still exists (the outer iterator self.module_table.modules.iter() holds an immutable reference while for_each mutates through it). A proper fix would eliminate the unsafe mutation entirely, e.g., by collecting mutations and applying them after the iterator completes, or using Cell/interior mutability.

@sapphi-red sapphi-red changed the title refactor(link_stage): restore inline let-else in exports-kind filter fix(link_stage): restore inline let-else in exports-kind filter Apr 27, 2026
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

I think we should merge this, but I think we should try to avoid the unsafe here.

@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app Bot commented Apr 27, 2026

Merge activity

Reverts the iterator restructuring from #9205 in `determine_module_exports_kind`, restoring the original `filter_map(rec.resolved_module)` + inline `let Module::Normal(importee) = ... else { return; }` pattern. The clarified `SAFETY` and promotion comments introduced in that PR are kept intact — only the structural refactor is rolled back.

We're rolling back the structural change because it appears to have introduced a regression that we have not yet been able to reduce to a reliable repro. Restoring the prior shape lets us unblock users while we continue investigating the root cause; once we have a minimal reproduction we can revisit the refactor with a regression test in place.
@graphite-app graphite-app Bot force-pushed the 04-27-chore_revert_refactor branch from a72548e to d295f42 Compare April 27, 2026 10:15
@graphite-app graphite-app Bot merged commit d295f42 into main Apr 27, 2026
33 checks passed
@graphite-app graphite-app Bot deleted the 04-27-chore_revert_refactor branch April 27, 2026 10:19
@IWANABETHATGUY
Copy link
Copy Markdown
Member Author

I think we should merge this, but I think we should try to avoid the unsafe here.

agree

This was referenced Apr 29, 2026
shulaoda added a commit that referenced this pull request Apr 29, 2026
## [1.0.0-rc.18] - 2026-04-29

### 💥 BREAKING CHANGES

- optimization: default unspecified inlineConst.mode to smart (#9248) by @IWANABETHATGUY

### 🐛 Bug Fixes

- rolldown_plugin_vite_import_glob: return error instead of panicking when virtual module uses a relative glob (#9241) by @shulaoda
- binding: treat empty inlineConst object as omitted (#9247) by @IWANABETHATGUY
- rolldown: keep enum declaration for optional-chain access (#9229) by @Dunqing
- link_stage: restore inline let-else in exports-kind filter (#9237) by @IWANABETHATGUY
- dev/lazy: avoid module reinitialization in lazy compilation patches (#9179) by @h-a-n-a
- dev: visit identifier references for runtime rewrites in HMR finalizer (#9191) by @h-a-n-a
- chunk-optimizer: pick dominator for runtime placement to avoid cycles (#9164) by @IWANABETHATGUY
- make `this.emitFile` chunk path synchronous to avoid deadlock (#9031) by @lazarv
- use sentinel id for `browser: false` ignored modules (#9192) by @shulaoda
- prevent chunk optimizer from creating import cycles (#9228) by @IWANABETHATGUY

### 🚜 Refactor

- replace tokio::sync::Mutex with std::sync::Mutex for non-IO data (#9176) by @shulaoda
- rolldown_plugin_vite_import_glob: do not rewrite import path for absolute base (#9195) by @shulaoda
- runtime_helper: wrap DependedRuntimeHelperMap in a struct (#9215) by @IWANABETHATGUY
- drop redundant clear() in determine_safely_merge_cjs_ns (#9206) by @IWANABETHATGUY
- clean up generate_lazy_export (#9208) by @IWANABETHATGUY
- bitset: return bool from set_bit to fuse guard-and-set (#9207) by @IWANABETHATGUY
- link_stage: simplify exports-kind filter and clarify safety comments (#9205) by @IWANABETHATGUY

### 📚 Documentation

- determine_module_exports_kind (#9252) by @IWANABETHATGUY
- fix dead link to esbuild ESM/CJS interop tests (#9230) by @Copilot
- remove CSS bundling references (#9234) by @shulaoda
- correct IncrementalFullBuild row in BundleMode table (#9214) by @IWANABETHATGUY
- design: add bundler data lifecycle design doc (#9212) by @hyf0
- remove minifier alpha status notices (#9202) by @sapphi-red

### ⚙️ Miscellaneous Tasks

- upgrade oxc to 0.128.0 (#9260) by @shulaoda
- deps: bump rolldown-ariadne to 0.6.0 (#9254) by @IWANABETHATGUY
- deps: update github actions (#9259) by @renovate[bot]
- deps: update github actions (#9258) by @renovate[bot]
- remove renovate overrides (#9257) by @Boshen
- use ubuntu-latest for security workflow (#9256) by @Boshen
- notify Discord around release publish (#9251) by @Boshen
- add release environment to npm publish workflow (#9250) by @Boshen
- justfile: drop the `--` separator before forwarded args in `vp run` (#9246) by @shulaoda
- deps: update test262 submodule for tests (#9243) by @sapphi-red
- add more tracing instrumentations (#9220) by @sapphi-red
- rolldown_plugin_vite_import_glob: remove outdated sourcemap doc comment (#9213) by @shulaoda
- update security workflow (#9201) by @Boshen

### ❤️ New Contributors

* @lazarv made their first contribution in [#9031](#9031)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
@rolldown-guard rolldown-guard Bot mentioned this pull request Apr 29, 2026
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.

2 participants