fix(allocator)!: make Vec non-drop

[overlookmotel]

oxc_allocator::Vec is intended for storing AST types in the arena. Vec is intended to be non-drop, because all AST types are non-drop. If they were not, it would be a memory leak, because those types will not have drop called on them when the allocator is dropped.

However, oxc_allocator::Vec is currently a wrapper around allocator_api2::vec::Vec, which is drop. That unintentionally makes oxc_allocator::Vec drop too.

This PR fixes this by wrapping the inner allocator_api2::vec::Vec in ManuallyDrop. This makes oxc_allocator::Vec non-drop.

The wider consequence of this change is that the compiler is now able to see that loads of other types which contain oxc_allocator::Vec are also non-drop. Once it can prove that, it can remove a load of code which handles dropping these types in the event of a panic. This probably also then allows it to make many further optimizations on that simplified code.

Strictly speaking, this PR is a breaking change. If oxc_allocator::Vec is abused to store drop types, then in some circumstances this change could produce a memory leak where there was none before. However, we've always been clear that only non-drop types should be stored in the arena, so such usage was always a bug.

#6622 fixes the only place in Oxc where we mistakenly stored non-drop types in Vec.

The change to oxc_prettier is because compiler can now deduce that Doc is non-drop, which causes clippy to raise a warning about using then instead of then_some.

As follow-up, we should:

1. Wrap other allocator_api2 types (e.g. IntoIter) in ManuallyDrop too, so compiler can prove they are non-drop too (or reimplement Vec ourselves - test: reduce size of vec #6488).
2. Introduce static checks to prevent non-drop types being used in Box and Vec, to make memory leaks impossible, and detect them at compile time.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

[overlookmotel]

• fix(allocator): prevent putting drop types into arena #6626
• fix(allocator)!: make Vec non-drop #6623 👈
• fix(isolated_declarations): fix potential memory leak #6622
• main

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

Join @overlookmotel and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #6623 will improve performances by 8.41%

_{Comparing 10-16-refactor_allocator_make_vec_non-drop (e1c2d30) with main (de99391)}

Summary

⚡ 3 improvements
✅ 27 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	10-16-refactor_allocator_make_vec_non-drop	Change
⚡	transformer[antd.js]	50.4 ms	47.7 ms	+5.73%
⚡	transformer[checker.ts]	20.2 ms	18.6 ms	+8.41%
⚡	transformer[pdf.mjs]	7.6 ms	7 ms	+8.01%

[Boshen]

What does this all mean?

[overlookmotel]

@Boshen I think this change is worthwhile. It's improving perf pretty much across the board, and #6626 demonstrates that it does not introduce any memory leaks within Oxc.

However, we should probably first check that Rolldown is not erroneously using Vec to store any drop types. I think that's doubtful, but if it was, this change would produce a memory leak in Rolldown. What's the easiest way to do that? Build Rolldown on top of #6626?

Alternatively, we could hold off merging this until we have static checks (like #6626, but more robust) that make it impossible to put Drop types into Box / Vec / the arena.

[Boshen]

I'll test Rolldown after the next oxc release.

[overlookmotel]

OK great. I think this can be merged then. I'll leave it to you to merge Boshen in case you want to check the code first.

[Boshen]

Merge activity

• Oct 19, 11:43 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 19, 11:43 AM EDT: A user added this pull request to the Graphite merge queue.
• Oct 19, 11:48 AM EDT: A user merged this pull request with the Graphite merge queue.