refactor(traverse): simplify pointer manipulation and improve readability

[Dunqing]

close: oxc-project/backlog#17

I don't know too much about raw pointers, the benchmark shows no difference both ways. I don't use addr_of because the documentation has noted this API has been soft-deprecated

[Dunqing]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• refactor(traverse): simplify pointer manipulation and improve readability #9162 👈 (View in Graphite)
• ci(miri): add oxc_transformer to paths #9211
• feat(ast)!: remove AstBuilder::copy #9210
• refactor(transformer/jsx): simplify and remove all AstBuilder::copy usages #9209
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

[codspeed-hq]

CodSpeed Performance Report

Merging #9162 will not alter performance

_{Comparing 02-17-refactor_traverse_simplify_pointer_manipulation_and_improve_readability (c7945c2) with main (611b029)}

Summary

✅ 33 untouched benchmarks

[overlookmotel]

I am pretty sure this change will cause undefined behavior.

There's a subtle difference between these 3:

(assume ptr is a *const Program)

1. &*( (ptr as *const u8).add( offset_of!(Program, span) ) as *const Span )
2. &*addr_of!( (*ptr).span )
3. &(&*ptr).span

(1) is what we have now, and (2) should be equivalent (if my understanding is correct).

They both convert the pointer to Program into a pointer to the Span, without creating a &Program reference in between.

But this PR uses something similar to (3), which is different from the first 2 because it creates an intermediate &Program reference (the &*ptr part deferences pointer *const Program to &Program).

And that's exactly what we need to avoid, because there already exists a live &mut Program - and holding a & reference and a &mut reference to the same thing is a violation of Rust's aliasing rules.

At least this is my understanding. And this is why I'm so cautious about touching Traverse - because it's really at the edges of my understanding. I know enough to be aware that there are subtleties, but I'm not sure if I understand them all. For example, the docs talk about a "value" and a "place" being different, but what exactly is that difference?

The good news is... now that you've removed AstBuilder::copy, we are finally in a position to deploy Miri and find out!

I propose to put this PR on hold for now. Next week I'd like to run the entire transformer conformance suite under Miri (which is slooooow - may take something like 12 - 48 hours) and we can find out whether it's sound in its current state. If it is, then we can look at modifying it, and then test again.

I'm going to mark this PR as "draft" in the meantime.

(sorry this is an incomplete explanation - I don't mean to invoke mysterious "dark magic" and avoid explaining myself - it's just really hard to put into words when I don't have a completely clear mental model myself)