Skip to content

fix(rust): don't mark pure-annotated local calls as execution-order-sensitive#8909

Closed
Dunqing wants to merge 1 commit intomainfrom
fix/remove-pure-annotation-execution-order-sensitive
Closed

fix(rust): don't mark pure-annotated local calls as execution-order-sensitive#8909
Dunqing wants to merge 1 commit intomainfrom
fix/remove-pure-annotation-execution-order-sensitive

Conversation

@Dunqing
Copy link
Copy Markdown
Collaborator

@Dunqing Dunqing commented Mar 26, 2026

Pure-annotated calls to local functions (e.g. /*#__PURE__*/ foo()) should not trigger ExecutionOrderSensitive, since the annotation is a developer contract that the call is side-effect-free. Only Unknown and GlobalVarAccess flags should trigger wrapping.

This also removes the now-unused SideEffectDetail::PureAnnotation flag.

… execution-order-sensitive

Pure-annotated calls to local functions (e.g. `/*#__PURE__*/ foo()`) should
not trigger `ExecutionOrderSensitive`, since the annotation is a developer
contract that the call is side-effect-free. Only `Unknown` and
`GlobalVarAccess` flags should trigger wrapping.

This also removes the now-unused `SideEffectDetail::PureAnnotation` flag.
//#endregion
__esmMin((() => {
init_dep();
nodeAssert.strictEqual(globalThis.value, 1);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The diff is back to the state before #5498

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since foo mutate the globalValue, it should be wrapped to ensure the correct execution order

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for reviewing. I indeed found a case where the mixed issue-4920 and issue-5303 can be broken after this change. Sorry for wasting your time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for reviewing. I indeed found a case where the mixed issue-4920 and issue-5303 can be broken after this change. Sorry for wasting your time.

Don't sweat it! It’s definitely not a waste of time—it was a tricky edge case. I'm just happy to help.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you think it is worth adding a test?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, let's do that.

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 26, 2026

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit 137ce23
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69c4a7f89dcebe000872d402
😎 Deploy Preview https://deploy-preview-8909--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.

@Dunqing Dunqing marked this pull request as ready for review March 26, 2026 03:30
@Dunqing Dunqing requested a review from IWANABETHATGUY March 26, 2026 03:30
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 26, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing fix/remove-pure-annotation-execution-order-sensitive (137ce23) with main (f6cb29d)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 (dd77c52) during the generation of this report, so f6cb29d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@IWANABETHATGUY
Copy link
Copy Markdown
Member

#5303

@Dunqing
Copy link
Copy Markdown
Collaborator Author

Dunqing commented Mar 26, 2026

#5303

This change doesn't affect #5303, and GlobalVarAccess actually already covered this case.

@Dunqing Dunqing closed this Mar 26, 2026
graphite-app bot pushed a commit that referenced this pull request Mar 27, 2026
…eck (#8933)

## Summary
- Adds an integration test combining scenarios from #4920 and #5303 to demonstrate that `PureAnnotation` must be included in the `ExecutionOrderSensitive` check.
- A `/*#__PURE__*/` call to a local function that reads a global variable set by another module must still trigger wrapping, otherwise execution order is broken.

Ref: #8909 (comment)

## Test plan
- [x] New test `pure_annotation_local_fn_reads_global` passes with `just dt`
- [x] Snapshot confirms `dep.js` is wrapped with `__esmMin` in both default and `onDemandWrapping` variants

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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